Horton

Re: Skipping logical replication transactions on subscriber side

Amit Kapila <amit.kapila16@gmail.com>

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

Attachments

On Fri, Jan 21, 2022 at 10:10 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Fri, Jan 21, 2022 at 1:20 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > What do we want to indicate by [, ... ]? To me, it appears like
> > multiple options but that is not what we support currently.
>
> You're right. It's an oversight.
>

I have fixed this and a few other things in the attached patch.
1.
The newly added column needs to be updated in the following statement:
-- All columns of pg_subscription except subconninfo are publicly readable.
REVOKE ALL ON pg_subscription FROM public;
GRANT SELECT (oid, subdbid, subname, subowner, subenabled, subbinary,
              substream, subtwophasestate, subslotname, subsynccommit,
subpublications)
    ON pg_subscription TO public;

2.
+stop_skipping_changes(bool clear_subskipxid, XLogRecPtr origin_lsn,
+   TimestampTz origin_timestamp)
+{
+ Assert(is_skipping_changes());
+
+ ereport(LOG,
+ (errmsg("done skipping logical replication transaction %u",
+ skip_xid)));

Isn't it better to move this LOG at the end of this function? Because
clear* functions can give an error, so it is better to move it after
that. I have done that in the attached.

3.
+-- fail - must be superuser
+SET SESSION AUTHORIZATION 'regress_subscription_user2';
+ALTER SUBSCRIPTION regress_testsub SKIP (xid = 100);
+ERROR:  must be owner of subscription regress_testsub

This test doesn't seem to be right. You want to get the error for the
superuser but the error is for the owner. I have changed this test to
do what it intends to do.

Apart from this, I have changed a few comments and ran pgindent. Do
let me know what you think of the changes?

Few things that I think we can improve in 028_skip_xact.pl are as follows:

After CREATE SUBSCRIPTION, wait for initial sync to be over and
two_phase state to be enabled. Please see 021_twophase. For the
streaming case, we might be able to ensure streaming even with lesser
data. Can you please try that?

-- 
With Regards,
Amit Kapila.

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