On Mon, Dec 9, 2024 at 7:43 PM vignesh C wrote:
>
> On Mon, 9 Dec 2024 at 16:25, Shubham Khanna
> wrote:
> >
> > Hi all,
> >
> > I am writing to propose the addition of the two_phase option in
> > pg_createsubscriber. As discussed in [1], supporting thi
ify the two_phase to
> true.
>
I have fixed the given comments. The v2 version patch attached at [1]
has the changes for the same.
[1] -
https://www.postgresql.org/message-id/CAHv8RjLcdmz%3D_RMwveuDdr8i7r%3D09TAwtOnFmXeaia_v2RmnYA%40mail.gmail.com
Thanks and Regards,
Shubham Khanna.
ther boolean variants.
>
Fixed.
> ==
> .../t/040_pg_createsubscriber.pl
>
> 11.
> +# Run pg_createsubscriber on a promoted server with two_phase=on
> +command_ok(
> + [
> + 'pg_createsubscriber', '--verbose',
> + '--recovery-timeout', "$PostgreSQL::Test::Utils::timeout_default",
> + '--pgdata', $node_b->data_dir,
> + '--publisher-server', $node_a->connstr($db10),
> + '--subscriber-port', $node_b->port,
> + '--database', $db10,
> + '--two_phase=on'
> + ],
> + 'created subscription with two-phase commit enabled');
>
> Hmm. I expect this should have been specified as '--two-phase=on'
> (dash instead of underscore for consistency with all other switches of
> pg_createsubscriber) but previous typos (e.g. #8 above) have
> compounded the confusion.
>
Fixed.
The v2 version patch attached at [1] has the changes for the same.
[1] -
https://www.postgresql.org/message-id/CAHv8RjLcdmz%3D_RMwveuDdr8i7r%3D09TAwtOnFmXeaia_v2RmnYA%40mail.gmail.com
Thanks and Regards,
Shubham Khanna.
oing more, so maybe it should give more details like "In
> passing, also test the --enable-two-phase option."
>
> ~~~
>
> 12.
> +# Verify that the prepared transaction is replicated to the subscriber
> +my $count_prepared_s =
> + $node_s->safe_psql($db1, "SELECT count(*) FROM pg_prepared_xacts;");
> +
> +is($count_prepared_s, qq(0), 'Prepared transaction replicated to
> subscriber');
> +
>
> Is this test OK? It says to verify it is replicated. How does checking
> subscriber has zero pg_prepared_xacts ensure replication occurred?
>
> ==
> Please see the attached NITPICKS diffs which includes some (not all)
> of the above suggestions.
>
> ==
I have fixed the given comments. The attached patch contains the
suggested changes.
Thanks and regards,
Shubham Khanna.
v3-0001-Add-support-for-two-phase-commit-in-pg_createsubs.patch
Description: Binary data
On Wed, Dec 11, 2024 at 10:59 AM vignesh C wrote:
>
> On Tue, 10 Dec 2024 at 17:17, Shubham Khanna
> wrote:
> >
> > On Mon, Dec 9, 2024 at 7:43 PM vignesh C wrote:
> >
> > The attached Patch contains the required changes.
>
> Few comments:
> 1) This is
reatesubscriber.pl
>
> 6.
> +is($count_prepared_s, qq(1), 'Prepared transaction replicated to
> subscriber');
>
> Should there also have been an earlier check (way back before the
> PREPARE) just to make sure this count was initially 0?
>
Removed this and added a new test case instead of this.
The attached patch contains the suggested changes.
Thanks and regards,
Shubham Khanna.
v5-0001-Add-support-for-two-phase-commit-in-pg_createsubs.patch
Description: Binary data
patch attached at [1]
has the changes for the same.
[1] -
https://www.postgresql.org/message-id/CAHv8Rj%2Bhd2MTNRs4AsK6%3DhRqvV6JC9g2tTAJwGjrNfXg6vhD8g%40mail.gmail.com
Thanks and Regards,
Shubham Khanna.
$count_prepared_s =
> + $node_s->safe_psql($db1, "SELECT count(*) FROM pg_prepared_xacts;");
> +
> +is($count_prepared_s, qq(1), 'Prepared transaction replicated to
> subscriber');
>
I have fixed the given comment. The v5 version patch attached at [1]
has the changes for the same.
[1] -
https://www.postgresql.org/message-id/CAHv8Rj%2Bhd2MTNRs4AsK6%3DhRqvV6JC9g2tTAJwGjrNfXg6vhD8g%40mail.gmail.com
Thanks and Regards,
Shubham Khanna.
> 3b.
> Also, if you are going to name char-codes (like 'p') in comments, it
> might be helpful to include the equivalent words, saving readers from
> having to search the documentation to find the meaning.
>
I have fixed the given comments. The attached patch contains the
suggested changes.
Thanks and regards,
Shubham Khanna.
v6-0001-Add-support-for-two-phase-commit-in-pg_createsubs.patch
Description: Binary data
; to occur).
>
> So, SQL like the one below might be the best:
>
> # Verify that all subtwophase states are pending or enabled,
> # e.g. there are no subscriptions where subtwophase is disabled ('d').
> SELECT count(1) = 0 FROM pg_subscription WHERE subtwophasestate IN ('d&#
On Fri, Dec 13, 2024 at 2:39 PM vignesh C wrote:
>
> On Fri, 13 Dec 2024 at 13:01, Shubham Khanna
> wrote:
> >
> > On Fri, Dec 13, 2024 at 12:20 PM Peter Smith wrote:
> > >
> > > Hi Shubham.
> > >
> > > Here are my review comments for
ggestion and updated the 'pg_createsubscriber'
documentation accordingly.
The attached Patch contains the suggested change.
Thanks and regards,
Shubham Khanna.
v7-0001-Validate-max_slot_wal_keep_size-in-pg_createsubsc.patch
Description: Binary data
> pg_size_bytes()
> to get max_slot_wal_keep_size.
>
I have fixed the given comment. The v7 version Patch attached at [1]
has the changes for the same.
[1] -
https://www.postgresql.org/message-id/CAHv8Rj%2BSxmBgz-vw5f7meRzEh6pgp9YhAjL5BPzTpevu31rY7Q%40mail.gmail.com
Thanks and regards,
Shubham Khanna.
ase find the updated patch.
Thanks and regards,
Shubham Khanna.
v12-0001-Validate-max_slot_wal_keep_size-in-pg_createsubs.patch
Description: Binary data
Hi,
Following the recent comments on Patch v13-0001, the patch was not
applied to the HEAD. I have addressed the feedback and prepared a
rebased version to incorporate the necessary adjustments.
Please find the updated patch.
Thanks and regards,
Shubham Khanna.
v14-0001-Add-support-for-two
. The attached patch has the changes for the
same.
[1] -
https://www.postgresql.org/message-id/CAA4eK1%2BJpALAokLqxVsQKgo9iFrO-zChfvNXXJMkC8jUgYykBw%40mail.gmail.com
Thanks and regards,
Shubham Khanna.
v1-0001-Enhance-pg_createsubscriber-to-fetch-and-append-a.patch
Description: Binary data
x27;--subscriber-port', $node_s->port,
> + '--publication', 'pub1',
> + '--publication', 'pub2',
> + '--subscription', 'sub1',
> + '--subscription', 'sub2',
> + '--database&#
On Mon, Jan 20, 2025 at 1:34 AM Shlok Kyal wrote:
>
> On Fri, 17 Jan 2025 at 09:52, Shubham Khanna
> wrote:
> >
> > On Fri, Jan 17, 2025 at 5:43 AM Peter Smith wrote:
> > >
> > > Hi Shubham,
> > >
> > > Some review comments
pLUHCeDN%2Be5p0CsdQxcKEOiWRnRjxfg%40mail.gmail.com
Thanks and regards,
Shubham Khanna.
.blogspot.com/2024/09/online-upgrading-logical-and-physical.html
Thanks and regards,
Shubham Khanna.
v1-0001-Support-for-dropping-all-publications-in-pg_creat.patch
Description: Binary data
-1 in the log file
> 'tmp_check/log/regress_log_040_pg_createsubscriber', which may not
> even be from the same test. Am I looking in the wrong place (???) e.g.
> Where is the logging evidence of that publisher's bad GUC (10MB) value
> in the logs before the warning is emitted?
>
I have fixed the given comments using your suggestions. The attached
patch contains the suggested changes.
Thanks and Regards,
Shubham Khanna.
v9-0001-Validate-max_slot_wal_keep_size-in-pg_createsubsc.patch
Description: Binary data
ot;. The actual value
> is of no real importance to the patch, so going to extra trouble to
> extract an int64 that we don't care about seems unnecessary
>
I have fixed the given comment and used the string to accept the
value. The v9 version Patch attached at [1] has the changes for the
same.
[1] -
https://www.postgresql.org/message-id/CAHv8RjL4MFEDosz9%2BTFmufYCDoNCnyN6PFeSxG6MwsH1ufhZLA%40mail.gmail.com
Thanks and regards,
Shubham Khanna.
er
> ...
> --
>
> See the attached diff for the TAP changes I used to expose this
> logging. Apply this atop v8.
>
> ==
> [1]
> https://www.postgresql.org/message-id/CAHut%2BPszk61QM%2BcEvq_1-A2y%2BJrAD0USB%2BNvtcidajYOfHDkyw%40mail.gmail.com
>
I have used your diff to update the TAP tests accordingly. The v9
version Patch attached at [1] has the changes for the same.
[1] -
https://www.postgresql.org/message-id/CAHv8RjL4MFEDosz9%2BTFmufYCDoNCnyN6PFeSxG6MwsH1ufhZLA%40mail.gmail.com
Thanks and regards,
Shubham Khanna.
_warning_details)
> The max_slot_wal_keep_size parameter is currently set to a non-default
> value, which may lead to replication failures. This parameter must be
> set to -1 to ensure that required WAL files are not prematurely
> removed.
>
> SUGGESTION (pg_log_warning_hint)
> Set the configuration parameter \"%s\" to -1 to ensure that required
> WAL files are not prematurely removed.
>
I have fixed the given comments using your suggestions. Tha attached
patch contains the suggested changes.
Thanks and Regards,
Shubham Khanna.
v6-0001-Validate-max_slot_wal_keep_size-in-pg_createsubsc.patch
Description: Binary data
in
> that case it might be a poor user experience if we are effectively
> saying: "Too bad, it's all broken now; we warned you (only if you
> tried a dry run), but you did not listen".
>
> In other words, why not make this an error condition up-front to
> entirely remove any chance of this failure?
>
Changed the warning condition to the error condition and also updated
the test case accordingly.
> ==
The attached Patch contains the suggested changes.
Thanks and regards,
Shubham Khanna.
v4-0001-Validate-max_slot_wal_keep_size-in-pg_createsubsc.patch
Description: Binary data
On Tue, Jan 14, 2025 at 4:53 PM Shlok Kyal wrote:
>
> On Fri, 27 Dec 2024 at 12:06, Shubham Khanna
> wrote:
> >
> > On Fri, Dec 27, 2024 at 11:30 AM vignesh C wrote:
> > > > > > >
> > > The documentation requires a minor update: instead of sp
rltidy' and made the necessary
adjustments.
The attached patch includes these formatting fixes along with the
latest changes.
Thanks and regards,
Shubham Khanna.
v10-0001-Validate-max_slot_wal_keep_size-in-pg_createsubs.patch
Description: Binary data
On Wed, Jan 15, 2025 at 3:54 PM Ajin Cherian wrote:
>
>
>
> On Wed, Jan 15, 2025 at 5:33 PM Shubham Khanna
> wrote:
> > Previously, the warning was necessary because the 'two-phase' option
> > was not available, and users needed to be informed about the defa
On Thu, Jan 16, 2025 at 4:53 AM Peter Smith wrote:
>
> On Wed, Jan 15, 2025 at 9:24 PM Ajin Cherian wrote:
> >
> >
> >
> > On Wed, Jan 15, 2025 at 5:33 PM Shubham Khanna
> > wrote:
> > > Previously, the warning was necessary because the 'tw
On Thu, Jan 16, 2025 at 3:15 PM Shlok Kyal wrote:
>
> On Wed, 15 Jan 2025 at 12:03, Shubham Khanna
> wrote:
> >
> > On Tue, Jan 14, 2025 at 4:53 PM Shlok Kyal wrote:
> > >
> > > On Fri, 27 Dec 2024 at 12:06, Shubham Khanna
> > > wrote:
> >
On Thu, Jan 16, 2025 at 12:12 PM Shlok Kyal wrote:
>
> On Thu, 16 Jan 2025 at 10:48, Shubham Khanna
> wrote:
> >
> > On Wed, Jan 15, 2025 at 3:28 AM Peter Smith wrote:
> > >
> > > Hi Shubham.
> > >
> > > Patch v9-0001 LGTM.
> > >
> 3b.
> I checked other PG source code, but I couldn't find any examples where
> the switch is given in single quotes quite like this.
>
> Maybe choose from one of the below instead:
>
> SUGGESTION #1
> Use the \"--enable-two-phase\" switch to enable two_phase.
&
isn't it enough to
> use INT64_FORMAT macro?
>
> ```
> +# Configure 'max_slot_wal_keep_size = 1' on the publisher and
> +# reload configuration
> +$node_p->append_conf('postgresql.conf', 'max_slot_wal_keep_size = 1');
> +$node_p->rel
://www.postgresql.org/message-id/be92c57b-82e1-4920-ac31-a8a04206db7b%40app.fastmail.com
Thanks and regards,
Shubham Khanna.
v1-0001-Validate-max_slot_wal_keep_size-in-pg_createsubsc.patch
Description: Binary data
om
> [2]
> https://www.postgresql.org/message-id/CALDaNm09cRzke52UN5zx33PT390whU92oXY4gfOSZEo17CLPjw%40mail.gmail.com
If 'max_slot_wal_keep_size' is not set to -1, there is no surety that
'pg_createsubscriber' will function as expected and the removal of WAL
files will start to occur. Therefore, a warning is raised instead of
an error to alert users about the potential configuration issue.
If 'max_slot_wal_keep_size' is -1 (the default), replication slots may
retain an unlimited amount of WAL files.
The attached Patch contains the suggested changes.
Thanks and regards,
Shubham Khanna.
v5-0001-Validate-max_slot_wal_keep_size-in-pg_createsubsc.patch
Description: Binary data
On Thu, Feb 13, 2025 at 5:36 PM Shlok Kyal wrote:
>
> On Thu, 13 Feb 2025 at 15:20, Shubham Khanna
> wrote:
> >
> > On Tue, Feb 11, 2025 at 9:56 PM Shlok Kyal wrote:
> > >
> > > On Tue, 11 Feb 2025 at 09:51, Shubham Khanna
> > > wrote:
>
them and adding publications.
>
> IMO a better choice of adjectives can be found below.
> "no suitable databases found..."
> "no appropriate databases found..."
> "no eligible databases found..."
> etc.
>
Fixed.
The attached patch contains the required changes.
Thanks and regards,
Shubham Khanna.
v8-0001-Enhance-pg_createsubscriber-to-fetch-and-append-a.patch
Description: Binary data
On Tue, Feb 11, 2025 at 9:56 PM Shlok Kyal wrote:
>
> On Tue, 11 Feb 2025 at 09:51, Shubham Khanna
> wrote:
> >
> > On Fri, Feb 7, 2025 at 7:46 AM Hayato Kuroda (Fujitsu)
> > wrote:
> > >
> > > Dear Shubham,
> > >
> > > Thanks for u
> patch seems not to do. Below example shows the case when three publications
> exist.
>
> ```
> pg_createsubscriber: dropping publications "aaa, bbb, ccc" in database
> "postgres"
> ```
>
> I think the output must be `"aaa", "bbb", "ccc"`. This means
> drop_publication() should be refactored.
>
> [1]:
> https://www.postgresql.org/docs/devel/error-style-guide.html#ERROR-STYLE-GUIDE-QUOTES
>
Fixed.
The attached patch at [1] contains the suggested changes.
[1] -
https://www.postgresql.org/message-id/CAHv8RjKGywu%2B3cAv9MPgxi1_TUXBT8yzUsW%2Bf%3Dg5UsgeJ8Uk6g%40mail.gmail.com
Thanks and regards,
Shubham Khanna.
On Wed, Feb 12, 2025 at 5:29 AM Peter Smith wrote:
>
> On Tue, Feb 11, 2025 at 9:16 PM Shubham Khanna
> wrote:
> >
> > > #13. Unanswered question "How are tests expecting this even passing?".
> > > Was a reason identified? IOW, how can we be sure
pg_log_error_hint("Ensure that there are non-template and
> connectable databases on the source server.");
> ```
>
Fixed the given comment. The attached patch at [1] contains the
suggested changes.
[1] -
https://www.postgresql.org/message-id/CAHv8RjKc6LMJ86b4yCmwNTn0c65mz0BGMCF-vPJSKDMOGagVGA%40mail.gmail.com
Thanks and regards,
Shubham Khanna.
ase refer to the attached top-up fix patch, which includes the
> above changes along with some cosmetic fixes in the test file
> 042_all_option.pl.
>
> [1]
> https://www.postgresql.org/message-id/CAExHW5uJHYAge99oS_iPfGWwZ_eCr2xFCNnifQkGs2GXeMQKGQ%40mail.gmail.com
>
> --
Fixed.
On Thu, Mar 20, 2025 at 3:41 PM vignesh C wrote:
>
> On Thu, 20 Mar 2025 at 10:25, Shubham Khanna
> wrote:
> >
> >
> > I have created two patches, v16-0001 and v16-0002, to address the
> > performance issue. I conducted performance testing, and here are the
>
On Sat, Mar 22, 2025 at 6:23 PM vignesh C wrote:
>
> On Fri, 21 Mar 2025 at 18:59, Shubham Khanna
> wrote:
> >
> >
> > During the recent testing, I observed that the tests were failing when
> > using wait_for_slot_catchup(). To address this, I reverted to usi
On Mon, Mar 24, 2025 at 5:41 PM Shlok Kyal wrote:
>
> On Mon, 24 Mar 2025 at 15:56, Shubham Khanna
> wrote:
> >
> > On Mon, Mar 24, 2025 at 12:29 PM Hayato Kuroda (Fujitsu)
> > wrote:
> > >
> > > Dear Shubham,
> > >
> > > > I
On Tue, Mar 25, 2025 at 4:31 PM Ashutosh Bapat
wrote:
>
> On Tue, Mar 25, 2025 at 4:28 PM Shubham Khanna
> wrote:
>
> >
> > The attached patches contain the suggested changes.
> > [1] -
> > https://www.postgresql.org/message-id/CAA4eK1KUDEO0t6
On Tue, Mar 18, 2025 at 4:25 PM vignesh C wrote:
>
> On Mon, 17 Mar 2025 at 16:51, Ashutosh Bapat
> wrote:
> >
> > On Mon, Mar 17, 2025 at 4:02 PM vignesh C wrote:
> > >
> > > On Mon, 17 Mar 2025 at 11:28, Shubham Khanna
> > > wrote:
> > &
On Tue, Mar 18, 2025 at 4:31 AM Euler Taveira wrote:
>
> On Mon, Mar 17, 2025, at 9:34 AM, Shubham Khanna wrote:
>
> I have incorporated the "--remove/-r" parameter in the attached patch,
> as it seems more intuitive and straightforward for users.
> The attached patch
On Tue, Mar 18, 2025 at 5:48 AM David G. Johnston
wrote:
>
> On Mon, Mar 17, 2025 at 4:01 PM Euler Taveira wrote:
>>
>> On Mon, Mar 17, 2025, at 9:34 AM, Shubham Khanna wrote:
>>
>> I have incorporated the "--remove/-r" parameter in the attached
s, so we must
> communicate it via the description.
>
> --
I have incorporated the "--remove/-r" parameter in the attached patch,
as it seems more intuitive and straightforward for users.
The attached patch contains the latest changes.
Thanks and regards,
Shubham Khanna,
v17-0001-Support-for-dropping-all-publications-in-pg_crea.patch
Description: Binary data
On Thu, Mar 20, 2025 at 9:37 AM vignesh C wrote:
>
> On Thu, 20 Mar 2025 at 09:06, Shubham Khanna
> wrote:
> >
> > The attached patch contains the suggested changes.
>
> Couple of minor comments:
> 1) The second and third line comments can be in same line:
> +#
On Wed, Mar 19, 2025 at 2:29 PM vignesh C wrote:
>
> On Wed, 19 Mar 2025 at 11:44, Shubham Khanna
> wrote:
> >
> >
> > I agree with you on this; switching to a single query with safe_psql()
> > will indeed simplify the test and make the verification logic cl
On Wed, Mar 19, 2025 at 3:15 PM vignesh C wrote:
>
> On Wed, 19 Mar 2025 at 14:32, Shubham Khanna
> wrote:
> >
> >
> > Changed -r to -R based on the shared analysis to avoid conflict with
> > the --retain option used in pg_upgrade and to maintain consiste
On Wed, Mar 19, 2025 at 11:17 AM vignesh C wrote:
>
> On Tue, 18 Mar 2025 at 17:34, Shubham Khanna
> wrote:
> >
> > I have added an additional test case to 040_pg_createsubscriber.pl as
> > suggested.
> >
> > The attached patch contains the suggested cha
On Wed, Mar 19, 2025 at 12:09 PM vignesh C wrote:
>
> On Wed, 19 Mar 2025 at 10:39, Shubham Khanna
> wrote:
> >
> > On Tue, Mar 18, 2025 at 5:17 PM Amit Kapila wrote:
> > >
> > > On Tue, Mar 18, 2025 at 4:01 PM Shubham Khanna
> > > wrote:
> &
as been removed
to align with the statement that they are redundant and serve no
purpose.
The attached patch at [1] contains the suggested changes.
[1] -
https://www.postgresql.org/message-id/CAHv8RjJNE1ZvWhsgL54iPsJhomhcG%2B-SGPN8AnnwdLmWt6A44A%40mail.gmail.com
Thanks and regards,
Shubham Khanna.
On Wed, Mar 19, 2025 at 12:44 PM Zhijie Hou (Fujitsu)
wrote:
>
> On Wed, Mar 19, 2025 at 2:55 PM Shubham Khanna wrote:
> >
> > I have merged the changes and prepared the latest patch. The attached
> > patch contains the suggested changes.
>
> Thanks for updating th
On Tue, Mar 18, 2025 at 5:17 PM Amit Kapila wrote:
>
> On Tue, Mar 18, 2025 at 4:01 PM Shubham Khanna
> wrote:
> >
> > On Tue, Mar 18, 2025 at 12:07 PM David G. Johnston
> > wrote:
> > >
> >
> > > It would be good if we could get this to play n
to affect or stop the dropping
> of the selected objects so consider taking a backup of them using pg_dump.
>
>
> Just add more paragraphs next to "publications:" as we add more object types.
>
Fixed.
The attached patch at [1] contains the suggested changes.
[1] -
https://www.postgresql.org/message-id/CAHv8RjJNE1ZvWhsgL54iPsJhomhcG%2B-SGPN8AnnwdLmWt6A44A%40mail.gmail.com
Thanks and regards,
Shubham Khanna.
On Tue, Mar 18, 2025 at 12:07 PM David G. Johnston
wrote:
>
> On Monday, March 17, 2025, Shubham Khanna wrote:
>>
>>
>> I have added validation for "all" to address both issues at once.
>>
>
> Usage needs to be changed to refer to object types an
On Thu, Mar 20, 2025 at 4:53 PM Ashutosh Bapat
wrote:
>
> On Thu, Mar 20, 2025 at 10:25 AM Shubham Khanna
> wrote:
>
> >
> > The attached patches contain the suggested changes.
> >
>
> I have started reviewing the patches again. Here are some review
On Thu, Mar 20, 2025 at 5:54 PM Amit Kapila wrote:
>
> On Thu, Mar 20, 2025 at 4:53 PM Ashutosh Bapat
> wrote:
> >
> > On Thu, Mar 20, 2025 at 10:25 AM Shubham Khanna
> > wrote:
> >
> >
> > +# run pg_createsubscriber with '--database'
On Wed, Mar 26, 2025 at 10:21 AM Amit Kapila wrote:
>
> On Wed, Mar 26, 2025 at 2:05 AM Euler Taveira wrote:
> >
> > On Tue, Mar 25, 2025, at 8:07 AM, Shubham Khanna wrote:
> >
> > Apologies for the oversight. I have attached the patches now. Please
> > fi
On Thu, Mar 27, 2025 at 12:16 PM Amit Kapila wrote:
>
> On Wed, Mar 26, 2025 at 4:02 PM Shubham Khanna
> wrote:
> >
>
> Let's combine 0001 and 0002.
>
Combined 0001 and 0002.
> A few minor comments:
> *
> + If database name is not specifi
On Mon, Feb 17, 2025 at 9:43 PM vignesh C wrote:
>
> On Wed, 22 Jan 2025 at 16:23, Shubham Khanna
> wrote:
> >
> > Hi,
> >
> > Following the recent comments on Patch v13-0001, the patch was not
> > applied to the HEAD. I have addressed the feedbac
>
Fixed.
The attached patch at [1] contains the suggested changes.
[1] -
https://www.postgresql.org/message-id/CAHv8RjKAdrrt3-pF6yHb5gBricB9%3DD7O47Dxe39zRxKkShdpmw%40mail.gmail.com
Thanks and regards,
Shubham Khanna.
On Tue, Feb 18, 2025 at 2:43 PM Zhijie Hou (Fujitsu)
wrote:
>
> On Thursday, February 13, 2025 11:28 AM Shubham Khanna
> wrote:
>
> Hi,
>
> > The attached patch contains the required changes.
>
> Thanks for updating the patch. I think it's a useful featur
On Fri, Feb 14, 2025 at 5:27 PM Ashutosh Bapat
wrote:
>
> Hi Shubham,
> Here are some review comments from my side
>
>
> On Thu, Feb 13, 2025 at 8:58 AM Shubham Khanna
> wrote:
> > The attached patch contains the required changes.
> >
>
> clusterdb, vacuumd
On Tue, Mar 4, 2025 at 4:31 PM Nisha Moond wrote:
>
> On Tue, Mar 4, 2025 at 10:30 AM Shubham Khanna
> wrote:
> >
> > Fixed the suggested changes. The attached patch contains the required
> > changes.
> >
>
> Hi Shubham,
>
> Thanks for the patch! I
lease do not modify randomly. Currently, made_publication is set to false
> when
> the command fails. No need to set to true here - Please follow that. Also,
> please
> do not remove comments within the if statement.
>
Fixed the suggested changes. The attached patch contains the
checked with grammarly as well.
>
Fixed.
The attached patch at [1] contains the required changes.
[1] -
https://www.postgresql.org/message-id/CAHv8Rj%2B37ja589BzqB5bz0ZYGWb5gpnP9of8SoqKc%3DDqLmvxBg%40mail.gmail.com
Thanks and regards,
Shubham Khanna.
uggesting this change?
>
> --
Updated the .sgml file accordingly. The attached patch at [1] contains
the required changes.
[1] -
https://www.postgresql.org/message-id/CAHv8Rj%2B37ja589BzqB5bz0ZYGWb5gpnP9of8SoqKc%3DDqLmvxBg%40mail.gmail.com
Thanks and regards,
Shubham Khanna.
On Wed, Mar 5, 2025 at 3:55 PM Nisha Moond wrote:
>
> On Tue, Mar 4, 2025 at 8:05 PM Shubham Khanna
> wrote:
> >
> > The attached Patch contains the suggested changes.
> >
>
> Hi Shubham,
>
> Here are few comments for 040_pg_createsubscriber.pl
>
> 1)
subscriber on node S using '--cleanup-existing-publications'.
> +# --verbose is used twice to show more information.
> # In passing, also test the --enable-two-phase option
> command_ok(
> [
> 'pg_createsubscriber',
> '--verbose', '--verbose',
> '--recovery-timeout' => $PostgreSQL::Test::Utils::timeout_default,
> - '--pgdata' => $node_s->data_dir,
> + '--pgdata' => $node_s1->data_dir,
> '--publisher-server' => $node_p->connstr($db1),
> - '--socketdir' => $node_s->host,
> - '--subscriber-port' => $node_s->port,
> + '--socketdir' => $node_s1->host,
> + '--subscriber-port' => $node_s1->port,
> '--publication' => 'pub1',
> '--publication' => 'pub2',
> '--replication-slot' => 'replslot1',
> '--replication-slot' => 'replslot2',
> '--database' => $db1,
> '--database' => $db2,
> - '--enable-two-phase'
> + '--enable-two-phase',
> + '--cleanup-existing-publications',
> ],
> 'run pg_createsubscriber on node S');
>
> Comment and Message should refer to S1, not S.
>
> ~~~
>
Fixed.
> 15.
> +# Create user-defined publications
> +$node_p->safe_psql($db3, "CREATE PUBLICATION test_pub3 FOR ALL TABLES;");
> +$node_p->safe_psql($db3, "CREATE PUBLICATION test_pub4 FOR ALL TABLES;");
>
> Probably these can be combined.
>
> ~~~
>
Fixed.
> 16.
> +# Drop the newly created publications
> +$node_p->safe_psql($db3, "DROP PUBLICATION IF EXISTS test_pub3;");
> +$node_p->safe_psql($db3, "DROP PUBLICATION IF EXISTS test_pub4;");
> +
>
> Probably these can be combined.
>
Fixed.
The attached patch at [1] contains the required changes.
[1] -
https://www.postgresql.org/message-id/CAHv8RjLCZyzFMGR8SBdvSocRZGAAr_eRd4ht48kXCp9oEaKQeQ%40mail.gmail.com
Thanks and regards,
Shubham Khanna.
tch
it directly from the instance for comparison, making the loop more
meaningful.
Additionally, as suggested by Ashutosh in [1], I have split the
040_pg_createsubscriber.pl file into three parts to improve clarity.
The attached patch includes all the suggested changes.
[1] -
https://www.postgresql.org/message-id/CAExHW5uJHYAge99oS_iPfGWwZ_eCr2xFCNnifQkGs2GXeMQKGQ%40mail.gmail.com
Thanks and regards,
Shubham Khanna.
On Thu, Mar 6, 2025 at 9:18 AM Ajin Cherian wrote:
>
> On Fri, Feb 28, 2025 at 11:34 PM Shubham Khanna
> wrote:
> >
> >
> > The attached Patch contains the suggested changes.
> >
> > Thanks and regards,
> > Shubham Khanna.
>
> S
On Mon, Mar 10, 2025 at 3:09 PM Nisha Moond wrote:
>
> On Mon, Mar 10, 2025 at 12:11 PM Shubham Khanna
> wrote:
> >
> > On Thu, Mar 6, 2025 at 9:27 AM Peter Smith wrote:
> > >
> > > 6.
> > > - dbinfo->pubname, dbinfo->dbname, PQresultErrorM
On Mon, Mar 10, 2025 at 5:02 PM Nisha Moond wrote:
>
> On Mon, Mar 10, 2025 at 4:31 PM Shubham Khanna
> wrote:
> >
> > > 2) This part of code has another bug:
> > > "dbinfos.dbinfo->made_publication = false;" incorrectly modifies
> > > d
;
> foreach my $dbname (@user_dbs)
> {
> $result =
> $node_s2->safe_psql('postgres',
> "SELECT count(*) FROM pg_subscription, pg_database
> WHERE subdbid = pg_database.oid and datname = '$dbname';");
>
&g
below.
> > >
> > > 01.
> > > ```
> > > + -a
> > > + --all-dbs
> > > ```
> > >
> > > Peter's comment [2] does not say that option name should be changed.
> > > The scope of his comment is only in the .c file.
> >
> > Yes, that's correct. My v9 suggestion to change 'all' to 'all_dbs' was
> > referring only to the field name of struct CreateSubscriberOptions,
> > nothing else. Not the usage help, not the error messages, not the
> > docs, not the tests, not the commit message.
>
> +1. We don't want yet another option to specify all databases. :)
>
Fixed.
The attached patch at [1] contains the suggested changes.
[1] -
https://www.postgresql.org/message-id/CAHv8RjJ9BsCg%2Bpur307b1JnfQebnmxFZLw4LdcGX7f-%3D6OK1vw%40mail.gmail.com
Thanks and regards,
Shubham Khanna.
t; > ```
> > + -a
> > + --all-dbs
> > ```
> >
> > Peter's comment [2] does not say that option name should be changed.
> > The scope of his comment is only in the .c file.
>
> Yes, that's correct. My v9 suggestion to change 'all' to 'all_dbs' was
> referring only to the field name of struct CreateSubscriberOptions,
> nothing else. Not the usage help, not the error messages, not the
> docs, not the tests, not the commit message.
>
Fixed.
The attached patch at [1] contains the suggested changes.
[1] -
https://www.postgresql.org/message-id/CAHv8RjJ9BsCg%2Bpur307b1JnfQebnmxFZLw4LdcGX7f-%3D6OK1vw%40mail.gmail.com
Thanks and regards,
Shubham Khanna.
ovacuum = off');
> +$node_a->start;
> ```
>
> I don't think new primary is not needed. Can't you reuse node_p?
>
> [1]:
> https://www.postgresql.org/message-id/OSCPR01MB14966D845AC77FC46ECEECCE4F5C72%40OSCPR01MB14966.jpnprd01.prod.outlook.com
>
The v10 patch at [1] required rebasing on the latest HEAD. I have
prepared a rebased version and updated the patch accordingly.
[1] -
https://www.postgresql.org/message-id/CAHv8RjJtzXc4BWoKTyTB-9WEcAJ4N5qsD6YuXK3EAmsT6237yw%40mail.gmail.com
The attached patch includes the suggested changes.
Thanks and regards,
Shubham Khanna.
v11-0001-Support-for-dropping-all-publications-in-pg_crea.patch
Description: Binary data
ocess.
As of the latest patch attached at [1], this issue remains unresolved.
I will proceed with splitting the tests once all other issues are
addressed.
[1] -
https://www.postgresql.org/message-id/CAHv8RjJ9BsCg%2Bpur307b1JnfQebnmxFZLw4LdcGX7f-%3D6OK1vw%40mail.gmail.com
Thanks and regards,
Shubham Khanna.
On Fri, Feb 14, 2025 at 4:57 PM Shlok Kyal wrote:
>
> On Fri, 14 Feb 2025 at 10:50, Shubham Khanna
> wrote:
> >
> > On Thu, Feb 13, 2025 at 5:36 PM Shlok Kyal wrote:
> > >
> > > On Thu, 13 Feb 2025 at 15:20, Shubham Khanna
> > > wrote:
> >
On Mon, Feb 17, 2025 at 4:06 PM Amit Kapila wrote:
>
> On Thu, Jan 23, 2025 at 6:42 PM Shubham Khanna
> wrote:
> >
> > Fixed the given comments. The attached patch contains the suggested changes.
> >
>
> I am fine with giving a WARNING for max_slot_wal_keep_s
> +
> +$result = $node_s1->safe_psql($db1, "SELECT * FROM tbl1 ORDER BY 1");
> +is( $result, qq(first row
> +replication test
> +second row), "replication successful in $db1");
> +
> +$node_s1->stop;
> # Run pg_createsubscriber on node S. --verbose is used twice
> # to show more information.
> command_ok(
> ```
>
> I'm not sure the part is not needed. This have already been checked in other
> parts, right?
>
Fixed.
The attached patch contains the suggested changes.
Thanks and regards,
Shubham Khanna.
v10-0001-Enhance-pg_createsubscriber-to-fetch-and-append-.patch
Description: Binary data
will be 3, 3, 3. Why?
>
> I guess you intended to verify that relevant subscriptions were
> created for each of the target databases. But this code is not doing
> that.
>
> ~~~
Fixed.
>
> 7.
> +# Verify replication is working
> +$node_p->safe_psql($db1, "INSER
EATE PUBLICATION test_pub FOR ALL TABLES;");
> +$node_p->safe_psql($db1, "CREATE PUBLICATION test_pub2 FOR ALL TABLES;");
> +
>
> /it's/their/
>
> ~~~
>
Fixed.
> 8.
> Maybe also do a CREATE PUBLICATION at node_s, prior to the
> pg_createsubvscript, so then you can verify that the user-created one
> is unaffected by the cleanup of all the others.
>
> ==
Since $node_s is a streaming standby, it does not allow object
creation. As a result, publications cannot be created on $node_s.
> [1]
> https://www.postgresql.org/message-id/CAExHW5t4ew7ZrgcDdTv7YmuG7LVQT1ZaEny_EvtngHtEBNyjcQ%40mail.gmail.com
>
The attached patch contains the suggested changes.
Thanks and regards,
Shubham Khanna.
v8-0001-Support-for-dropping-all-publications-in-pg_creat.patch
Description: Binary data
On Fri, Feb 21, 2025 at 8:31 AM Peter Smith wrote:
>
> On Thu, Feb 20, 2025 at 5:44 PM Shubham Khanna
> wrote:
> >
> > On Mon, Feb 17, 2025 at 9:49 AM Peter Smith wrote:
> > >
> ...
> > > ==
> > > 1 GENERAL. Option Name?
> > >
&
g_log_info("dropping publication \"%s\" in database \"%s\"",
> dbinfo->pubname, dbinfo->dbname);
> pg_log_debug("command is: %s", str->data);
> ```
>
Fixed.
> 08.
> I feel we must update made_publication.
>
Fixed.
The attached patch at [1] contains the suggested changes.
[1] -
https://www.postgresql.org/message-id/CAHv8Rj%2BzmkwunNubo%2B_Gp26S_qXbD%3Dp%2BrMfEnPnjiEE1A6GTXQ%40mail.gmail.com
Thanks and regards,
Shubham Khanna.
On Mon, Feb 24, 2025 at 10:57 AM Amit Kapila wrote:
>
> On Tue, Feb 18, 2025 at 12:16 AM Shubham Khanna
> wrote:
> >
>
> -static void check_publisher(const struct LogicalRepInfo *dbinfo);
> -static char *setup_publisher(struct LogicalRepInfo *dbinfo);
> +static void c
subscriber.pl
> ```
> +# Set up node A as primary
> +my $node_a = PostgreSQL::Test::Cluster->new('node_a');
> +my $aconnstr = $node_a->connstr;
> +$node_a->init(allows_streaming => 'logical');
> +$node_a->append_conf('postgresql.conf', 'autovacuum = off');
> +$node_a->start;
> ```
>
> I don't think new primary is not needed. Can't you reuse node_p?
>
Fixed.
The attached patch contains the suggested changes.
Thanks and regards,
Shubham Khanna.
v10-0001-Support-for-dropping-all-publications-in-pg_crea.patch
Description: Binary data
) similar to
> pg_upgrade. So, using -R for this option sounds reasonable to me.
>
> --
Fixed the mentioned issue by changing -r to -R to avoid conflict with
the --retain option used in pg_upgrade. The attached patch at [1]
contains the required change.
[1] -
https://www.postgresql.org/message-id/CAHv8RjKbYaCRrzJzmU6gU0jt85xJftzVV-Hu9rXrkb1yFzYTtA%40mail.gmail.com
Thanks and regards,
Shubham Khanna.
/60b45b8a-3047-4a21-ba2a-ddb15daa638f%40eisentraut.org
> [2]: https://www.postgresql.org/docs/devel/pgupgrade.html
>
Changed -r to -R based on the shared analysis to avoid conflict with
the --retain option used in pg_upgrade and to maintain consistency
across tools.
The attached patch contains the
wait_for_subscription_sync
> was used there, but I feel this may not be correct). How about unifing them
> like
> attached?
>
Fixed.
> 08.
> ```
> +# Verify logical replication works for all databases
> +# Insert rows on node P
> +$node_p->safe_psql('pos
On Mon, Mar 24, 2025 at 6:08 PM Ashutosh Bapat
wrote:
>
> On Fri, Mar 21, 2025 at 6:59 PM Shubham Khanna
> wrote:
> >
> > On Thu, Mar 20, 2025 at 4:53 PM Ashutosh Bapat
> > wrote:
> > >
> > > On Thu, Mar 20, 2025 at 10:25 AM Shubham Khanna
>
se particular checks
were retained in the main patch because they ensure the fundamental
validations for incompatible options (--database and --publication
with --all) are covered early on.
> 03.
> ```
> +# Verify that the required logical replication objects are created. The
> +# expected count 3 refers to postgres, $db1 and $db2 databases.
> ```
>
> Hmm, but since we did a dry-run, any objects are not created, right?
>
Fixed.
The attached patches contain the suggested changes.
[1] -
https://www.postgresql.org/message-id/CAA4eK1KUDEO0t6i16_CcEpg33sgcgEddHcdVC_q8j4tVUb5FWw%40mail.gmail.com
Thanks and regards,
Shubham Khanna.
101 - 197 of 197 matches
Mail list logo
