Re: Proposal: Filter irrelevant change before reassemble transactions during logical decoding

Hi Ajin,

Some review comments for patch v17-0002.

======
src/backend/replication/logical/decode.c

1.
/*
+ * Check if filtering changes before decoding is supported and we're
not suppressing filter
+ * changes currently.
+ */
+static inline bool
+FilterChangeIsEnabled(LogicalDecodingContext *ctx)
+{
+ return (ctx->callbacks.filter_change_cb != NULL &&
+ ctx->reorder->try_to_filter_change);
+}
+

I still have doubts about the need for/benefits of this to be a
separate function.

It is only called from *one* place within the other static function,
FilterChange.

Just putting that code inline seems just as readable as maintaining
the separate function for it.

SUGGESTION:
static inline bool
FilterChange(LogicalDecodingContext *ctx, XLogRecPtr origptr, TransactionId xid,
RelFileLocator *target_locator, ReorderBufferChangeType
change_type)
{
return
ctx->callbacks.filter_change_cb &&
ctx->reorder->try_to_filter_change &&
ReorderBufferFilterByRelFileLocator(ctx->reorder, xid, origptr,
target_locator,
change_type));
}

======
src/backend/replication/logical/reorderbuffer.c

RelFileLocatorCacheInvalidateCallback:

2.
if (hash_search(RelFileLocatorFilterCache,
- &entry->key,
- HASH_REMOVE,
- NULL) == NULL)
+ &entry->key,
+ HASH_REMOVE,
+ NULL) == NULL)
elog(ERROR, "hash table corrupted");

I think this whitespace change belongs back in patch 0001 where this
function was introduced, not here in patch 0002.

~~~

ReorderBufferFilterByRelFileLocator:

3.
+ /*
+ * Quick return if we already know that the relation is not to be
+ * decoded. These are for special relations that are unlogged and for
+ * sequences and catalogs.
+ */
+ if (entry->filterable)
+ return true;

/These are for/This is for/

~~~

4.
if (RelationIsValid(relation))
{
- entry->relid = RelationGetRelid(relation);
+ if (IsToastRelation(relation))
+ {
+ char *toast_name = RelationGetRelationName(relation);
+ int n PG_USED_FOR_ASSERTS_ONLY;
+
+ n = sscanf(toast_name, "pg_toast_%u", &entry->relid);
+
+ Assert(n == 1);
+ }
+ else
+ entry->relid = RelationGetRelid(relation);
+
entry->filterable = false;
+ rb->try_to_filter_change = rb->filter_change(rb, entry->relid, change_type,
+ true, &cache_valid);
RelationClose(relation);
}
else
{
entry->relid = InvalidOid;
- entry->filterable = true;
+ rb->try_to_filter_change = entry->filterable = true;
}

rb->try_to_filter_change = entry->filterable;
~

Something seems fishy here. AFAICT, the rb->try_to_filter_change will
already be assigned in both the *if* and the *else* blocks. So, why is
it being overwritten again outside that if/else?

======
Kind Regards,
Peter Smith.
Fujitsu Australia