Re: Changeset Extraction v7.6.1

On Fri, Feb 14, 2014 at 4:55 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> [ new patches ]

0001 already needs minor

+ * copied stuff from tuptoaster.c. Perhaps there should be toast_internal.h?

Yes, please. If you can submit a separate patch creating this file
and relocating this stuff there, I will commit it.

+ /*
+ * XXX: It's impolite to ignore our argument and keep decoding until the
+ * current position.
+ */

Eh, what?

+ * We misuse the original meaning of SnapshotData's xip and
subxip fields
+ * to make the more fitting for our needs.
[...]
+ * XXX: Do we want extra fields instead of misusing existing
ones instead?

If we're going to do this, then it surely needs to be documented in
snapshot.h. On the second question, you're not the first hacker to
want to abuse the meanings of the existing fields; SnapshotDirty
already does it. It's tempting to think we need a more principled
approach to this, like what we've done with Node i.e. typedef enum ...
SnapshotType; and then a separate struct definition for each kind, all
beginning with SnapshotType type.

+ /*
+ * XXX: Timeline handling/naming. Do we need to include the timeline in
+ * snapshot's name? Outside of very obscure, user triggered, cases every
+ * LSN should correspond to exactly one timeline?
+ */

I can't really comment intelligently on this, so you need to figure it
out. My off-the-cuff thought is that erring on the side of including
it couldn't hurt anything.

+ * XXX: use hex format for the LSN as well?

Yes, please.

+ /* prepared abort contain a normal
commit abort... */

contains.

+ /*
+ * Abort all transactions that we keep
track of that are older
+ * than the record's oldestRunningXid.
This is the most
+ * convenient spot for doing so since,
in contrast to shutdown
+ * or end-of-recovery checkpoints, we
have sufficient
+ * knowledge to deal with prepared
transactions here.
+ */

I have no real quibble with this, but I think the comment could be
clarified slightly to state *what* knowledge we have here that we
wouldn't have there.

+ /* only crash recovery/replication needs to care */

I believe I know what you're getting at here, but the next guy might
not. I suggest: "Although these records only exist to serve the needs
of logical decoding, all the work happens as part of crash or archive
recovery, so we don't need to do anything here."

+ * Treat HOT update as normal updates, there
is no useful

s/, t/. T/

+ * There are cases in which inplace updates
are used without xids
+ * assigned (like VACUUM), there are others
(like CREATE INDEX
+ * CONCURRENTLY) where an xid is present. If
an xid was assigned

In-place updates can be used either by XID-bearing transactions (e.g.
in CREATE INDEX CONCURRENTLY) or by XID-less transactions (e.g.
VACUUM). In the former case, ...

+ * redundant because the commit will do that
as well, but one
+ * we'll support decoding in-progress
relations, this will be

s/one/once/
s/we'll/we/

+ /* we don't care about row level locks for now */
+ case XLOG_HEAP_LOCK:
+ break;

The position of the comment isn't consistent with the comments for the
other WAL record type in this section; that is, it's above rather than
below the case.

+ * transaction's contents as the various caches need to always be

I think you should use "since" or "because" rather than "as" here, and
maybe put a comma before it.

+ * the transaction's invalidations. This currently won't be needed if
+ * we're just skipping over the transaction, since that currently only
+ * happens when starting decoding, after we invalidated all caches, but
+ * transactions in other databases might have touched shared relations.

I'm having trouble understanding what this means, especially the part
after the "but".

+ * Read a HeapTuple as WAL logged by heap_insert, heap_update and
+ * heap_delete, but not by heap_multi_insert into a tuplebuf.

"but not by heap_multi_insert" needs punctuation both before and
after. You can just add a comma after, or change it into a
parenthetical phrase.

As the above comments probably make clear, I'm pretty much happy with decode.c.

+ /* TODO: We got to change that someday soon.. */

Two periods. Maybe "We need to change this some day soon." - and then
follow that with a paragraph explaining what roughly what would need
to be done.

+ /* shorter lines... */
+ slot = MyReplicationSlot;
+
+ /* first some sanity checks that are unlikely to be violated */
+ if (MyReplicationSlot == NULL)
+ elog(ERROR, "cannot perform logical decoding without a
acquired slot");

Can test slot.

+ /* make sure the passed slot is suitable, these are user
facing errors */

Make sure the passed slot is suitable. These are user-facing errors.

+ if (IsTransactionState() &&
+ GetTopTransactionIdIfAny() != InvalidTransactionId)
+ ereport(ERROR,
+ (errcode(ERRCODE_ACTIVE_SQL_TRANSACTION),
+ errmsg("cannot perform changeset
extraction in transaction that has performed writes")));

This is sort of an awkward restriction, as it makes it hard to compose
this feature with others. What underlies the restriction, can we get
rid of it, and if not can we include a comment here explaining why it
exists?

+ * the xlog records didn't result in anyting
relevant for

anything.

+ /* register output plugin name with slot */
+ strncpy(NameStr(slot->data.plugin), plugin,
+ NAMEDATALEN);
+ NameStr(slot->data.plugin)[NAMEDATALEN - 1] = '\0';

If it's safe to do this without a lock, I don't know why.

More broadly, I wonder why this is_init stuff is in here at all.
Maybe the stuff that happens in the is_init case should be done in the
caller, or another helper function.

+ /* prevent WAL removal as fast as possible */
+ ReplicationSlotsComputeRequiredLSN();

If there's a race here, can't we rejigger the order of operations to
eliminate it? Or is that just too hard and not worth it?

+begin_txn_wrapper(ReorderBuffer *cache, ReorderBufferTXN *txn)
+ state.callback_name = "pg_decode_begin_txn";
+ ctx->callbacks.begin_cb(ctx, txn);

I feel that there's a certain lack of naming consistency between these
things. Can we improve that? (and similarly for parallel cases)

+pg_create_decoding_replication_slot(PG_FUNCTION_ARGS)

I thought we were going to have physical and logical slots, not
physical and decoding slots.

+ /* make sure we don't end up with an unreleased slot */
+ PG_TRY();
+ {
...
+ PG_CATCH();
+ {
+ ReplicationSlotRelease();
+ ReplicationSlotDrop(NameStr(*name));
+ PG_RE_THROW();
+ }
+ PG_END_TRY();

I don't think this is a very good idea. The problem with doing things
during error recovery that can themselves fail is that you'll lose the
original error, which is not cool, and maybe even blow out the error
stack. Many people have confuse PG_TRY()/PG_CATCH() with an
exception-handling system, but it's not. One way to fix this is to
put some of the initialization logic in ReplicationSlotCreate() just
prior to calling CreateSlotOnDisk(). If the work that needs to be
done is too complex or protracted to be done there, then I think that
it should be pulled out of the act of creating the replication slot
and made to happen as part of first use, or as a separate operation
like PrepareForLogicalDecoding.

+ * When the client has confirmed flushes >= candidate_xmin_after we can

candidate_xmin_after is not otherwise referenced; incomplete identifier rename?

Nothing in patch 1 sets PROC_IN_LOGICAL_DECODING. Is that right?

+ProcArraySetReplicationSlotXmin(TransactionId xmin, TransactionId catalog_xmin,
+ bool
already_locked)

Maybe Assert(!already_locked || LWLockHeldByMe(ProcArrayLock))

+void
+ProcArrayGetReplicationSlotXmin(TransactionId *xmin,
+
TransactionId *catalog_xmin)
+{
+ LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);

If we need the lock in exclusive mode here, there should be a comment
explaining why. And regardless, there should probably be some sort of
header comment.

+ /*
+ * Acquire spinlock so other backends are guaranteed
to see this in
+ * time - we cannot generally acquire the lwlock here
since we might
+ * be still holding it in an error path.
+ */
+ SpinLockAcquire(&MyReplicationSlot->mutex);
+ MyReplicationSlot->active = false;
+ SpinLockRelease(&MyReplicationSlot->mutex);
+
+ /* might not have been set when we've been a plain slot */
+ LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);
+ MyPgXact->vacuumFlags &= ~PROC_IN_LOGICAL_DECODING;
+ LWLockRelease(ProcArrayLock);

OK, a couple of things. First, the comment for the first chunk says
we can't acquire the LWLock here, and then the second part acquires an
LWLock. Second, if we're about to dump our PGPROC altogether, why do
we need to update vacuumFlags first? Third, this isn't actually
called anywhere.

+ * At some point in the future it probaly makes sense to have a more elaborate
+ * resource management here, but it's not entirely clear how that would look
+ * like.

s/how/what/

ReorderBufferGetTXN() should get a comment about the performance
impact of this. There's a tiny bit there in ReorderBufferReturnTXN()
but it should be better called out. Should these call the valgrind
macros to make the memory inaccessible while it's being held in cache?

My eyes are starting to glaze over, so more later.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company