.828s | 6.422s | 31.704s
Logical rep(10 w)| 0.646s | 3.843s | 18.425s
pg_subscriber | 3.977s | 9.988s | 12.665s
Here, 'w' stands for 'workers'. I have included the tests to see the
test result variations with different values for
'max_sync_workers_per_subs
ress/expected/dict.out
patching file src/test/regress/expected/oidjoins.out
patching file src/test/regress/expected/opr_sanity.out
patching file src/test/regress/parallel_schedule
Hunk #1 FAILED at 111.
1 out of 1 hunk FAILED -- saving rejects to file
src/test/regress/parallel_schedule.rej
patching file src/test/regress/sql/dict.sql
Please send the Re-base version of the patch.
Thanks and Regards,
Shubham Khanna.
view.sql
patching file src/test/regress/sql/privileges.sql
patching file src/test/regress/sql/vacuum.sql
Please send the Re-base version of the patch.
Thanks and Regards,
Shubham Khanna.
by.c.rej
Please send the Re-base version of the patch.
Thanks and Regards,
Shubham Khanna.
On Tue, Jan 16, 2024 at 11:58 AM Shubham Khanna
wrote:
>
> On Thu, Dec 21, 2023 at 11:47 AM Amit Kapila wrote:
> >
> > On Wed, Dec 6, 2023 at 12:53 PM Euler Taveira wrote:
> > >
> > > On Thu, Nov 9, 2023, at 8:12 PM, Michael Paquier wrote:
> > >
&
ntation fault.
0x7e5b in cleanup_objects_atexit () at pg_subscriber.c:173
173 if (perdb->made_subscription)
(gdb) p perdb
$1 = (LogicalRepPerdbInfo *) 0x0
Thanks and Regards,
Shubham Khanna.
nd loop;
> end;
> $$
> language plpgsql;
> select pg_create_logical_replication_slot('s', 'test_decoding');
> select testfn(5);
> set logical_decoding_work_mem to '4MB';
> select count(*) from pg_logical_slot_peek_changes('s', null, nul
-
------------
--
pg_createsubscriber_5_1895366 | user=shubham
passfile='/home/shubham/.pgpass' channel_binding=prefer ho
st=127.0.0.1 port=5432 sslmode=prefer sslcompression=0
sslcertmode=allow sslsni=1 ssl_min_protocol_versi
on=TLSv1.2 gssencmode=disable krbsrvname=postgres gssdelegation=0
target_session_attrs=any load_balance_
hosts=disable dbname=postgres
(1 row)
Here, we can see that channel_binding, sslmode, sslcertmode, sslsni,
gssencmode, krbsrvname, etc are getting included. This does not look
intentional, we should keep the subscription connection same as in
v15-0001.
Thanks and Regards,
Shubham Khanna.
Thanks and Regards,
Shubham Khanna.
x27;s say it has a backlog that will
> take some time to apply. Unless, you have a local agent, you have no data
> about
> this server until pg_createsubscriber terminates. Even the local agent might
> not connect to the server unless you use the current port.
I tried verifying few scenarios by using 5 databases and came across
the following errors:
./pg_createsubscriber -D ../new_standby -P "host=localhost port=5432
dbname=postgres" -S "host=localhost port=9000 dbname=postgres" -d db1
-d db2 -d db3 -d db4 -d db5
pg_createsubscriber: error: publisher requires 6 wal sender
processes, but only 5 remain
pg_createsubscriber: hint: Consider increasing max_wal_senders to at least 7.
It is successful only with 7 wal senders, so we can change error
messages accordingly.
pg_createsubscriber: error: publisher requires 6 replication slots,
but only 5 remain
pg_createsubscriber: hint: Consider increasing max_replication_slots
to at least 7.
It is successful only with 7 replication slots, so we can change error
messages accordingly.
Thanks and Regards,
Shubham Khanna,
possibly lost: 44 bytes in 3 blocks
==651272==still reachable: 3,066 bytes in 22 blocks
==651272== suppressed: 0 bytes in 0 blocks
==651272==
==651272== For lists of detected and suppressed errors, rerun with: -s
==651272== ERROR SUMMARY: 17 errors from 17 contexts (suppressed:
| 100 | 10 | 2 | 1 | 5000
--||--|-|---|---|--
Head|106281.236|10669.992|1073.815|214.287|107.62 |54.947
Patch|103108.673|10603.139|1064.98 |210.229|106.321|54.218
%imp| 02.985| 0.626 |0.822 |01.893 |01.207 |01.326
The execution time is in milliseconds. The column header indicates the
number of inserts in the transaction. I can notice with the test
result that the issue has been resolved with the new patch.
Thanks and Regards,
Shubham Khanna.
patch.
Thanks and Regards,
Shubham Khanna.
v1-0001-Improve-documentation-of-upgrade-for-streaming-re (2).patch
Description: Binary data
ation system like
node1->node2->node3 and ran pg_createsubscriber for node2. After
running the script, I started the node2 again and found
pg_createsubscriber command was successful after which the physical
replication between node2 and node3 has been broken. I feel
pg_createsubscriber should check
supported or not?
Scenario 2) Create cascade replication like node1->node2->node3 using
replication slots (attached cascade_3node_setup_with_slots has the
script for this):
Here, slot name was used as slot1 for node1 to node2 and slot2 for
node2 to node3. Then I ran pg_createsubscriber by speci
iber will
> be available. The client log can be redirected to a file for future
> inspection.
>
> Comments?
I applied the v25 patch and did RUN the 'pg_createsubscriber' command.
I was unable to execute it and experienced the following error:
$ ./pg_createsubscriber -D node2/ -P "host=localhost port=5432
dbname=postgres" -d postgres -d db1 -p 9000 -r
./pg_createsubscriber: invalid option -- 'p'
pg_createsubscriber: hint: Try "pg_createsubscriber --help" for more
information.
Here, the --p is not accepting the desired port number. Thoughts?
Thanks and Regards,
Shubham Khanna.
ace.
const char *export_query_v10 =
.git/rebase-apply/patch:826: trailing whitespace.
.git/rebase-apply/patch:1142: trailing whitespace.
result = PQexec(conn,
warning: squelched 4 whitespace errors
warning: 9 lines add whitespace errors.
Applying: Add pg_export_stats, pg_import_stats.
Thanks and Regards,
Shubham Khanna.
-
> + regression | regress_zeropriv_owner | SQL_ASCII | libc| C
> | C || | (none)
> +(1 row)
>
> This test is also not stable, since it depends on the locale definition
> of the regression test database. If you use "make installcheck", that could
> be a different locale.
>
> I think that these tests are not absolutely necessary, and the other tests
> are sufficient. Consequently, I took the simple road of removing them.
>
> I also tried to improve the commit message.
>
> Patch attached.
I tested the Patch for the modified changes and it is working fine.
Thanks and regards,
Shubham Khanna.
;DELETE", "TRUNCATE",
+ "REFERENCES", "TRIGGER", "CREATE", "EXECUTE", "USAGE",
+ "MAINTAIN", "ALL", "GRANT OPTION FOR");
I could not find "alter default privileges revoke maintain", should
this be removed?
Regards,
Shubham Khanna
gt; otherwise).
>
> Attached is an updated patch, incorporating those comments.
>
> Barring any further comments, I think this is ready for commit.
I reviewed the given Patch and it is working fine.
Thanks and Regards,
Shubham Khanna.
On Mon, Nov 27, 2023 at 9:58 PM vignesh C wrote:
>
> On Fri, 24 Nov 2023 at 18:37, Shubham Khanna
> wrote:
> >
> > n Fri, Nov 24, 2023 at 6:33 PM vignesh C wrote:
> > >
> > > Hi,
> > >
> > > Improved tab completion for "ALTER DEFAUL
can optionally specify an operator class and/or
>> ordering options; these are described fully under CREATE INDEX."
>>
>> You may need to update this sentence to reflect that exclude_element
>> can also optionally specify collation.
I have reviewed the changes and it looks fine.
Thanks and Regards,
Shubham Khanna.
On Thu, Nov 9, 2023 at 10:00 PM shihao zhong wrote:
>
> That looks good to me!
>
> The new status of this patch is: Ready for Committer
I have reviewed the patch and it is working fine.
Thanks and Regards,
Shubham Khanna.
'is' should be removed.
-v7-0015-Trigger-authors-need-not-worry-about-parallelism.patch
Plus, this patch adds an index entry so the new verbage is easy to find
for those who do investigate.
Here 'verbage' should be replaced with 'verbiage'.
-v7-0016-Predicate-locks-are-held-per-cluster-not-per-data.patch
This is a significant corner case and so should be documented. It is
also somewhat suprising since the databases within a cluster are
otherwise isolated, at least from the user's perspective.
Here 'suprising' should be replaced with 'surprising'.
Predicate locks are held per-cluster, not per database.
+ This means that serializeable transactions in one database can have
+ effects in another.
+ Long running serializeable transactions, as might occur accidentally
+ when
+ pagination
+ halts psql output, can have
+ significant inter-database effects.
+ These include exhausting available predicate locks and
+ cluster-wide WAL checkpoint delay.
+ When making use of serializeable transactions consider having
+ separate clusters for production and non-production use.
Here 'serializeable' should be replaced with 'serializable'.
Thanks and Regards,
Shubham Khanna.
is a member of.
> + The current session user must have the SET for the
> + specified role_name, either
> + directly or indirectly via a chain of memberships with the
> + SET option.
> (If the session user is a superuser, any role can be selected.)
>
>
> --
&g
on build was successful and there was no Spell-check issue also. I
> did not find any issues. The patch looks >good to me.
>
>Thanks and Regards,
>Shubham Khanna.
#x27;m
> tempted to be less ambitious in a first step and just move that in the
> compatibility section.
>
> >> +
> >> +Special Considerations for 64-Bit Windows
> >> +
> >> + PostgreSQL will only build for the x64 architecture on 64-bit
> >> Windows.
> >> +
> >> +
> >> + Mixing 32- and 64-bit versions in the same build tree is not
> >> supported.
> >> + The build system will automatically detect if it's running in a 32-
> >> or
> >> + 64-bit environment, and build PostgreSQL accordingly. For this
> >> reason, it
> >> + is important to start the correct command prompt before building.
> >> +
> >
> >> Isn't this directly contradicting the earlier
> >> +The native Windows port requires a 32 or 64-bit version of Windows
> >> +2000 or later. Earlier operating systems do
> > ?
>
> How it that? Mixing 32b and 64b libraries is not related to the
> minimal runtime version. This is just telling to not mix both.
> --
>
Patch is not applying. Please share the Rebased Version. Please find the error:
D:\Project\Postgres>git am D:\Project\Patch\v5-0001-Remove-MSVC-scripts.patch
error: patch failed: doc/src/sgml/filelist.sgml:38
error: doc/src/sgml/filelist.sgml: patch does not apply
error: patch failed: src/tools/msvc/Mkvcbuild.pm:1
error: src/tools/msvc/Mkvcbuild.pm: patch does not apply
error: patch failed: src/tools/msvc/Solution.pm:1
error: src/tools/msvc/Solution.pm: patch does not apply
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Applying: Remove MSVC scripts
Patch failed at 0001 Remove MSVC scripts
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Thanks and Regards,
Shubham Khanna.
ywhere. This matches what's in catalogs,
and requires less explaining than the rules for
`pg_get_serial_sequence`.
Here 'sholud' have been 'should'.
Thanks and Regards,
Shubham Khanna.
ply
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Applying: postgres_fdw: add postgres_fdw_verify_connection variants
Patch failed at 0001 postgres_fdw: add postgres_fdw_verify_connection variants
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Please rebase and post an updated version of the Patch.
Thanks and Regards,
Shubham Khanna.
On Mon, Dec 11, 2023 at 6:28 AM Thomas Munro wrote:
>
> Hi,
>
> This is not exhaustive, I just noticed in passing that we don't need these.
I was able to compile the changes with "--with-llvm" successfully, and
the changes look good to me.
Thanks and Regards,
Shubham Khanna.
the single quote.
>
> OTOH, it works for CREATE SUBSCRIPTION sub CONNECTION xx \t, should we fix it?
>
I reviewed the Patch and it looks fine to me.
Thanks and Regards,
Shubham Khanna.
value(record, name, value,
> > -
> > PGC_S_FILE, ERROR,
> > +
> > PGC_S_TEST, ERROR,
> >
> > &newval, &newextra))
> > ereport(ERROR,
> >
> > (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
>
> This is rebased over my own patch to enable checks for
> REGRESSION_TEST_NAME_RESTRICTIONS.
>
I was reviewing the Patch and came across a minor issue that the Patch
does not apply on the current Head. Please provide the updated version
of the patch.
Thanks and Regards,
Shubham Khanna.
ed to consider
> incremental sort in this case, because for a partial path of a grouped
> or partially grouped relation, it is either unordered (HashAggregate or
> Append), or it has been ordered by the group_pathkeys (GroupAggregate).
> It seems there is no case that we'd have a partial path that is
> partially sorted.
>
I reviewed the Patch and it looks fine to me.
Thanks and Regards,
Shubham Khanna.
On Thu, Dec 28, 2023 at 4:01 PM Shubham Khanna
wrote:
>
> On Thu, Dec 28, 2023 at 4:00 PM Richard Guo wrote:
> >
> >
> > On Wed, Mar 29, 2023 at 4:00 AM David Rowley wrote:
> >>
> >> If you write some tests for this code, it will be useful to prove that
On Sat, Dec 23, 2023 at 8:53 AM vignesh C wrote:
>
> Hi,
>
> I didn't see anyone volunteering for the January Commitfest, so I'll
> volunteer to be CF manager for January 2024 Commitfest.
I can assist with the January 2024 Commitfest.
Thanks and Regards,
Shubham Khanna.
on
of the patch. Also, I found one typo:
0008-ecpg-fix-typo-in-get_dtype-return-value-for-ECPGd_co.patch
All the other enum values return a string mathing the enum label, but
this has had a trailing r since the function was added in commit
339a5bbfb17ecd171ebe076c5bf016c4e66e2c0a
Here 'mathing' should be 'matching'.
Thanks and Regards,
Shubham Khanna.
RED_RELATION BKI_ROW
> * slots) in the upstream database are enabled
> * to be synchronized to the standbys. */
>
> + bool subincludegeneratedcolumn; /* True if generated columns must be
> published */
> +
> #ifdef CATALOG_VARLEN /* variable-length fields start here */
> /* Connection string to the publisher */
> text subconninfo BKI_FORCE_NOT_NULL;
> @@ -157,6 +159,7 @@ typedef struct Subscription
> List*publications; /* List of publication names to subscribe to */
> char*origin; /* Only publish data originating from the
> * specified origin */
> + bool includegeneratedcolumn; /* publish generated column data */
> } Subscription;
Fixed.
The attached Patch contains the suggested changes.
Thanks and Regards,
Shubham Khanna.
v6-0001-Enable-support-for-include_generated_columns-opti.patch
Description: Binary data
rigin; /* Only publish data originating from the
> * specified origin */
> + bool include_generated_column; /* publish generated columns */
> } logical;
> } proto;
> } WalRcvStreamOptions;
>
> ~
>
> This new field name should be plural.
Fixed.
> ==
> src/test/subscription/t/011_generated.pl
>
> 16.
> +my ($cmdret, $stdout, $stderr) = $node_subscriber->psql('postgres', qq(
> + CREATE SUBSCRIPTION sub2 CONNECTION '$publisher_connstr' PUBLICATION
> pub2 WITH (include_generated_column = true)
> +));
> +ok( $stderr =~
> + qr/copy_data = true and include_generated_column = true are
> mutually exclusive options/,
> + 'cannot use both include_generated_column and copy_data as true');
>
> Isn't this mutual exclusiveness of options something that could have
> been tested in the regress test suite instead of TAP tests? e.g. AFAIK
> you won't require a connection to test this case.
> 17. Missing test?
>
> IIUC there is a missing test scenario. You can add another subscriber
> table TAB3 which *already* has generated cols (e.g. generating
> different values to the publisher) so then you can verify they are NOT
> overwritten, even when the 'include_generated_cols' is true.
>
> ==
Moved this test case to the Regression test.
Patch v6-0001 contains all the changes required. See [1] for the changes added.
[1]
https://www.postgresql.org/message-id/CAHv8RjJn6EiyAitJbbvkvVV2d45fV3Wjr2VmWFugm3RsbaU%2BRg%40mail.gmail.com
Thanks and Regards,
Shubham Khanna.
names.
>
> +$result = $node_subscriber->safe_psql('postgres', "SELECT * FROM tab2");
> +is( $result, qq(4|8
> +5|10), 'generated columns replicated to non-generated column on subscriber');
> +
> +$node_publisher->safe_psql('postgres', "INSERT INTO tab3 VALUES (4), (5)");
> +
> +$node_publisher->wait_for_catchup('sub3');
> +
> +$result = $node_subscriber->safe_psql('postgres', "SELECT * FROM tab3");
> +is( $result, qq(4|24
> +5|25), 'generated columns replicated to non-generated column on subscriber');
All the comments are handled.
The attached Patch contains all the suggested changes.
Thanks and Regards,
Shubham Khanna.
v8-0001-Currently-generated-column-values-are-not-replica.patch
Description: Binary data
= '1' the generated column 'b'
> values will not be replicated
> SELECT data FROM pg_logical_slot_peek_changes('regression_slot', NULL,
> NULL, 'include-xids', '0', 'skip-empty-xacts', '1',
> 'include-generated-columns', '1');
> -- When 'include-generated-columns' = '0' the generated column 'b'
> values will be replicated
> SELECT data FROM pg_logical_slot_peek_changes('regression_slot', NULL,
> NULL, 'include-xids', '0', 'skip-empty-xacts', '1',
> 'include-generated-columns', '0');
>
> 9) In commit message the option used is wrong
> include_generated_columns should actually be
> include-generated-columns:
> Usage from test_decoding plugin:
> SELECT data FROM pg_logical_slot_get_changes('slot2', NULL, NULL,
> 'include-xids', '0', 'skip-empty-xacts', '1',
> 'include_generated_columns','1');
All the comments are handled.
Patch v8-0001 contains all the changes required. See [1] for the changes added.
[1]
https://www.postgresql.org/message-id/CAHv8Rj%2BAi0CgtXiAga82bWpWB8fVcOWycNyJ_jqXm788v3R8rQ%40mail.gmail.com
Thanks and Regards,
Shubham Khanna.
fe_psql('postgres', "INSERT INTO tab2 VALUES (4), (5)");
> +
> +$node_publisher->wait_for_catchup('sub2');
> +
> +$result = $node_subscriber->safe_psql('postgres', "SELECT * FROM tab2");
> +is( $result, qq(4|8
> +5|10), 'generated columns replicated to non-generated column on subscriber');
> +
> +$node_publisher->safe_psql('postgres', "INSERT INTO tab3 VALUES (4), (5)");
> +
> +$node_publisher->wait_for_catchup('sub3');
> +
> +$result = $node_subscriber->safe_psql('postgres', "SELECT * FROM tab3");
> +is( $result, qq(4|24
> +5|25), 'generated columns replicated to non-generated column on subscriber');
> +
>
> Here also I think there should be explicit comments about what these
> cases are testing, what results you are expecting, and why. The
> comments will look something like the message parameter of those
> safe_psql(...)
>
> e.g.
> # confirm generated columns ARE replicated when the subscriber-side
> column is not generated
>
> e.g.
> # confirm generated columns are NOT replicated when the
> subscriber-side column is also generated
>
> ==
>
> 99.
> Please also see my nitpicks attachment patch for various other
> cosmetic and docs problems, and apply theseif you agree:
> - documentation wording/rendering
> - wrong comments
> - spacing
> - etc.
All the comments are handled.
Patch v8-0001 contains all the changes required. See [1] for the changes added.
[1]
https://www.postgresql.org/message-id/CAHv8Rj%2BAi0CgtXiAga82bWpWB8fVcOWycNyJ_jqXm788v3R8rQ%40mail.gmail.com
Thanks and Regards,
Shubham Khanna.
*
> returning rows in some unexpected order.
>
> ~
>
> 6b.
> IMO there should be more comments here to explain how you can tell the
> column was NOT replicated. E.g. it is because the result value of 'b'
> is the subscriber-side computed value (which you made deliberately
> different to the publisher-side computed value).
>
> ==
>
> 99.
> Please also refer to the attached nitpicks top-up patch for minor
> cosmetic stuff.
All the comments are handled.
The attached Patch contains all the suggested changes.
Thanks and Regards,
Shubham Khanna.
v9-0001-Currently-generated-column-values-are-not-replica.patch
Description: Binary data
rises.
Both the comments are handled.
Patch v9-0001 contains all the changes required. See [1] for the changes added.
[1]
https://www.postgresql.org/message-id/CAHv8Rj%2B6kwOGmn5MsRaTmciJDxtvNsyszMoPXV62OGPGzkxrCg%40mail.gmail.com
Thanks and Regards,
Shubham Khanna.
.com
> [2]
> https://www.postgresql.org/message-id/CAHut%2BPvsRWq9t2tEErt5ZWZCVpNFVZjfZ_owqfdjOhh4yXb_3Q%40mail.gmail.com
> [3]
> https://www.postgresql.org/message-id/CAHut%2BPsHsT3V1wQ5uoH9ynbmWn4ZQqOe34X%2Bg37LSi7sgE_i2g%40mail.gmail.com
All the comments are handled.
I have attached the updated patch v11 here in [1]. See [1] for the
changes added.
[1]
https://www.postgresql.org/message-id/CAHv8RjJpS_XDkR6OrsmMZtCBZNxeYoCdENhC0%3Dbe0rLmNvhiQw%40mail.gmail.com
Thanks and Regards,
Shubham Khanna.
d this issue in the latest Patches.
Please refer to the updated v15 Patches here in [1]. See [1] for the
changes added.
[1]
https://www.postgresql.org/message-id/CAHv8Rj%2B%3Dhn--ALJQvzzu7meX3LuO3tJKppDS7eO1BGvNFYBAbg%40mail.gmail.com
Thanks and Regards,
Shubham Khanna.
l still be good
> to go.
As suggested, I have created a separate patch for the Bitmapset(BMS)
idea of patch 0001 that changes the 'columns' meaning to include
generated cols also where necessary.
Please refer to the updated v17 Patches here in [1]. See [1] for the
changes added.
[1]
https://www.postgresql.org/message-id/CAHv8RjJ0gAUd62PvBRXCPYy2oTNZWEY-Qe8cBNzQaJPVMZCeGA%40mail.gmail.com
Thanks and Regards,
Shubham Khanna.
for the generated column is not possible as copy command does not
support getting data for the generated column. If this feature is
required we can remove this limitation from the copy command and then
add it as a subscription option later.
Thoughts?
Thanks and Regards,
Shubham Khanna.
v1-0001-Support-capturing-generated-column-data-using-pgo.patch
Description: Binary data
;
>
> c. pg_decode_change()
>
> ```
> -true);
> + true, data->include_generated_columns );
> ```
>
> Please remove the blank.
Fixed.
The attached v3 Patch has the changes for the same.
Thanks and Regards,
Shubham Khanna.
v3-0001-Support-generated-column-capturing-generated-colu.patch
Description: Binary data
put_parameters
>
> bool origin_option_given = false;
> + bool generate_column_option_given = false;
>
> data->binary = false;
> data->streaming = LOGICALREP_STREAM_OFF;
> data->messages = false;
> data->two_phase = false;
> + data->publish_generated_column = false;
>
> I think the 1st var should be 'include_generated_columns_option_given'
> for consistency with the name of the actual option that was given.
Updated the name to 'include_generated_columns_option_given'
> ==
> src/include/replication/logicalproto.h
>
> 7.
> (Same as a previous review comment)
>
> For all the API changes the new parameter name should be plural.
>
> /publish_generated_column/publish_generated_columns/
Updated the name to 'include_generated_columns'
> ==
> src/include/replication/pgoutput.h
>
> 8.
> bool publish_no_origin;
> + bool publish_generated_column;
> } PGOutputData;
>
> /publish_generated_column/publish_generated_columns/
Updated the name to 'include_generated_columns'
The attached Patch contains the suggested changes.
Thanks and Regards,
Shubham Khanna.
v4-0001-Enable-support-for-include_generated_columns-opti.patch
Description: Binary data
list(att->attnum, columns))
> continue;
>
> + if (att->attgenerated && !publish_generated_column)
> + continue;
>
> ditto #6
>
> ==
> src/include/catalog/pg_subscription.h
Fixed.
> 10. CATALOG
>
> + bool subgeneratedcolumn; /* True if generated colums must be published */
>
> /colums/columns/
>
> ==
> src/test/regress/sql/publication.sql
Fixed.
> 11.
> --- error: generated column "d" can't be in list
> +-- ok
>
>
> Maybe change "ok" to say like "ok: generated cols can be in the list too"
Fixed.
> 12.
> GENERAL - Missing CREATE SUBSCRIPTION test?
> GENERAL - Missing ALTER SUBSCRIPTION test?
>
> How come this patch adds a new CREATE SUBSCRIPTION option but does not
> seem to include any test case for that option in either the CREATE
> SUBSCRIPTION or ALTER SUBSCRIPTION regression tests?
Added the test cases for the same.
Patch v4-0001 contains all the changes required. See [1] for the changes added.
[1]
https://www.postgresql.org/message-id/CAHv8RjJcOsk%3Dy%2BvJ3y%2BvXhzR9ZUzUEURvS_90hQW3MNfJ5di7A%40mail.gmail.com
Thanks and Regards,
Shubham Khanna.
continue;
> +
> + if (att->attgenerated && !publish_generated_column)
> continue;
> ```
>
> I think changes in v2 was reverted or wrongly merged.
Fixed.
> 08. test code
>
> Can you add tests that generated columns are replicated by the logical
> replication?
Added the test cases.
Patch v4-0001 contains all the changes required. See [1] for the changes added.
[1]
https://www.postgresql.org/message-id/CAHv8RjJcOsk%3Dy%2BvJ3y%2BvXhzR9ZUzUEURvS_90hQW3MNfJ5di7A%40mail.gmail.com
Thanks and Regards,
Shubham Khanna.
On Thu, May 23, 2024 at 5:56 PM vignesh C wrote:
>
> On Thu, 23 May 2024 at 09:19, Shubham Khanna
> wrote:
> >
> > > Dear Shubham,
> > >
> > > Thanks for creating a patch! Here are high-level comments.
> >
> > > 1.
> > > Please
. pgoutput_change
> -
> + bool publish_generated_column = data->publish_generated_column;
> +
>
> I'm not convinced this saves any code, and anyway, it is not
> consistent with other fields in this function that are not extracted
> to another variable (e.g. data->bi
On Fri, Aug 16, 2024 at 2:47 PM vignesh C wrote:
>
> On Fri, 16 Aug 2024 at 10:04, Shubham Khanna
> wrote:
> >
> > On Thu, Aug 8, 2024 at 12:43 PM Peter Smith wrote:
> > >
> > > Hi Shubham,
> > >
> > > I think the v25-0001 patch only hal
make the comment
> clearer about that.
> nit - Use sentence case in the comments.
I have addressed all the comments in the v-28-0001 Patch. Please refer
to the updated v28-0001 Patch here in [1]. See [1] for the changes
added.
[1]
https://www.postgresql.org/message-id/CAHv8RjL7rkxk6qSroRPg5ZARWMdK2Nd4-QyYNeoc2vhBm3cdDg%40mail.gmail.com
Thanks and Regards,
Shubham Khanna.
om scenario "PASS".
>
> * The test "# TEST tab_alter" expected empty result also seems
> unhelpful. It might be related to the previous bullet.
I have addressed all the comments in the v-28-0002 Patch. Please refer
to the updated v28-0002 Patch here in [1]. See [1] for the changes
added.
[1]
https://www.postgresql.org/message-id/CAHv8RjL7rkxk6qSroRPg5ZARWMdK2Nd4-QyYNeoc2vhBm3cdDg%40mail.gmail.com
Thanks and Regards,
Shubham Khanna.
On Tue, Sep 10, 2024 at 2:51 AM Masahiko Sawada wrote:
>
> On Mon, Sep 9, 2024 at 2:38 AM Shubham Khanna
> wrote:
> >
> > On Thu, Aug 29, 2024 at 11:46 AM Amit Kapila
> > wrote:
> > >
> > > On Thu, Aug 29, 2024 at 8:44 AM Masahiko Sawada
> >
if
> there was no publication column list
> AFTER: BMS 'columns' means "columns to be replicated" or NULL if all
> columns are to be replicated
I have addressed all the comments in v19-0004 patch.
Please refer to the updated v19-0004 Patch here in [1]. See [1] for
the changes added.
[1]
https://www.postgresql.org/message-id/CAHv8Rj%2BR0cj%3Dz1bTMAgQKQWx1EKvkMEnV9QsHGvOqTdnLUQi1A%40mail.gmail.com
Thanks and Regards,
Shubham Khanna.
t;b"
> 2024-08-05 13:25:24.926 AEST [11225] CONTEXT: processing remote data
> for replication origin "pg_16390" during message type "INSERT" in
> transaction 742, finished at 0/1967BB0
> 2024-08-05 13:25:24.927 AEST [20039] LOG: background worker "logical
> replication apply worker" (PID 11225) exited with exit code 1
>
This is an expected behaviour. The error message here is improvised.
This error is consistent and it is being handled in the 0002 patch.
Below are the logs for the same:
2024-08-07 10:47:45.977 IST [29756] LOG: logical replication table
synchronization worker for subscription "sub1", table
"tab_nogen_to_gen" has started
2024-08-07 10:47:46.116 IST [29756] ERROR: logical replication target
relation "public.tab_nogen_to_gen" has a generated column "b" but
corresponding column on source relation is not a generated column
0002 Patch needs to be applied to get rid of this error.
Thanks and Regards,
Shubham Khanna.
On Thu, Oct 17, 2024 at 3:59 PM vignesh C wrote:
>
> On Wed, 16 Oct 2024 at 23:25, Shubham Khanna
> wrote:
> >
> > On Wed, Oct 9, 2024 at 9:08 AM vignesh C wrote:
> > >
> > > On Tue, 8 Oct 2024 at 11:37, Shubham Khanna
> > > wrote:
> >
On Wed, Oct 9, 2024 at 11:00 AM vignesh C wrote:
>
> On Tue, 8 Oct 2024 at 11:37, Shubham Khanna
> wrote:
> >
> > On Fri, Oct 4, 2024 at 9:36 AM Peter Smith wrote:
> > >
> > > Hi Shubham, here are my review comments for v36-0001.
> > >
the following cases: a. Any
> + * publication that is not FOR ALL TABLES. b. When the publication is
> + * FOR ALL TABLES and 'publish_generated_columns' is false. FOR ALL
> + * TABLES publication doesn't have user-defined column lists, so all
> + * columns will be replicated by default. However, if
> + * 'publish_generated_columns' is set to false, column lists must
> + * still be created to exclude any generated columns from being
> + * published.
> */
>
> nit - please reformat this comment so the bullets are readable
>
I have fixed all the comments and posted the v39 patches for them.
Please refer to the updated v39 Patches here in [1]. See [1] for the
changes added.
[1]
https://www.postgresql.org/message-id/CAHv8RjLjb%2B98i5ZQUphivxdOZ3hSGLfq2SiWQetUvk8zGyAQwQ%40mail.gmail.com
Thanks and Regards,
Shubham Khanna.
On Wed, Oct 9, 2024 at 11:52 AM vignesh C wrote:
>
> On Tue, 8 Oct 2024 at 11:37, Shubham Khanna
> wrote:
> >
> > On Fri, Oct 4, 2024 at 9:36 AM Peter Smith wrote:
> > >
> > > Hi Shubham, here are my review comments for v36-0001.
> > >
-
>
> Again, I did not understand how these test cases differ from each
> other. Surely, those can be combined easily enough just by adding
> another table with a different kind of column list.
>
> ~~~
>
> 9.
> +# ------
> +# Testcase: Publisher replicates all columns if publish_generated_columns is
> +# enabled and there is no column list
> +# --
> +
>
> Here is yet another test case that AFAICT can just be combined with
> other test cases that were using publish_generated_columns=true. It
> seems all you need is one extra table with no column list. You don't
> need all the extra create/drop pub/sub overheads to test this.
>
> ==
I have fixed all the comments and posted the v39 patches for them.
Please refer to the updated v39 Patches here in [1]. See [1] for the
changes added.
[1]
https://www.postgresql.org/message-id/CAHv8RjLjb%2B98i5ZQUphivxdOZ3hSGLfq2SiWQetUvk8zGyAQwQ%40mail.gmail.com
Thanks and Regards,
Shubham Khanna.
ql.org/message-id/CAHut%2BPu7EcK_JTgWS7GzeStHk6Asb1dmEzCJU2TJf%2BW1Zy30LQ%40mail.gmail.com
>
I have fixed the comments and posted the v37 patches for them. Please
refer to the updated v37 Patches here in [1]. See [1] for
the changes added.
[1]
https://www.postgresql.org/message-id/CAHv8Rj%2BRnw%2B_SfSyyrvWL49AfJzx4O8YVvdU9gB%2BSQdt3%3DqF%2BA%40mail.gmail.com
Thanks and Regards,
Shubham Khanna.
wing was done on the PUBLISHER node:
CREATE TABLE t1 (c1 int, c2 int GENERATED ALWAYS AS (c1 * 2) STORED);
INSERT INTO t1 (c1) VALUES (1), (2);
CREATE PUBLICATION pub1 for table t1;
Following was done on the SUBSCRIBER node:
CREATE TABLE t1 (c1 int, c2 int);
CREATE SUBSCRIPTION sub1 CONNECTION 'dbname=postgres' PUBLICATION pub1;
Following Error occurs repeatedly in the Subscriber log files:
ERROR: could not start initial contents copy for table "public.t1":
ERROR: column "c2" is a generated column
DETAIL: Generated columns cannot be used in COPY.
Thanks and Regards,
Shubham Khanna.
elow?
>
> ```
> "SELECT DISTINCT gpt.attrs"
> ```
>
Fixed this.
The v44 version patches attached at [1] have the changes for the same.
[1] -
https://www.postgresql.org/message-id/CAHv8RjLvr8ZxX-1TcaxrZns1nwgrVUTO_2jhDdOPys0WgrDyKQ%40mail.gmail.com
Thanks and Regards,
Shubham Khanna.
int PRIMARY KEY, b int);
> +));
> +
> +$node_subscriber->safe_psql(
> + 'postgres', qq(
> + CREATE SUBSCRIPTION sub_gen CONNECTION '$publisher_connstr'
> PUBLICATION pub_gen;
> +));
>
> Should combine these.
>
> ~~~
>
> 7.
> +$node_publisher->wait_for_catchup('sub_gen');
> +
> +is( $node_subscriber->safe_psql(
> + 'postgres', "SELECT * FROM test_gen ORDER BY a"),
> + qq(1|2),
> + 'replication with generated columns in column list');
> +
>
> But, this is only testing normal replication. You should also include
> some initial table data so you can test that the initial table
> synchronization works too. Otherwise, I think current this patch has
> no proof that the initial 'copy_data' even works at all.
>
All the agreed comments have been addressed. Please find the attached
v44 Patches for the required changes.
Thanks and Regards,
Shubham Khanna.
v44-0001-Support-logical-replication-of-generated-columns.patch
Description: Binary data
v44-0002-Support-copy-generated-column-during-table-sync.patch
Description: Binary data
umn
> # list) are considered to have the same column list.
> $node_publisher->safe_psql(
> 'postgres', qq(
> CREATE TABLE test_mix_4 (a int PRIMARY KEY, b int, c int, d int
> GENERATED ALWAYS AS (a + 1) STORED);
> ALTER TABLE test_mix_4 DROP COLUMN c;
>
> - CREATE PUBLICATION pub_mix_7 FOR TABLE test_mix_4 (a, b);
> - CREATE PUBLICATION pub_mix_8 FOR TABLE test_mix_4;
> + CREATE PUBLICATION pub_mix_7 FOR TABLE test_mix_4 WITH
> (publish_generated_columns = true);
> + CREATE PUBLICATION pub_mix_8 FOR TABLE test_mix_4 WITH
> (publish_generated_columns = false);
>
> I felt the comment for this test ought to be saying something more
> about what you are doing with the 'publish_generated_columns'
> parameters and what behaviour it was expecting.
>
> ==
> Please refer to the attachment which addresses some of the nit
> comments mentioned above.
>
> ==
> [1] my review of v31-0001:
> https://www.postgresql.org/message-id/CAHut%2BPsv-neEP_ftvBUBahh%2BKCWw%2BqQMF9N3sGU3YHWPEzFH-Q%40mail.gmail.com
> [2] my review of v30-0001:
> https://www.postgresql.org/message-id/CAHut%2BPuaitgE4tu3nfaR%3DPCQEKjB%3DmpDtZ1aWkbwb%3DJZE8YvqQ%40mail.gmail.com
> [3]
> https://www.postgresql.org/message-id/CALDaNm1c7xPBodHw6LKp9e8hvGVJHcKH%3DDHK0iXmZuXKPnxZ3Q%40mail.gmail.com
> [4]
> https://www.postgresql.org/message-id/CAHut%2BPv45gB4cV%2BSSs6730Kb8urQyqjdZ9PBVgmpwqCycr1Ybg%40mail.gmail.com
>
I have addressed all the comments in the v34-0001 Patch. Please refer
to the updated v34-0001 Patch here in [1]. See [1] for the changes
added.
[1]
https://www.postgresql.org/message-id/CAHv8RjJkUdYCdK_bL3yvEV%3DzKrA2dsnZYa1VMT2H5v0%2BqbaGbA%40mail.gmail.com
Thanks and Regards,
Shubham Khanna.
;ve changed it to like I meant in the nitpicks attachment.
> Please see if that works the same.
>
> ==
> [1] my review of v31-0002.
> https://www.postgresql.org/message-id/CAHut%2BPusbhvPrL1uN1TKY%3DFd4zu3h63eDebZvsF%3Duy%2BLBKTwgA%40mail.gmail.com
>
I have addressed all the comments in the v34-0002 Patch. Please refer
to the updated v34-0002 Patch here in [1]. See [1] for the changes
added.
[1]
https://www.postgresql.org/message-id/CAHv8RjJkUdYCdK_bL3yvEV%3DzKrA2dsnZYa1VMT2H5v0%2BqbaGbA%40mail.gmail.com
Thanks and Regards,
Shubham Khanna.
On Mon, Sep 23, 2024 at 6:19 PM vignesh C wrote:
>
> On Fri, 20 Sept 2024 at 17:15, Shubham Khanna
> wrote:
> >
> > On Wed, Sep 11, 2024 at 8:55 AM Peter Smith wrote:
> >
> > I have fixed all the comments. The attached patches contain the desired
> > ch
EATE SUBSCRIBER docs. But not here.
>
> (I removed this in my nitpicks attachment)
>
> TBH, it would be better if patches 0001 and 0002 were merged then you
> can avoid all this. IIUC they were only separate in the first place
> because 2 different people wrote them. It is not making reviews easier
> with them split.
>
> ==
>
> Please see the attachment which implements some of the nits above.
>
I have addressed all the comments in the v32-0001 Patch. Please refer
to the updated v32-0001 Patch here in [1]. See [1] for the changes
added.
[1]
https://www.postgresql.org/message-id/CAHv8RjKkoaS1oMsFvPRFB9nPSVC5p_D4Kgq5XB9Y2B2xU7smbA%40mail.gmail.com
Thanks and Regards,
Shubham Khanna.
sing with Sawada-San -- e.g. when there are 2
> publications for the same table subscribed by just 1 subscription but
> having different values of the 'publish_generated_columns' for the
> publications.
>
I have addressed all the comments in the v32-0001 Patch. Please refer
to the updated v32-0001 Patch here in [1]. See [1] for the changes
added.
[1]
https://www.postgresql.org/message-id/CAHv8RjKkoaS1oMsFvPRFB9nPSVC5p_D4Kgq5XB9Y2B2xU7smbA%40mail.gmail.com
Thanks and Regards,
Shubham Khanna.
> + if (!gencols_allowed)
> + appendStringInfo(&cmd, " AND a.attgenerated = ''");
>
> Can the 'gencols_allowed' var be removed, and the condition just be
> replaced with if (!has_pub_with_pubgencols)? It seems equivalent
> unless I am mistaken.
>
> ==
>
> Please refer to the attachment which implements some of the nits
> mentioned above.
>
> ==
> [1]
> https://www.postgresql.org/message-id/CAD21AoBun9crSWaxteMqyu8A_zme2ppa2uJvLJSJC2E3DJxQVA%40mail.gmail.com
>
I have addressed the comments in the v32-0002 Patch. Please refer to
the updated v32-0002 Patch here in [1]. See [1] for the changes added.
[1]
https://www.postgresql.org/message-id/CAHv8RjKkoaS1oMsFvPRFB9nPSVC5p_D4Kgq5XB9Y2B2xU7smbA%40mail.gmail.com
Thanks and Regards,
Shubham Khanna.
I don't see any change yet.
>
This comment is still open. Will fix this and post in the next version
of patches.
Please refer to the updated v36-0002 Patch here in [1]. See [1] for
the changes added.
[1]
https://www.postgresql.org/message-id/CAHv8Rj%2B1RDd7AnJNzOJXk--zcbTtU3nys%3DZgU3ktB4e3DWbJgg%40mail.gmail.com
Thanks and Regards,
Shubham Khanna.
ublish_as_relid))
>
> Let's change this comment to: "The column list takes precedence over
> publish_generated_columns option. Those will be checked later, see
> pgoutput_column_list_init."
>
Fixed.
The v43 version patch attached at [1] has the changes for the same.
[1] -
https://www.postgresql.org/message-id/CAHv8RjJJJRzy83tG0nB90ivYcp7sFKTU%3D_BcQ-nUZ7VbHFwceA%40mail.gmail.com
Thanks and Regards,
Shubham Khanna.
-
> +# A "regular -> missing" replication fails, reporting an error
> +# that the subscriber side is missing replicated columns.
> +#
> +# Testcase: regular -> missing
> +# Publisher table has regular columns 'c2' and 'c3'.
> +# Subscriber table is missing columns 'c2' and 'c3'.
> +# --
>
> I've also added the "generated -> missing" combination and addressed
> the review comment about intercluding a test where there are BOTH
> missing and generated columns, so you can see which error takes
> precedence. Please see the NITPICKS diff.
>
I have fixed the given comments. The attached Patch contains the
required changes.
Thanks and regards,
Shubham Khanna.
v5-0001-Error-message-improvement.patch
Description: Binary data
On Sat, Nov 16, 2024 at 5:43 PM Shlok Kyal wrote:
>
> On Fri, 15 Nov 2024 at 15:57, Shubham Khanna
> wrote:
> >
> > On Thu, Nov 14, 2024 at 2:09 PM Peter Smith wrote:
> > >
> > > Hi Shubham,
> > >
> > > +1 for the patch idea.
>
ments. The v2 version patch attached at [1]
has the changes for the same.
[1] -
https://www.postgresql.org/message-id/CAHv8RjJfuLO7HK1P%3DhaY2stdGxYRAqrOwe6Ov4rzsprU63NQkg%40mail.gmail.com
Thanks and Regards,
Shubham Khanna.
mn problems where the
> column names are matching, not others
>
> ~~~~
>
> 6.
> Also, as previously mentioned in internal reviews, this patch should
> include a 2nd test case to do pretty much the same testing but
> expecting to get a "missing replication column".
>
On Fri, Nov 15, 2024 at 7:07 PM vignesh C wrote:
>
> On Fri, 15 Nov 2024 at 15:57, Shubham Khanna
> wrote:
> >
> > I have fixed the given comments. The attached Patch contains the
> > required changes.
>
> Few comments:
> 1)
> a)You can mention that
On Mon, Nov 18, 2024 at 4:11 PM vignesh C wrote:
>
> On Mon, 18 Nov 2024 at 15:47, Shubham Khanna
> wrote:
> >
> > On Fri, Nov 15, 2024 at 7:07 PM vignesh C wrote:
> >
> > I have fixed the given comments. The attached Patch contains the
> > required cha
lication of the generated column on the subscriber is not
supported. I have attached a patch for the same.
[1]:
https://www.postgresql.org/message-id/TYCPR01MB5693AF061D62E55189490D2DF5562%40TYCPR01MB5693.jpnprd01.prod.outlook.com
Thanks and Regards,
Shubham Khanna.
v1-0001-Error-message-improvement.patch
Description: Binary data
sing' error takes precedence.
> + */
> + logicalrep_report_missing_and_gen_attrs(remoterel, missingatts,
> + generatedattrs);
>
> This comment can also be removed. The function name is already
> self-explanatory, and the information of the "Note" part belongs in
> the function comment.
>
> ==
> src/test/subscription/t/011_generated.pl
>
> The tests LGTM.
>
> ==
>
> Please refer to the attached diffs patch which includes most (but not
> all) of the suggestions mentioned above.
>
> ==
> [1]
> https://www.postgresql.org/message-id/CAHut%2BPuoDsPUO1YDBOEWAsKT8dXA0PDoK6S_Yc6kO_s8yPKHfA%40mail.gmail.com
>
I have fixed the given comments. The attached Patch contains the
required changes.
Thanks and regards,
Shubham Khanna.
v6-0001-Error-message-improvement.patch
Description: Binary data
with two_phase
option and make use of the advantages of creating subscription with
two_phase option.
The attached patch has the changes for the same.
[1] -
https://www.postgresql.org/message-id/CAA4eK1%2B7gRxiK3xNEH03CY3mMKxqi7BVz2k3VxxVZp8fgjHZxg%40mail.gmail.com
Thanks and regards,
Shubham Khanna
On Mon, Dec 30, 2024 at 10:10 AM vignesh C wrote:
>
> On Mon, 30 Dec 2024 at 09:34, Shubham Khanna
> wrote:
> >
> > Hi,
> >
> > Currently, there is a risk that pg_createsubscriber may fail to
> > complete successfully when the max_slot_wal_keep_size valu
efault is false.
>
I have fixed the given comment. The attached patch contains the
suggested changes.
Thanks and regards,
Shubham Khanna.
v9-0001-Add-support-for-two-phase-commit-in-pg_createsubs.patch
Description: Binary data
On Mon, Dec 30, 2024 at 4:03 PM vignesh C wrote:
>
> On Mon, 30 Dec 2024 at 12:04, Shubham Khanna
> wrote:
> >
> > On Mon, Dec 30, 2024 at 10:10 AM vignesh C wrote:
> > >
> > > On Mon, 30 Dec 2024 at 09:34, Shubham Khanna
> > > wrote:
> >
On Tue, Feb 4, 2025 at 3:49 PM Ashutosh Bapat
wrote:
>
> Hi Shubham,
>
> On Tue, Feb 4, 2025 at 2:10 PM Shubham Khanna
> wrote:
> > > >
> > >
> > > It could be a bit tricky to find that for users but they can devise a
> > > query to get
t; query to get the names and numbers of databases matching the given
> pattern. OTOH, I am not sure there is a clear need at this stage for
> pattern matching for this tool. So, we can go with a simple switch as
> you are proposing at this stage.
>
After reconsidering the idea of support
On Tue, Feb 4, 2025 at 5:39 PM Shlok Kyal wrote:
>
> On Wed, 29 Jan 2025 at 15:14, Shubham Khanna
> wrote:
> >
> > On Wed, Jan 29, 2025 at 10:42 AM Hayato Kuroda (Fujitsu)
> > wrote:
> > >
> > > Dear Shubham,
> > >
> > >
"--replslot"
>
> ~~~
>
> 12.
> - * If --database option is not provided, try to obtain the dbname from
> - * the publisher conninfo. If dbname parameter is not available, error
> - * out.
> + * If neither --database nor --all-databases option is provided, try
> + * to obtain the dbname from the publisher conninfo. If dbname
> + * parameter is not available, error out.
>
> For this comment to make sense I think the previous comment (where
> fetch_source_databases is called) needs to explain that when
> --all-databases is defined then it is going to treat that internally
> as the equivalent of a whole lot of user-specified --database options.
>
> ==
> .../t/040_pg_createsubscriber.pl
>
> 13.
> + qr/cannot specify --publication or --replication-slot when using
> --all-databases/,
> + 'fail if --all-databases is used with publication and slot');
> +
>
> How are tests expecting this even passing?
>
> Searching the patch I cannot find any such error!
>
> ==
Fixed the given comments. The attached patch at [1] contains the
suggested changes.
[1] -
https://www.postgresql.org/message-id/CAHv8Rj%2BmhYgh8XLFM8sN8J05z0ia%2BknfB1kP6kjbnB55H-b-mw%40mail.gmail.com
Thanks and regards,
Shubham Khanna.
tabase cannot be
> used with --all-databases");
> + exit(1);
> +
> ```
>
> I think this erroring is not enough. getopt_long() receives options with the
> specified ordering, thus
> -d can be accepted if the -a is specified latter.
&
On Wed, Feb 5, 2025 at 5:31 PM Shlok Kyal wrote:
>
> On Wed, 5 Feb 2025 at 01:26, Shubham Khanna
> wrote:
> >
> > On Tue, Feb 4, 2025 at 3:49 PM Ashutosh Bapat
> > wrote:
> > >
> > > Hi Shubham,
> > >
> >
;
> +is($db_count_before, '1', 'database count without --all-databases option');
> ```
>
> This test is not actually realted with our codes, no need to do.
>
> 11. 040_pg_createsubscriber.pl
> ```
> +# Ensure there are some user databases on the publisher
> +$node_a->safe_psql('postgres', 'CREATE DATABASE db3');
> +$node_a->safe_psql('postgres', 'CREATE DATABASE db4');
> +
> +$node_b->stop;
> ```
>
> You must wait until all changes have been replicated to the standby here.
>
> 12. 040_pg_createsubscriber.pl
>
> Can we do the similar tests without adding new instances? E.g., runs with
> dry-run mode and confirms all databases become target.
>
Fixed the given comments. The attached patch at [1] contains the
suggested changes.
[1] -
https://www.postgresql.org/message-id/CAHv8Rj%2BDOQ9vvkKCsmDeFKyM073yRfs47Tn_qywQOdx15pmOMA%40mail.gmail.com
Thanks and regards,
Shubham Khanna.
> 16a.
> Why is this code checking 'opt->all_databases'? Isn't it only possible
> to get to this function via --all-databases. You can just Assert this.
>
> ~
>
> 16b.
> Why are you reusing 'slot_name' variable like this?
>
> ~
>
> 16c.
> Was there some ERROR checking missing here to ensure the length of the
> dbname is not going to cause pub/slot to exceed NAMEDATALEN?
>
> ~
>
> 16d.
> In hindsight, most of this function seems unnecessary to me. Probably
> you could've done all this pub/slot name assignment inside the
> fetch_all_databases() at the same time as you were doing
> simple_string_list_append(&opt->database_names, dbname); for each
> database.
>
> ~~~
>
> 17.
> - while ((c = getopt_long(argc, argv, "d:D:np:P:s:t:U:v",
> + while ((c = getopt_long(argc, argv, "ad:D:np:P:s:t:U:v",
> long_options, &option_index)) != -1)
>
>
> Isn't the code within this loop missing all the early switch
> validation for checking you are not combining incompatible switches?
>
> e.g.
> --all-databases - "Cannot specify --all-databases more than once"
> --all-databases - "Cannot combine with --database, --publication,
> --subscription, --replication-slot"
> --database - "Cannot combine with --all-databases"
> --publication - "Cannot combine with --all-databases"
> --subscription - "Cannot combine with --all-databases"
> --replication-slot - "Cannot combine with --all-databases"
>
> ~~~
>
> 18.
> +
> +
> /* Number of object names must match number of databases */
>
> Remove spurious whitespace.
>
> ==
Fixed the given comments. The attached patch at [1] contains the
suggested changes.
[1] -
https://www.postgresql.org/message-id/CAHv8Rj%2BDOQ9vvkKCsmDeFKyM073yRfs47Tn_qywQOdx15pmOMA%40mail.gmail.com
Thanks and regards,
Shubham Khanna.
>
> (same as the previous review comment).
>
> Previously reported. Claimed fixed -a to --all-databases. Not fixed.
>
Fixed.
> ==
> src/bin/pg_basebackup/pg_createsubscriber.c
>
> 5.
> + bool all_databases; /* fetch and specify all databases */
>
> Comment wording. "and specified" (??). Maybe just "--all-databases
> option was specified"
>
> ==
Fixed.
The attached Patch contains the suggested changes.
[1] -
https://www.postgresql.org/message-id/CAHv8Rj%2BDOQ9vvkKCsmDeFKyM073yRfs47Tn_qywQOdx15pmOMA%40mail.gmail.com
Thanks and regards,
Shubham Khanna.
v6-0001-Enhance-pg_createsubscriber-to-fetch-and-append-a.patch
Description: Binary data
ated before --cleanup-publisher-objects is run');
> +
> ```
>
> Also, there is a possibility that CREATE PUBLICATION on node_p is not
> replicated yet
> when SELECT COUNT(*) is executed. Please wait for the replay.
>
> [1]:
> https://www.postgresql.org/message-id/CAHv8RjL4OvoYafofTb_U_JD5HuyoNowBoGpMfnEbhDSENA74Kg%40mail.gmail.com
> [2]:
> https://www.postgresql.org/message-id/OSCPR01MB1496664FDC38DA40A441F449FF5EE2%40OSCPR01MB14966.jpnprd01.prod.outlook.com
> [3]: https://www.postgresql.org/docs/devel/source-format.html
>
Fixed.
The attached Patch contains the suggested changes.
Thanks and regards,
Shubham Khanna.
v4-0001-Support-for-dropping-all-publications-in-pg_creat.patch
Description: Binary data
On Wed, Feb 12, 2025 at 1:15 PM Shubham Khanna
wrote:
>
> 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 th
= off');
> +$node_a->start;
> +
> +# Set up node B as standby linking to node A
> +$node_a->backup('backup_3');
> +my $node_b = PostgreSQL::Test::Cluster->new('node_b');
> +$node_b->init_from_backup($node_a, 'backup_3', has_streaming => 1);
> +$node_
On Wed, Dec 11, 2024 at 2:08 PM vignesh C wrote:
>
> On Wed, 11 Dec 2024 at 11:21, Shubham Khanna
> wrote:
> >
> > On Wed, Dec 11, 2024 at 4:21 AM Peter Smith wrote:
> >
> > I have fixed the given comments. The attached patch contains the
> > suggested c
1 - 100 of 197 matches
Mail list logo
