Re: Single transaction in the tablesync worker? - Mailing list pgsql-hackers

On Tue, Feb 9, 2021 at 12:02 PM Peter Smith <smithpb2250@gmail.com> wrote:
>
> Here are my feedback comments for the V29 patch.
>
> ====
>
> FILE: logical-replication.sgml
>
> +    slots have generated names:
> <quote><literal>pg_%u_sync_%u_%llu</literal></quote>
> +    (parameters: Subscription <parameter>oid</parameter>,
> +    Table <parameter>relid</parameter>, system
> identifier<parameter>sysid</parameter>)
> +   </para>
>
> 1.
> There is a missing space before the sysid parameter.
>
> =====
>
> FILE: subscriptioncmds.c
>
> + * SUBREL_STATE_FINISHEDCOPY. The apply worker can also
> + * concurrently try to drop the origin and by this time the
> + * origin might be already removed. For these reasons,
> + * passing missing_ok = true from here.
> + */
> + snprintf(originname, sizeof(originname), "pg_%u_%u", sub->oid, relid);
> + replorigin_drop_by_name(originname, true, false);
> + }
>
> 2.
> Don't really need to say "from here".
> (same comment applies multiple places, in this file and in tablesync.c)
>
> 3.
> Previously the tablesync origin name format was encapsulated in a
> common function. IMO it was cleaner/safer how it was before, instead
> of the same "pg_%u_%u" cut/paste and scattered in many places.
> (same comment applies multiple places, in this file and in tablesync.c)
>

Fixed all the three above comments.

> 4.
> Calls like replorigin_drop_by_name(originname, true, false); make it
> unnecessarily hard to read code when the boolean params are neither
> named as variables nor commented. I noticed on another thread [et0205]
> there was an idea that having no name/comments is fine because anyway
> it is not difficult to figure out when using a "modern IDE", but since
> my review tools are only "vi" and "meld" I beg to differ with that
> justification.
> (same comment applies multiple places, in this file and in tablesync.c)
>

Already responded to it separately. I went ahead and removed such
comments from other places in the patch.

> [et0205] https://www.postgresql.org/message-id/c1d9833f-eeeb-40d5-89ba-87674e1b7ba3%40www.fastmail.com
>
> =====
>
> FILE: tablesync.c
>
> 5.
> Previously there was a function tablesync_replorigin_drop which was
> encapsulating the tablesync origin name formatting. I thought that was
> better than the V29 code which now has the same formatting scattered
> over many places.
> (same comment applies for worker_internal.h)
>

I am not sure what different you are expecting here than point-3?

> + * Determine the tablesync slot name.
> + *
> + * The name must not exceed NAMEDATALEN - 1 because of remote node constraints
> + * on slot name length. We do append system_identifier to avoid slot_name
> + * collision with subscriptions in other clusters. With current scheme
> + * pg_%u_sync_%u_UINT64_FORMAT (3 + 10 + 6 + 10 + 20 + '\0'), the maximum
> + * length of slot_name will be 50.
> + *
> + * The returned slot name is either:
> + * - stored in the supplied buffer (syncslotname), or
> + * - palloc'ed in current memory context (if syncslotname = NULL).
> + *
> + * Note: We don't use the subscription slot name as part of tablesync slot name
> + * because we are responsible for cleaning up these slots and it could become
> + * impossible to recalculate what name to cleanup if the subscription slot name
> + * had changed.
> + */
> +char *
> +ReplicationSlotNameForTablesync(Oid suboid, Oid relid, char
> syncslotname[NAMEDATALEN])
> +{
> + if (syncslotname)
> + sprintf(syncslotname, "pg_%u_sync_%u_" UINT64_FORMAT, suboid, relid,
> + GetSystemIdentifier());
> + else
> + syncslotname = psprintf("pg_%u_sync_%u_" UINT64_FORMAT, suboid, relid,
> + GetSystemIdentifier());
> +
> + return syncslotname;
> +}
>
> 6.
> "We do append" --> "We append"
> "With current scheme" -> "With the current scheme"
>

Fixed.

> 7.
> Maybe consider to just assign GetSystemIdentifier() to a static
> instead of calling that function for every slot?
> static uint64 sysid = GetSystemIdentifier();
> IIUC the sysid value is never going to change for a process, right?
>

Already responded.

> FILE: alter_subscription.sgml
>
> 8.
> +  <para>
> +   Commands <command>ALTER SUBSCRIPTION ... REFRESH ..</command> and
> +   <command>ALTER SUBSCRIPTION ... SET PUBLICATION ..</command> with refresh
> +   option as true cannot be executed inside a transaction block.
> +  </para>
>
> My guess is those two lots of double dots ("..") were probably meant
> to be ellipsis ("...")
>

Fixed, for the first one I completed the command by adding PUBLICATION.

>
> When looking at the DropSubscription code I noticed that there is a
> small difference between the HEAD code and the V29 code when slot_name
> = NONE.
>
> HEAD does
> ------
>     if (!slotname)
>     {
>         table_close(rel, NoLock);
>         return;
>     }
> ------
>
> V29 does
> ------
>         if (!slotname)
>         {
>             /* be tidy */
>             list_free(rstates);
>             return;
>         }
> ------
>
> Isn't the V29 code missing doing a table_close(rel, NoLock) there?
>

Yes, good catch. Fixed.

-- 
With Regards,
Amit Kapila.