Re: [HACKERS] logical decoding of two-phase transactions

Hi Nikhil,

I've been looking at this patch series, and I do have a bunch of
comments and questions, as usual ;-)

Overall, I think it's clear the main risk associated with this patch is
the decode group code - it touches PROC entries, so a bug may cause
trouble pretty easily. So I've focused on this part, for now.

1) LogicalLockTransaction does roughly this

...

if (MyProc->decodeGroupLeader == NULL)
{
leader = AssignDecodeGroupLeader(txn->xid);

if (leader == NULL ||
!BecomeDecodeGroupMember((PGPROC *)leader, txn->xid))
goto lock_cleanup;
}

leader = BackendXidGetProc(txn->xid);
if (!leader)
goto lock_cleanup;

leader_lwlock = LockHashPartitionLockByProc(leader);
LWLockAcquire(leader_lwlock, LW_EXCLUSIVE);

pgxact = &ProcGlobal->allPgXact[leader->pgprocno];
if(pgxact->xid != txn->xid)
{
LWLockRelease(leader_lwlock);
goto lock_cleanup;
}

...

I wonder why we need the BackendXidGetProc call after the first if
block. Can we simply grab MyProc->decodeGroupLeader at that point?

2) InitProcess now resets decodeAbortPending/decodeLocked flags, while
checking decodeGroupLeader/decodeGroupMembers using asserts. Isn't that
a bit strange? Shouldn't it do the same thing with both?

3) A comment in ProcKill says this:

* Detach from any decode group of which we are a member. If the leader
* exits before all other group members, its PGPROC will remain allocated
* until the last group process exits; that process must return the
* leader's PGPROC to the appropriate list.

So I'm wondering what happens if the leader dies before other group
members, but the PROC entry gets reused for a new connection. It clearly
should not be a leader for that old decode group, but it may need to be
a leader for another group.

4) strange hunk in ProcKill

There seems to be some sort of merge/rebase issue, because this block of
code (line ~880) related to lock groups

/* Return PGPROC structure (and semaphore) to appropriate freelist */
proc->links.next = (SHM_QUEUE *) *procgloballist;
*procgloballist = proc;

got replaced by code relared to decode groups. That seems strange.

5) ReorderBufferCommitInternal

I see the LogicalLockTransaction() calls in ReorderBufferCommitInternal
have vastly variable comments. Some calls have no comment, some calls
have "obvious" comment like "Lock transaction before catalog access" and
one call has this very long comment

/*
* Output plugins can access catalog metadata and we
* do not have any control over that. We could ask
* them to call
* LogicalLockTransaction/LogicalUnlockTransaction
* APIs themselves, but that leads to unnecessary
* complications and expectations from plugin
* writers. We avoid this by calling these APIs
* here, thereby ensuring that the in-progress
* transaction will be around for the duration of
* the apply_change call below
*/

I find that rather inconsistent, and I'd say those comments are useless.
I suggest to remove all the per-call comments and instead add a comment
about the locking into the initial file-level comment, which already
explains handling of large transactions, etc.

regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services