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

Hi Ajin,

Some review comments for patch v14-0001.

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

1.
+ * We also try and filter changes that are not relevant for logical decoding
+ * as well as give the option for plugins to filter changes in advance.
+ * Determining whether to filter a change requires information about the
+ * relation from the catalog, requring a transaction to be started.
+ * When most changes in a transaction are unfilterable, the overhead of
+ * starting a transaction for each record is significant. To reduce this
+ * overhead a hash cache of relation file locators is created. Even then a
+ * hash search for every record before recording has considerable overhead
+ * especially for use cases where most tables in an instance are
not filtered.
+ * To further reduce this overhead a simple approach is used to suspend
+ * filtering for a certain number of changes CHANGES_THRESHOLD_FOR_FILTER
+ * when an unfilterable change is encountered. In other words, continue
+ * filtering changes if the last record was filtered out. If an unfilterable
+ * change is found, skip filtering the next CHANGES_THRESHOLD_FOR_FILTER
+ * changes.
+ *

1a.
/try and filter/try to filter/

~

1b.
There is some leading whitespace problem happening (spaces instead of tabs?)

~

1c.
Minor rewording

SUGGESTION (e.g. anyway this should be identical to the commit message text)

Determining whether to filter a change requires information about the
relation and the publication from the catalog, which means a
transaction must be started. But, the overhead of starting a
transaction for each record is significant. To reduce this overhead a
hash cache of relation file locators is used to remember which
relations are filterable.

Even so, doing a hash search for every record has considerable
overhead, especially for scenarios where most tables in an instance
are published. To further reduce overheads a simple approach is used:
When an unfilterable change is encountered we suspend filtering for a
certain number (CHANGES_THRESHOLD_FOR_FILTER) of subsequent changes.
In other words, continue filtering until an unfilterable change is
encountered; then skip filtering the next CHANGES_THRESHOLD_FOR_FILTER
changes, before attempting filtering again.

~~~

2.
+/*
+ * After encountering a change that cannot be filtered out, filtering is
+ * temporarily suspended. Filtering resumes after processing every 100 changes.
+ * This strategy helps to minimize the overhead of performing a hash table
+ * search for each record, especially when most changes are not filterable.
+ */
+#define CHANGES_THRESHOLD_FOR_FILTER 100

Maybe you can explain where this magic number comes from.

SUGGESTION
The CHANGES_THRESHOLD_FOR_FILTER value of 100 was chosen as the best
trade-off value after performance tests were carried out using
candidate values 10, 50, 100, and 200.

~~~

ReorderBufferQueueChange:

3.
+ /*
+ * If filtering was suspended and we've crossed the change threshold,
+ * attempt to filter again
+ */
+ if (!rb->can_filter_change && (++rb->unfiltered_changes_count
+ >= CHANGES_THRESHOLD_FOR_FILTER))
+ {
+ rb->can_filter_change = true;
+ rb->unfiltered_changes_count = 0;
+ }
+

/If filtering was suspended/If filtering is currently suspended/

~~~

ReorderBufferGetRelation:

4.
+static Relation
+ReorderBufferGetRelation(ReorderBuffer *rb, RelFileLocator *rlocator,
+ bool has_tuple)

My suggested [2-#4] name change 'ReorderBufferGetRelationForDecoding'
is not done yet. I saw Kuroda-san also said this name was confusing
[1-#02], and suggested something similar
'GetPossibleDecodableRelation'.

~~~

RelFileLocatorCacheInvalidateCallback:

5.
+ /*
+ * If relid is InvalidOid, signaling a complete reset, we must remove
+ * all entries, otherwise just remove the specific relation's entry.
+ * Always remove negative cache entries.
+ */
+ if (relid == InvalidOid || /* complete reset */
+ entry->relid == InvalidOid || /* invalid cache entry */
+ entry->relid == relid) /* individual flushed relation */
+ {
+ if (hash_search(RelFileLocatorFilterCache,
+ &entry->key,
+ HASH_REMOVE,
+ NULL) == NULL)
+ elog(ERROR, "hash table corrupted");
+ }

5a.
IMO the relid *parameter* should be mentioned explicitly to
disambiguate relid from the entry->relid.

/If relid is InvalidOid, signaling a complete reset,/If a complete
reset is requested (when 'relid' parameter is InvalidOid),/

~

5b.
/Always remove negative cache entries./Remove any invalid cache
entries (these are indicated by invalid entry->relid)/

~~~

ReorderBufferFilterByRelFileLocator:

6.
I previously [2-#7] had suggested this function code could be
refactored to share some common return logic. It is not done, but OTOH
there is no reply, so I don't know if it was overlooked or simply
rejected.

======
src/include/replication/reorderbuffer.h

7.
+ /* should we try to filter the change? */
+ bool can_filter_change;
+
+ /* number of changes after a failed attempt at filtering */
+ int8 unfiltered_changes_count;
+

The potential renaming of that 'can_filter_change' field to something
better is still an open item IMO [2-#8] pending consensus on what a
better name for this might be.

======
[1] https://www.postgresql.org/message-id/OSCPR01MB14966021B3390856464C5E27FF5C42%40OSCPR01MB14966.jpnprd01.prod.outlook.com
[2] https://www.postgresql.org/message-id/CAHut%2BPtrLu%3DYrxo_YQ-LC%2BLSOEUYmuFo2brjCQ18JM9-Vi2DwQ%40mail.gmail.com

Kind Regards,
Peter Smith.
Fujitsu Australia