From: | Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> |
---|---|
To: | Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> |
Cc: | "tanghy(dot)fnst(at)fujitsu(dot)com" <tanghy(dot)fnst(at)fujitsu(dot)com>, vignesh C <vignesh21(at)gmail(dot)com>, Greg Nancarrow <gregn4422(at)gmail(dot)com>, Peter Eisentraut <peter(dot)eisentraut(at)enterprisedb(dot)com>, "houzj(dot)fnst(at)fujitsu(dot)com" <houzj(dot)fnst(at)fujitsu(dot)com>, "osumi(dot)takamichi(at)fujitsu(dot)com" <osumi(dot)takamichi(at)fujitsu(dot)com>, Alexey Lesovsky <lesovsky(at)gmail(dot)com>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Skipping logical replication transactions on subscriber side |
Date: | 2022-01-21 11:55:22 |
Message-ID: | CAA4eK1LOMxfnzduPBEF0H-xyH8b_RA9Fn16F=f7DRiLzaJhf2g@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Fri, Jan 21, 2022 at 10:10 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> On Fri, Jan 21, 2022 at 1:20 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)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.
Attachment | Content-Type | Size |
---|---|---|
v10-0001-Add-ALTER-SUBSCRIPTION-.-SKIP-to-skip-the-transa.patch | application/octet-stream | 59.5 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Amit Kapila | 2022-01-21 12:13:12 | Re: Skipping logical replication transactions on subscriber side |
Previous Message | Andrey Lepikhov | 2022-01-21 11:09:01 | Re: POC: GROUP BY optimization |