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