Re: logical decoding and replication of sequences, take 2 - Mailing list pgsql-hackers

FWIW, here are some more minor review comments for v20231127-3-0001

======
doc/src/sgml/logicaldecoding.sgml

1.
+      The <parameter>txn</parameter> parameter contains meta information about
+      the transaction the sequence change is part of. Note however that for
+      non-transactional updates, the transaction may be NULL, depending on
+      if the transaction already has an XID assigned.
+      The <parameter>sequence_lsn</parameter> has the WAL location of the
+      sequence update. <parameter>transactional</parameter> says if the
+      sequence has to be replayed as part of the transaction or directly.

/says if/specifies whether/

======
src/backend/commands/sequence.c

2. DecodeSeqTuple

+ memcpy(((char *) tuple->tuple.t_data),
+    data + sizeof(xl_seq_rec),
+    SizeofHeapTupleHeader);
+
+ memcpy(((char *) tuple->tuple.t_data) + SizeofHeapTupleHeader,
+    data + sizeof(xl_seq_rec) + SizeofHeapTupleHeader,
+    datalen);

Maybe I am misreading but isn't this just copying 2 contiguous pieces
of data? Won't a single memcpy of (SizeofHeapTupleHeader + datalen)
achieve the same?

======
.../replication/logical/reorderbuffer.c

3.
+ *   To decide if a sequence change is transactional, we maintain a hash
+ *   table of relfilenodes created in each (sub)transactions, along with
+ *   the XID of the (sub)transaction that created the relfilenode. The
+ *   entries from substransactions are copied to the top-level transaction
+ *   to make checks cheaper. The hash table gets cleaned up when the
+ *   transaction completes (commit/abort).

/substransactions/subtransactions/

~~~

4.
+ * A naive approach would be to just loop through all transactions and check
+ * each of them, but there may be (easily thousands) of subtransactions, and
+ * the check happens for each sequence change. So this could be very costly.

/may be (easily thousands) of/may be (easily thousands of)/

~~~

5. ReorderBufferSequenceCleanup

+ while ((ent = (ReorderBufferSequenceEnt *)
hash_seq_search(&scan_status)) != NULL)
+ {
+ (void) hash_search(txn->toptxn->sequences_hash,
+    (void *) &ent->rlocator,
+    HASH_REMOVE, NULL);
+ }

Typically, other HASH_REMOVE code I saw would check result for NULL to
give elog(ERROR, "hash table corrupted");

~~~

6. ReorderBufferQueueSequence

+ if (xid != InvalidTransactionId)
+ txn = ReorderBufferTXNByXid(rb, xid, true, NULL, lsn, true);

How about using the macro: TransactionIdIsValid

~~~

7. ReorderBufferQueueSequence

+ if (reloid == InvalidOid)
+ elog(ERROR, "could not map filenode \"%s\" to relation OID",
+ relpathperm(rlocator,
+ MAIN_FORKNUM));

How about using the macro: OidIsValid

~~~

8.
+ /*
+ * Calculate the first value of the next batch (at which point we
+ * generate and decode another WAL record.
+ */

Missing ')'

~~~

9. ReorderBufferAddRelFileLocator

+ /*
+ * We only care about sequence relfilenodes for now, and those always have
+ * a XID. So if there's no XID, don't bother adding them to the hash.
+ */
+ if (xid == InvalidTransactionId)
+ return;

How about using the macro: TransactionIdIsValid

~~~

10. ReorderBufferProcessTXN

+ if (reloid == InvalidOid)
+ elog(ERROR, "could not map filenode \"%s\" to relation OID",
+ relpathperm(change->data.sequence.locator,
+ MAIN_FORKNUM));

How about using the macro: OidIsValid

~~~

11. ReorderBufferChangeSize

+ if (tup)
+ {
+ sz += sizeof(HeapTupleData);
+ len = tup->tuple.t_len;
+ sz += len;
+ }

Why is the 'sz' increment split into 2 parts?

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