ase 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.
> 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.
l 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.
t;
> ~
>
> 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
gres', "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.
ELECT 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.
"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
ould 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.
* 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
> + 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->binary).
Fixed.
> ==
&g
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
ted_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.
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.
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
;
>
> 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
en
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
| 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.
y 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: 0 from 0)
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.
cenario 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 specifying primary
as nod
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 c
patch.
Thanks and Regards,
Shubham Khanna.
v1-0001-Improve-documentation-of-upgrade-for-streaming-re (2).patch
Description: Binary data
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,
-
--------
----------
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.
n_slot('s', 'test_decoding');
> select testfn(5);
> set logical_decoding_work_mem to '4MB';
> select count(*) from pg_logical_slot_peek_changes('s', null, null)";
>
> and here are results:
>
> * HEAD: 16877.281 ms
> * HEAD w/ patches (0001 and 0002): 655.154 ms
>
&
5557e5b in cleanup_objects_atexit () at pg_subscriber.c:173
173if (perdb->made_subscription)
(gdb) p perdb
$1 = (LogicalRepPerdbInfo *) 0x0
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:
> > >
&
c.rej
Please send the Re-base version of the patch.
Thanks and Regards,
Shubham Khanna.
t/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.
ted/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.
.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_subscription' ranging from 2 to 10. I ran the
tests for different data records; for 100MB I put 3,
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.
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 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
ental 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.
> > PGC_S_FILE, ERROR,
> > +
> > PGC_S_TEST, ERROR,
> >
> > , ))
> > 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.
e 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.
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.
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.
t'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.
>> +
> >> + 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.
n 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.
nt 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.)
>
>
> --
> I have Reviewed the patch. Patch
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.
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.
nt 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 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 "ALTE
>
> 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.
;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
iv_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.
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.
51 matches
Mail list logo