Re: Single transaction in the tablesync worker?

On Mon, Jan 4, 2021 at 10:48 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> Few more comments on v9:
> ======================
> 1.
> + /* Drop the tablesync slot. */
> + {
> + char *syncslotname = ReplicationSlotNameForTablesync(subid, relid);
> +
> + /*
> + * If the subscription slotname is NONE/NULL and the connection to publisher is
> + * broken, but the DropSubscription should still be allowed to complete.
> + * But without a connection it is not possible to drop any tablesync slots.
> + */
> + if (!wrconn)
> + {
> + /* FIXME - OK to just log a warning? */
> + elog(WARNING, "!!>> DropSubscription: no connection. Cannot drop
> tablesync slot \"%s\".",
> + syncslotname);
> + }
>
> Why is this not an ERROR? We don't want to keep the table slots
> lingering after DropSubscription. If there is any tablesync slot that
> needs to be dropped and the publisher is not available then we should
> raise an error.

Previously there was only the subscription slot. If the connection was
broken and caused an error then it was still possible for the user to
disassociate the subscription from the slot using ALTER SUBSCRIPTION
... SET (slot_name = NONE). And then (when the slotname is NULL) the
DropSubscription could complete OK. I expect in that case the Admin
still had some slot clean-up they would need to do on the Publisher
machine.

But now we have the tablesync slots so if I caused them to give ERROR
when the connection is broken then the subscription would become
un-droppable. If you think that having ERROR and an undroppable
subscription is better than the current WARNING then please let me
know - there is no problem to change it.

> 2.
> + /*
> + * Tablesync resource cleanup (slots and origins).
> + *
> + * Any READY-state relations would already have dealt with clean-ups.
> + */
> + {
>
> There is no need to start a separate block '{' here.

Written this way so I can declare variables only at the scope they are
needed. I didn’t see anything in the PG code conventions discouraging
doing this practice: https://www.postgresql.org/docs/devel/source.html

> 3.
> +#define SUBREL_STATE_COPYDONE 'C' /* tablesync copy phase is completed */
>
> You can mention in the comments that sublsn will be NULL for this
> state as it is mentioned for other similar states. Can we think of
> using any letter in lower case for this as all other states are in
> lower-case except for this which makes it a look bit odd? We can use
> 'f' or 'e' and describe it as 'copy finished' or 'copy end'. I am fine
> if you have any better ideas.
>

Fixed in latest patch [v11]

> 4.
> LogicalRepSyncTableStart()
> {
> ..
> ..
> +copy_table_done:
> +
> + /* Setup replication origin tracking. */
> + {
> + char originname[NAMEDATALEN];
> + RepOriginId originid;
> +
> + snprintf(originname, sizeof(originname), "pg_%u_%u",
> MySubscription->oid, MyLogicalRepWorker->relid);
> + originid = replorigin_by_name(originname, true);
> + if (!OidIsValid(originid))
> + {
> + /*
> + * Origin tracking does not exist. Create it now, and advance to LSN
> got from walrcv_create_slot.
> + */
> + elog(LOG, "!!>> LogicalRepSyncTableStart: 1 replorigin_create
> \"%s\".", originname);
> + originid = replorigin_create(originname);
> + elog(LOG, "!!>> LogicalRepSyncTableStart: 1 replorigin_session_setup
> \"%s\".", originname);
> + replorigin_session_setup(originid);
> + replorigin_session_origin = originid;
> + elog(LOG, "!!>> LogicalRepSyncTableStart: 1 replorigin_advance
> \"%s\".", originname);
> + replorigin_advance(originid, *origin_startpos, InvalidXLogRecPtr,
> + true /* go backward */ , true /* WAL log */ );
> + }
> + else
> + {
> + /*
> + * Origin tracking already exists.
> + */
> + elog(LOG, "!!>> LogicalRepSyncTableStart: 2 replorigin_session_setup
> \"%s\".", originname);
> + replorigin_session_setup(originid);
> + replorigin_session_origin = originid;
> + elog(LOG, "!!>> LogicalRepSyncTableStart: 2
> replorigin_session_get_progress \"%s\".", originname);
> + *origin_startpos = replorigin_session_get_progress(false);
> + }
> ..
> ..
> }
>
> I am not sure if this code is correct because, for the very first time
> when the copy is done, we don't expect replication origin to exist
> whereas this code will silently use already existing replication
> origin which can lead to a wrong start position for the slot. In such
> a case it should error out. I guess we should create the replication
> origin before making the state as copydone. I feel we should even have
> a test case for this as it is not difficult to have a pre-existing
> replication origin.
>

Fixed as suggested in latest patch [v11]

> 5. Is it possible to write a testcase where we fail (say due to pk
> violation or some other error) after the initial copy is done, then
> remove the conflicting row and allow a copy to be completed? If we
> already have any such test then it is fine.
>

Causing a PK violation during the initial copy is not a problem to
test, but doing it after the initial copy is difficult. I have done
exactly this test scenario before but I thought it cannot be
automated. E.g. To cause an PK violation error somewhere between
COPYDONE and SYNDONE means that the offending insert (the one which
tablesync will fail to replicate) has to be sent while the tablesync
is in CATCHUP mode. But AFAIK that can only be achieved using the
debugger to get the timing right.

> 6.
> +/*
> + * Drop the replication slot at the publisher node
> + * using the replication connection.
> + */
>
> This comment looks a bit odd. The first line appears to be too short.
> We have limit of 80 chars but this is much lesser than that.
>

Fixed in latest patch [v11]

> 7.
> @@ -905,7 +905,7 @@ replorigin_advance(RepOriginId node,
> LWLockAcquire(&replication_state->lock, LW_EXCLUSIVE);
>
> /* Make sure it's not used by somebody else */
> - if (replication_state->acquired_by != 0)
> + if (replication_state->acquired_by != 0 &&
> replication_state->acquired_by != MyProcPid)
> {
>

TODO

> I think you won't need this change if you do replorigin_advance before
> replorigin_session_setup in your patch.
>
> 8.
> - * that ensures we won't loose knowledge about that after a crash if the
> + * that ensures we won't lose knowledge about that after a crash if the
>
> It is better to submit this as a separate patch.
>

Done. Please see CF entry. https://commitfest.postgresql.org/32/2926/

----
[v11] = https://www.postgresql.org/message-id/CAHut%2BPu0A6TUPgYC-L3BKYQfa_ScL31kOV_3RsB3ActdkL1iBQ%40mail.gmail.com

Kind Regards,
Peter Smith.
Fujitsu Australia.