Re: Skipping logical replication transactions on subscriber side

On Fri, Jan 21, 2022 at 1:18 AM Peter Eisentraut
<peter(dot)eisentraut(at)enterprisedb(dot)com> wrote:
>
> On 18.01.22 07:05, Masahiko Sawada wrote:
> > I've attached a rebased patch.
>
> I think this is now almost done. Attached I have a small fixup patch
> with some documentation proof-reading, and removing some comments I felt
> are redundant. Some others have also sent you some documentation
> updates, so feel free to merge mine in with them.

Thank you for reviewing the patch and attaching the fixup patch!

>
> Some other comments:
>
> parse_subscription_options() and AlterSubscriptionStmt mixes regular
> options and skip options in ways that confuse me. It seems to work
> correctly, though. I guess for now it's okay, but if we add more skip
> options, it might be better to separate those more cleanly.

Agreed.

>
> I think the superuser check in AlterSubscription() might no longer be
> appropriate. Subscriptions can now be owned by non-superusers. Please
> check that.

IIUC we don't allow non-superuser to own the subscription yet. We
still have the following superuser checks:

In CreateSubscription():

if (!superuser())
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg("must be superuser to create subscriptions")));

and in AlterSubscriptionOwner_internal();

/* New owner must be a superuser */
if (!superuser_arg(newOwnerId))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg("permission denied to change owner of
subscription \"%s\"",
NameStr(form->subname)),
errhint("The owner of a subscription must be a superuser.")));

Also, doing superuser check here seems to be consistent with
pg_replication_origin_advance() which is another way to skip
transactions and also requires superuser permission.

>
> The display order in psql \dRs+ is a bit odd. I would put it at the
> end, certainly not between Two phase commit and Synchronous commit.

Fixed.

>
> Please run pgperltidy over 028_skip_xact.pl.

Fixed.

>
> Is the setting of logical_decoding_work_mem in the test script required?
> If so, comment why.

Yes, it makes the tests check streaming logical replication cases
easily. Added the comment.

>
> Please document arguments origin_lsn and origin_timestamp of
> stop_skipping_changes(). Otherwise, one has to dig quite deep to find
> out what they are for.

Added.

Also, after reading the documentation updates, I realized that there
are two paragraphs describing almost the same things so merged them.
Please check the doc updates in the latest patch.

I've attached an updated patch that incorporated these commends as
well as other comments I got so far.

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/