Horton

Re: Skipping logical replication transactions on subscriber side

Masahiko Sawada <sawada.mshk@gmail.com>

View thread View on postgr.es
From: Masahiko Sawada <sawada.mshk@gmail.com>
To: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
Cc: "tanghy.fnst@fujitsu.com" <tanghy.fnst@fujitsu.com>, Amit Kapila <amit.kapila16@gmail.com>, vignesh C <vignesh21@gmail.com>, Greg Nancarrow <gregn4422@gmail.com>, "houzj.fnst@fujitsu.com" <houzj.fnst@fujitsu.com>, Alexey Lesovsky <lesovsky@gmail.com>, PostgreSQL-development <pgsql-hackers@postgresql.org>, "osumi.takamichi@fujitsu.com" <osumi.takamichi@fujitsu.com>
Date: 2022-01-21T03:08:28Z
Lists: pgsql-hackers

Attachments

On Fri, Jan 21, 2022 at 1:18 AM Peter Eisentraut
<peter.eisentraut@enterprisedb.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/

Commits

  1. Test ALIGNOF_DOUBLE==4 compatibility under ALIGNOF_DOUBLE==8.

  2. Reorder subskiplsn in pg_subscription to avoid alignment issues.

  3. Add ALTER SUBSCRIPTION ... SKIP.

  4. Optionally disable subscriptions on error.

  5. Update docs of logical replication for commit 8d74fc96db.

  6. Respect permissions within logical replication.

  7. Fix regression test failure caused by commit 8d74fc96db.

  8. Add a view to show the stats of subscription workers.

  9. Add logical change details to logical replication worker errcontext.

  10. Rename LOGICAL_REP_MSG_STREAM_END to LOGICAL_REP_MSG_STREAM_STOP.

  11. Fix typo in protocol.sgml.

  12. Remove unused argument in apply_handle_commit_internal().

  13. Fix replication of in-progress transactions in tablesync worker.

  14. Reorder pg_sequence columns to avoid alignment issue