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

Hi Tomas,

>> Unfortunately, this does segfault for me in `make check` almost
immediately. Try

This is due to the new ERROR handling code that I added today for the
lock/unlock APIs. Will fix.

>> Also, current value for LOGICALREP_IS_COMMIT is 1, but previous code expected
flags to be zero, so this way logical replication between postgres-10 and
postgres-with-2pc-decoding will be broken.

Good point. Will also test pg-10 to pg-11 logical replication to
ensure that there are no issues.

>> So I think we need a subscription parameter to enable/disable this,
defaulting to 'disabled'.

Ok, will add it to the "CREATE SUBSCRIPTION", btw, we should have
allowed storing options in an array form for a subscription. We might
add more options in the future and adding fields one by one doesn't
seem that extensible.

> 1) twophase.c
> ---------
>
> I think this comment is slightly inaccurate:
>
> /*
> * Coordinate with logical decoding backends that may be already
> * decoding this prepared transaction. When aborting a transaction,
> * we need to wait for all of them to leave the decoding group. If
> * committing, we simply remove all members from the group.
> */
>
> Strictly speaking, we're not waiting for the workers to leave the
> decoding group, but to set decodeLocked=false. That is, we may proceed
> when there still are members, but they must be in unlocked state.
>

Agreed. Will modify it to mention that it will wait only if some of
the backends are in locked state.

>
> 2) reorderbuffer.c
> ------------------
>
> I've already said it before, I find the "flags" bitmask and rbtxn_*
> macros way less readable than individual boolean flags. It was claimed
> this was done on Andres' request, but I don't see that in the thread. I
> admit it's rather subjective, though.
>

Yeah, this is a little subjective.

> I see ReorederBuffer only does the lock/unlock around apply_change and
> RelationIdGetRelation. That seems insufficient - RelidByRelfilenode can
> do heap_open on pg_class, for example. And I guess we need to protect
> rb->message too, because who knows what the plugin does in the callback?
>
> Also, we should not allocate gid[GIDSIZE] for every transaction. For
> example subxacts never need it, and it seems rather wasteful to allocate
> 200B when the rest of the struct has only ~100B. This is particularly
> problematic considering ReorderBufferTXN is not spilled to disk when
> reaching the memory limit. It needs to be allocated ad-hoc only when
> actually needed.
>

OK, will look at allocating GID only when needed.

Regards,
Nikhils
--
Nikhil Sontakke http://www.2ndQuadrant.com/
PostgreSQL/Postgres-XL Development, 24x7 Support, Training & Services