Re: long-standing data loss bug in initial sync of logical replication
Hi Kuroda-san, Thanks for reviewing the patch. > > 1. > I feel the name of SnapBuildDistributeNewCatalogSnapshot() should be updated > because it > distributes two objects: catalog snapshot and invalidation messages. Do you > have good one > in your mind? I considered > "SnapBuildDistributeNewCatalogSnapshotAndInValidations" or > "SnapBuildDistributeItems" but seems not good :-(. I have renamed the function to 'SnapBuildDistributeSnapshotAndInval'. Thoughts? > 2. > Hmm, still, it is overengineering for me to add a new type of invalidation > message > only for the publication. According to the ExecRenameStmt() we can implement > an > arbitrary rename function like RenameConstraint() and RenameDatabase(). > Regaring the ALTER PUBLICATION OWNER TO, I feel adding > CacheInvalidateRelcacheAll() > and InvalidatePublicationRels() is enough. I agree with you. > > I attached a PoC which implements above. It could pass tests on my env. Could > you > please see it tell me how you think? I have tested the POC and it is working as expected. The changes look fine to me. I have created a patch for the same. Currently, we are passing 'PUBLICATION_PART_ALL' as an argument to function 'GetPublicationRelations' and 'GetAllSchemaPublicationRelations'. Need to check if we can use 'PUBLICATION_PART_ROOT' or 'PUBLICATION_PART_LEAF' depending on the 'publish_via_partition_root' option. Will test and address this in the next version of the patch. For now, I have added a TODO. Thanks and Regards, Shlok Kyal v12-0001-Distribute-invalidatons-if-change-in-catalog-tab.patch Description: Binary data v12-0002-Selective-Invalidation-of-Cache.patch Description: Binary data
Re: long-standing data loss bug in initial sync of logical replication
On Mon, 30 Sept 2024 at 16:56, Hayato Kuroda (Fujitsu) wrote: > > > 2. > > Similarly with above, the relcache won't be invalidated when ALTER > > PUBLICATION > > OWNER TO is executed. This means that privilage checks may be ignored if the > > entry > > is still valid. Not sure, but is there a possibility this causes an > > inconsistency? > > Hmm, IIUC, the attribute pubowner is not used for now. The paragpargh > "There are currently no privileges on publications" [1] may show the > current > status. However, to keep the current behavior, I suggest to invalidate the > relcache > of pubrelations when the owner is altered. > > [1]: https://www.postgresql.org/docs/devel/logical-replication-security.html I have shared the updated patch [1]. So now, when 'ALTER .. PUBLICATION .. OWNER TO ..' is executed the relcache is invalidated for that specific publication. [1] : https://www.postgresql.org/message-id/CANhcyEWEXL3rxvKH9-Xtx-DgGX0D62EktHpW%2BnG%2BMSSaMVUVig%40mail.gmail.com Thanks and Regards, Shlok Kyal
Re: long-standing data loss bug in initial sync of logical replication
> > I have addressed the comment for 0002 patch and attached the patches. > > Also, I have moved the tests in the 0002 to 0001 patch. > > Thanks for updating the patch. 0002 patch seems to remove cache invalidations > from publication_invalidation_cb(). Related with it, I found an issue and had > a concern. > > 1. > The replication continues even after ALTER PUBLICATION RENAME is executed. > For example - assuming that a subscriber subscribes only "pub": > > ``` > pub=# INSERT INTO tab values (1); > INSERT 0 1 > pub=# ALTER PUBLICATION pub RENAME TO pub1; > ALTER PUBLICATION > pub=# INSERT INTO tab values (2); > INSERT 0 1 > > sub=# SELECT * FROM tab ; -- (2) should not be replicated however... > a > --- > 1 > 2 > (2 rows) > ``` > > This happens because 1) ALTER PUBLICATION RENAME statement won't be > invalidate the > relation cache, and 2) publications are reloaded only when invalid > RelationSyncEntry > is found. In given example, the first INSERT creates the valid cache and > second > INSERT reuses it. Therefore, the pubname-check is skipped. > > For now, the actual renaming is done at AlterObjectRename_internal(), a > generic > function. I think we must implement a dedecated function to publication and > must > invalidate relcaches there. > > 2. > Similarly with above, the relcache won't be invalidated when ALTER PUBLICATION > OWNER TO is executed. This means that privilage checks may be ignored if the > entry > is still valid. Not sure, but is there a possibility this causes an > inconsistency? > Hi Kuroda-san, Thanks for testing the patch. I have fixed the comments and attached the updated patch. I have added a callback function rel_sync_cache_publicationrel_cb(). This callback invalidates the cache of tables in a particular publication. This callback is called when there is some modification in pg_publication catalog. I have tested the two cases 'ALTER PUBLICATION ... RENAME TO ...' and 'ALTER PUBLICATION ... OWNER TO ...' and debugged it. The newly added callback is called and it invalidates the cache of tables present in that particular publication. I have also added a test related to 'ALTER PUBLICATION ... RENAME TO ...' to 0001 patch. Thanks and Regards, Shlok Kyal v11-0001-Distribute-invalidatons-if-change-in-catalog-tab.patch Description: Binary data v11-0002-Selective-Invalidation-of-Cache.patch Description: Binary data
Re: long-standing data loss bug in initial sync of logical replication
On Thu, 26 Sept 2024 at 11:39, Shlok Kyal wrote: > > > In the v7 patch, I am looping through the reorder buffer of the > > current committed transaction and storing all invalidation messages in > > a list. Then I am distributing those invalidations. > > But I found that for a transaction we already store all the > > invalidation messages (see [1]). So we don't need to loop through the > > reorder buffer and store the invalidations. > > > > I have modified the patch accordingly and attached the same. > > > > [1]: > > https://github.com/postgres/postgres/blob/7da1bdc2c2f17038f2ae1900be90a0d7b5e361e0/src/include/replication/reorderbuffer.h#L384 > > Hi, > > I tried to add changes to selectively invalidate the cache to reduce > the performance degradation during the distribution of invalidations. > > Here is the analysis for selective invalidation. > Observation: > Currently when there is a change in a publication, cache related to > all the tables is invalidated including the ones that are not part of > any publication and even tables of different publications. For > example, suppose pub1 includes tables t1 to t1000, while pub2 contains > just table t1001. If pub2 is altered, even though it only has t1001, > this change will also invalidate all the tables t1 through t1000 in > pub1. > Similarly for a namespace, whenever we alter a schema or we add/drop a > schema to the publication, cache related to all the tables is > invalidated including the ones that are on of different schema. For > example, suppose pub1 includes tables t1 to t1000 in schema sc1, while > pub2 contains just table t1001 in schema sc2. If schema ‘sc2’ is > changed or if it is dropped from publication ‘pub2’ even though it > only has t1001, this change will invalidate all the tables t1 through > t1000 in schema sc1. > ‘rel_sync_cache_publication_cb’ function is called during the > execution of invalidation in both above cases. And > ‘rel_sync_cache_publication_cb’ invalidates all the tables in the > cache. > > Solution: > 1. When we alter a publication using commands like ‘ALTER PUBLICATION > pub_name DROP TABLE table_name’, first all tables in the publications > are invalidated using the function ‘rel_sync_cache_relation_cb’. Then > again ‘rel_sync_cache_publication_cb’ function is called which > invalidates all the tables. This happens because of the following > callback registered: > CacheRegisterSyscacheCallback(PUBLICATIONRELMAP, > rel_sync_cache_publication_cb, (Datum) 0); > > So, I feel this second function call can be avoided. And I have > included changes for the same in the patch. Now the behavior will be > as: > suppose pub1 includes tables t1 to t1000, while pub2 contains just > table t1001. If pub2 is altered, it will only invalidate t1001. > > 2. When we add/drop a schema to/from a publication using command like > ‘ALTER PUBLICATION pub_name ADD TABLES in SCHEMA schema_name’, first > all tables in that schema are invalidated using > ‘rel_sync_cache_relation_cb’ and then again > ‘rel_sync_cache_publication_cb’ function is called which invalidates > all the tables. This happens because of the following callback > registered: > CacheRegisterSyscacheCallback(PUBLICATIONNAMESPACEMAP, > rel_sync_cache_publication_cb, (Datum) 0); > > So, I feel this second function call can be avoided. And I have > included changes for the same in the patch. Now the behavior will be > as: > suppose pub1 includes tables t1 to t1000 in schema sc1, while pub2 > contains just table t1001 in schema sc2. If schema ‘sc2’ dropped from > publication ‘pub2’, it will only invalidate table t1001. > > 3. When we alter a namespace using command like ‘ALTER SCHEMA > schema_name RENAME to new_schema_name’ all the table in cache are > invalidated as ‘rel_sync_cache_publication_cb’ is called due to the > following registered callback: > CacheRegisterSyscacheCallback(NAMESPACEOID, > rel_sync_cache_publication_cb, (Datum) 0); > > So, we added a new callback function ‘rel_sync_cache_namespacerel_cb’ > will be called instead of function ‘rel_sync_cache_publication_cb’ , > which invalidates only the cache of the tables which are part of that > particular namespace. For the new function the ‘namespace id’ is added > in the Invalidation message. > > For example, if namespace ‘sc1’ has table t1 and t2 and a namespace > ‘sc2’ has table t3. Then if we rename namespace ‘sc1’ to ‘sc_new’. > Only tables in sc1 i.e. tables t1 and table t2 are invalidated. > > > Performance Comparison: > I have run the same tests as shared in [1] and observed a significant > decrease in the degradation with the new changes. With selective > invalidation degradation is aro
Re: long-standing data loss bug in initial sync of logical replication
Hi Kuroda-san, Thanks for reviewing the patch. > > Solution: > > 1. When we alter a publication using commands like ‘ALTER PUBLICATION > > pub_name DROP TABLE table_name’, first all tables in the publications > > are invalidated using the function ‘rel_sync_cache_relation_cb’. Then > > again ‘rel_sync_cache_publication_cb’ function is called which > > invalidates all the tables. > > On my environment, rel_sync_cache_publication_cb() was called first and > invalidate > all the entries, then rel_sync_cache_relation_cb() was called and the > specified > entry is invalidated - hence second is NO-OP. > You are correct. I made a silly mistake while writing the write-up. rel_sync_cache_publication_cb() is called first and invalidate all the entries, then rel_sync_cache_relation_cb() is called and the specified entry is invalidated > > This happens because of the following > > callback registered: > > CacheRegisterSyscacheCallback(PUBLICATIONRELMAP, > > rel_sync_cache_publication_cb, (Datum) 0); > > But even in this case, I could understand that you want to remove the > rel_sync_cache_publication_cb() callback. Yes, I think rel_sync_cache_publication_cb() callback can be removed, as it is invalidating all the other tables as well (which are not in this publication). > > 2. When we add/drop a schema to/from a publication using command like > > ‘ALTER PUBLICATION pub_name ADD TABLES in SCHEMA schema_name’, first > > all tables in that schema are invalidated using > > ‘rel_sync_cache_relation_cb’ and then again > > ‘rel_sync_cache_publication_cb’ function is called which invalidates > > all the tables. > > Even in this case, rel_sync_cache_publication_cb() was called first and then > rel_sync_cache_relation_cb(). > Yes, your observation is correct. rel_sync_cache_publication_cb() is called first and then rel_sync_cache_relation_cb(). > > > > 3. When we alter a namespace using command like ‘ALTER SCHEMA > > schema_name RENAME to new_schema_name’ all the table in cache are > > invalidated as ‘rel_sync_cache_publication_cb’ is called due to the > > following registered callback: > > CacheRegisterSyscacheCallback(NAMESPACEOID, > > rel_sync_cache_publication_cb, (Datum) 0); > > > > So, we added a new callback function ‘rel_sync_cache_namespacerel_cb’ > > will be called instead of function ‘rel_sync_cache_publication_cb’ , > > which invalidates only the cache of the tables which are part of that > > particular namespace. For the new function the ‘namespace id’ is added > > in the Invalidation message. > > Hmm, I feel this fix is too much. Unlike ALTER PUBLICATION statements, I think > ALTER SCHEMA is rarely executed at the production stage. However, this > approach > requires adding a new cache callback system, which affects the entire postgres > system; this is not very beneficial compared to the outcome. It should be > discussed > on another thread to involve more people, and then we can add the improvement > after being accepted. > Yes, I also agree with you. I have removed the changes in the updated patch. > > Performance Comparison: > > I have run the same tests as shared in [1] and observed a significant > > decrease in the degradation with the new changes. With selective > > invalidation degradation is around ~5%. This results are an average of > > 3 runs. > > IIUC, the executed workload did not contain ALTER SCHEMA command, so > third improvement did not contribute this improvement. I have removed the changes corresponding to the third improvement. I have addressed the comment for 0002 patch and attached the patches. Also, I have moved the tests in the 0002 to 0001 patch. Thanks and Regards, Shlok Kyal v10-0002-Selective-Invalidation-of-Cache.patch Description: Binary data v10-0001-Distribute-invalidatons-if-change-in-catalog-tab.patch Description: Binary data
Re: Using per-transaction memory contexts for storing decoded tuples
On Mon, 23 Sept 2024 at 09:59, Amit Kapila wrote:
>
> On Sun, Sep 22, 2024 at 11:27 AM David Rowley wrote:
> >
> > On Fri, 20 Sept 2024 at 17:46, Amit Kapila wrote:
> > >
> > > On Fri, Sep 20, 2024 at 5:13 AM David Rowley wrote:
> > > > In general, it's a bit annoying to have to code around this
> > > > GenerationContext fragmentation issue.
> > >
> > > Right, and I am also slightly afraid that this may not cause some
> > > regression in other cases where defrag wouldn't help.
> >
> > Yeah, that's certainly a possibility. I was hoping that
> > MemoryContextMemAllocated() being much larger than logical_work_mem
> > could only happen when there is fragmentation, but certainly, you
> > could be wasting effort trying to defrag transactions where the
> > changes all arrive in WAL consecutively and there is no
> > defragmentation. It might be some other large transaction that's
> > causing the context's allocations to be fragmented. I don't have any
> > good ideas on how to avoid wasting effort on non-problematic
> > transactions. Maybe there's something that could be done if we knew
> > the LSN of the first and last change and the gap between the LSNs was
> > much larger than the WAL space used for this transaction. That would
> > likely require tracking way more stuff than we do now, however.
> >
>
> With more information tracking, we could avoid some non-problematic
> transactions but still, it would be difficult to predict that we
> didn't harm many cases because to make the memory non-contiguous, we
> only need a few interleaving small transactions. We can try to think
> of ideas for implementing defragmentation in our code if we first can
> prove that smaller block sizes cause problems.
>
> > With the smaller blocks idea, I'm a bit concerned that using smaller
> > blocks could cause regressions on systems that are better at releasing
> > memory back to the OS after free() as no doubt malloc() would often be
> > slower on those systems. There have been some complaints recently
> > about glibc being a bit too happy to keep hold of memory after free()
> > and I wondered if that was the reason why the small block test does
> > not cause much of a performance regression. I wonder how the small
> > block test would look on Mac, FreeBSD or Windows. I think it would be
> > risky to assume that all is well with reducing the block size after
> > testing on a single platform.
> >
>
> Good point. We need extensive testing on different platforms, as you
> suggest, to verify if smaller block sizes caused any regressions.
I did similar tests on Windows. rb_mem_block_size was changed from 8kB
to 8MB. Below table shows the result (average of 5 runs) and Standard
Deviation (of 5 runs) for each block-size.
===
block-size |Average time (ms) |Standard Deviation (ms)
-
8kb|12580.879 ms |144.6923467
16kb |12442.7256 ms |94.02799006
32kb |12370.7292 ms |97.7958552
64kb |11877.4888 ms |222.2419142
128kb|11828.8568 ms |129.732941
256kb|11801.086 ms |20.60030913
512kb|12361.4172 ms |65.27390105
1MB |12343.3732 ms |80.84427202
2MB |12357.675 ms |79.40017604
4MB | 12395.8364 ms |76.78273689
8MB |11712.8862 ms |50.74323039
==
>From the results, I think there is a small regression for small block size.
I ran the tests in git bash. I have also attached the test script.
Thanks and Regards,
Shlok Kyal
#!/bin/bash
if [ -z "$1" ]
then
size="8kB"
else
size=$1
fi
echo 'Clean up'
echo $size
./pg_ctl stop -D data
rm -rf data* logfile
echo 'Set up'
./initdb -D data -U postgres
cat << EOF >> data/postgresql.conf
wal_level = logical
autovacuum = false
checkpoint_timeout = 1h
shared_buffers = '10GB'
work_mem = '1GB'
logical_decoding_work_mem = '2097151 kB'
max_wal_size = 20GB
min_wal_size = 10GB
rb_mem_block_size = $size
EOF
./pg_ctl -D data start -w -l logfile
(
echo -E "SELECT * FROM pg_create_logical_replication_slot('test', 'test_decoding');"
echo -E "CREATE TABLE foo (id int);"
echo -E "INSERT INTO foo VALUES (generate_series(1, 1000));"
) | ./psql -U postgres
for i in `seq 1 5`
do
(
echo -E "\timing"
echo -E "SELECT count(*) FROM pg_logical_slot_peek_changes('test', NULL, NULL);"
) | ./psql -U postgres
done
Re: long-standing data loss bug in initial sync of logical replication
atch for the same
v9-0001 : distribute invalidation to inprogress transaction
v9-0002: Selective invalidation
[1]:https://www.postgresql.org/message-id/CANhcyEW4pq6%2BPO_eFn2q%3D23sgV1budN3y4SxpYBaKMJNADSDuA%40mail.gmail.com
Thanks and Regards,
Shlok Kyal
From 4222dca86e4892fbae6698ed7a6135f61d499d8f Mon Sep 17 00:00:00 2001
From: Shlok Kyal
Date: Fri, 23 Aug 2024 14:02:20 +0530
Subject: [PATCH v9 1/2] Distribute invalidatons if change in catalog tables
Distribute invalidations to inprogress transactions if the current
committed transaction change any catalog table.
---
.../replication/logical/reorderbuffer.c | 5 +-
src/backend/replication/logical/snapbuild.c | 34 +++--
src/include/replication/reorderbuffer.h | 4 +
src/test/subscription/t/100_bugs.pl | 128 ++
4 files changed, 157 insertions(+), 14 deletions(-)
diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index 22bcf171ff..c5dfc1ab06 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -221,9 +221,6 @@ int debug_logical_replication_streaming = DEBUG_LOGICAL_REP_STREAMING_BUFFERED
*/
static ReorderBufferTXN *ReorderBufferGetTXN(ReorderBuffer *rb);
static void ReorderBufferReturnTXN(ReorderBuffer *rb, ReorderBufferTXN *txn);
-static ReorderBufferTXN *ReorderBufferTXNByXid(ReorderBuffer *rb,
- TransactionId xid, bool create, bool *is_new,
- XLogRecPtr lsn, bool create_as_top);
static void ReorderBufferTransferSnapToParent(ReorderBufferTXN *txn,
ReorderBufferTXN *subtxn);
@@ -622,7 +619,7 @@ ReorderBufferReturnRelids(ReorderBuffer *rb, Oid *relids)
* (with the given LSN, and as top transaction if that's specified);
* when this happens, is_new is set to true.
*/
-static ReorderBufferTXN *
+ReorderBufferTXN *
ReorderBufferTXNByXid(ReorderBuffer *rb, TransactionId xid, bool create,
bool *is_new, XLogRecPtr lsn, bool create_as_top)
{
diff --git a/src/backend/replication/logical/snapbuild.c b/src/backend/replication/logical/snapbuild.c
index 0450f94ba8..42c947651b 100644
--- a/src/backend/replication/logical/snapbuild.c
+++ b/src/backend/replication/logical/snapbuild.c
@@ -300,7 +300,7 @@ static void SnapBuildFreeSnapshot(Snapshot snap);
static void SnapBuildSnapIncRefcount(Snapshot snap);
-static void SnapBuildDistributeNewCatalogSnapshot(SnapBuild *builder, XLogRecPtr lsn);
+static void SnapBuildDistributeNewCatalogSnapshot(SnapBuild *builder, XLogRecPtr lsn, TransactionId xid);
static inline bool SnapBuildXidHasCatalogChanges(SnapBuild *builder, TransactionId xid,
uint32 xinfo);
@@ -859,18 +859,21 @@ SnapBuildProcessNewCid(SnapBuild *builder, TransactionId xid,
}
/*
- * Add a new Snapshot to all transactions we're decoding that currently are
- * in-progress so they can see new catalog contents made by the transaction
- * that just committed. This is necessary because those in-progress
- * transactions will use the new catalog's contents from here on (at the very
- * least everything they do needs to be compatible with newer catalog
- * contents).
+ * Add a new Snapshot and invalidation messages to all transactions we're
+ * decoding that currently are in-progress so they can see new catalog contents
+ * made by the transaction that just committed. This is necessary because those
+ * in-progress transactions will use the new catalog's contents from here on
+ * (at the very least everything they do needs to be compatible with newer
+ * catalog contents).
*/
static void
-SnapBuildDistributeNewCatalogSnapshot(SnapBuild *builder, XLogRecPtr lsn)
+SnapBuildDistributeNewCatalogSnapshot(SnapBuild *builder, XLogRecPtr lsn, TransactionId xid)
{
dlist_iter txn_i;
ReorderBufferTXN *txn;
+ ReorderBufferTXN *curr_txn;
+
+ curr_txn = ReorderBufferTXNByXid(builder->reorder, xid, false, NULL, InvalidXLogRecPtr, false);
/*
* Iterate through all toplevel transactions. This can include
@@ -913,6 +916,14 @@ SnapBuildDistributeNewCatalogSnapshot(SnapBuild *builder, XLogRecPtr lsn)
SnapBuildSnapIncRefcount(builder->snapshot);
ReorderBufferAddSnapshot(builder->reorder, txn->xid, lsn,
builder->snapshot);
+
+ /*
+ * Add invalidation messages to the reorder buffer of inprogress
+ * transactions except the current committed transaction
+ */
+ if (txn->xid != xid && curr_txn->ninvalidations > 0)
+ ReorderBufferAddInvalidations(builder->reorder, txn->xid, lsn,
+ curr_txn->ninvalidations, curr_txn->invalidations);
}
}
@@ -1184,8 +1195,11 @@ SnapBuildCommitTxn(SnapBuild *builder, XLogRecPtr lsn, TransactionId xid,
/* refcount of the snapshot builder for the new snapshot */
SnapBuildSnapIncRefcount(builder->snapshot);
- /* add a new catalog snapshot to all currently running transactions */
-
Re: long-standing data loss bug in initial sync of logical replication
On Mon, 9 Sept 2024 at 10:41, Shlok Kyal wrote: > > On Mon, 2 Sept 2024 at 10:12, Amit Kapila wrote: > > > > On Fri, Aug 30, 2024 at 3:06 PM Shlok Kyal wrote: > > > > > > Next I am planning to test solely on the logical decoding side and > > > will share the results. > > > > > > > Thanks, the next set of proposed tests makes sense to me. It will also > > be useful to generate some worst-case scenarios where the number of > > invalidations is more to see the distribution cost in such cases. For > > example, Truncate/Drop a table with 100 or 1000 partitions. > > > > -- > > With Regards, > > Amit Kapila. > > Hi, > > I did some performance testing solely on the logical decoding side and > found some degradation in performance, for the following testcase: > 1. Created a publisher on a single table, say 'tab_conc1'; > 2. Created a second publisher on a single table say 'tp'; > 4. two sessions are running in parallel, let's say S1 and S2. > 5. Begin a transaction in S1. > 6. Now in a loop (this loop runs 'count' times): > S1: Insert a row in table 'tab_conc1' > S2: BEGIN; Alter publication DROP/ ADD tp; COMMIT > 7. COMMIT the transaction in S1. > 8. run 'pg_logical_slot_get_binary_changes' to get the decoding changes. > > Observation: > With fix a new entry is added in decoding. During debugging I found > that this entry only comes when we do a 'INSERT' in Session 1 after we > do 'ALTER PUBLICATION' in another session in parallel (or we can say > due to invalidation). Also, I observed that this new entry is related > to sending replica identity, attributes,etc as function > 'logicalrep_write_rel' is called. > > Performance: > We see a performance degradation as we are sending new entries during > logical decoding. Results are an average of 5 runs. > > count|Head (sec)|Fix (sec)|Degradation (%) > -- > 1 |1.298|1.574 |21.26348228 > 5 |22.892 |24.997 |9.195352088 > 10 |88.602 |93.759 |5.820410374 > > I have also attached the test script here. > For the above case I tried to investigate the inconsistent degradation and found out that Serialization was happening for a large number of 'count'. So, I tried adjusting 'logical_decoding_work_mem' to a large value, so that we can avoid serialization here. I ran the above performance test again and got the following results: count|Head (sec)|Fix (sec) |Degradation (%) --- 1 |0.415446 |0.53596167 | 29.00874482 5 |7.950266 |10.37375567 |30.48312685 75000 |17.192372|22.246715 |29.39875312 10 |30.555903|39.431542 |29.04721552 These results are an average of 3 runs. Here the degradation is consistent around ~30%. Thanks and Regards, Shlok Kyal
Re: long-standing data loss bug in initial sync of logical replication
On Mon, 2 Sept 2024 at 10:12, Amit Kapila wrote: > > On Fri, Aug 30, 2024 at 3:06 PM Shlok Kyal wrote: > > > > Next I am planning to test solely on the logical decoding side and > > will share the results. > > > > Thanks, the next set of proposed tests makes sense to me. It will also > be useful to generate some worst-case scenarios where the number of > invalidations is more to see the distribution cost in such cases. For > example, Truncate/Drop a table with 100 or 1000 partitions. > > -- > With Regards, > Amit Kapila. Also, I did testing with a table with partitions. To test for the scenario where the number of invalidations are more than distribution. Following is the test case: 1. Created a publisher on a single table, say 'tconc_1'; 2. Created a second publisher on a partition table say 'tp'; 3. Created 'tcount' partitions for the table 'tp'. 4. two sessions are running in parallel, let's say S1 and S2. 5. Begin a transaction in S1. 6. S1: Insert a row in table 'tconc_1' S2: BEGIN; TRUNCATE TABLE tp; COMMIT; With patch, this will add 'tcount * 3' invalidation messages to transaction in session 1. S1: Insert a row in table 't_conc1' 7. COMMIT the transaction in S1. 8. run 'pg_logical_slot_get_binary_changes' to get the decoding changes. Performance: We see a degradation in performance. Results are an average of 5 runs. count of partitions | Head (sec) |Fix (sec) |Degradation (%) - 1000 | 0.114 |0.118 |3.50877193 5000 | 0.502 |0.522 |3.984063745 1 | 1.012 |1.024 |1.185770751 I have also attached the test script here. And will also do further testing. Thanks and Regards, Shlok Kyal test3.pl Description: Binary data
Re: long-standing data loss bug in initial sync of logical replication
On Mon, 2 Sept 2024 at 10:12, Amit Kapila wrote: > > On Fri, Aug 30, 2024 at 3:06 PM Shlok Kyal wrote: > > > > Next I am planning to test solely on the logical decoding side and > > will share the results. > > > > Thanks, the next set of proposed tests makes sense to me. It will also > be useful to generate some worst-case scenarios where the number of > invalidations is more to see the distribution cost in such cases. For > example, Truncate/Drop a table with 100 or 1000 partitions. > > -- > With Regards, > Amit Kapila. Hi, I did some performance testing solely on the logical decoding side and found some degradation in performance, for the following testcase: 1. Created a publisher on a single table, say 'tab_conc1'; 2. Created a second publisher on a single table say 'tp'; 4. two sessions are running in parallel, let's say S1 and S2. 5. Begin a transaction in S1. 6. Now in a loop (this loop runs 'count' times): S1: Insert a row in table 'tab_conc1' S2: BEGIN; Alter publication DROP/ ADD tp; COMMIT 7. COMMIT the transaction in S1. 8. run 'pg_logical_slot_get_binary_changes' to get the decoding changes. Observation: With fix a new entry is added in decoding. During debugging I found that this entry only comes when we do a 'INSERT' in Session 1 after we do 'ALTER PUBLICATION' in another session in parallel (or we can say due to invalidation). Also, I observed that this new entry is related to sending replica identity, attributes,etc as function 'logicalrep_write_rel' is called. Performance: We see a performance degradation as we are sending new entries during logical decoding. Results are an average of 5 runs. count|Head (sec)|Fix (sec)|Degradation (%) -- 1 |1.298|1.574 |21.26348228 5 |22.892 |24.997 | 9.195352088 10 |88.602 |93.759 |5.820410374 I have also attached the test script here. Thanks and Regards, Shlok Kyal test2.pl Description: Binary data
Re: long-standing data loss bug in initial sync of logical replication
> BTW, we should do some performance testing by having a mix of DML and > DDLs to see the performance impact of this patch. > > [1] - > https://www.postgresql.org/message-id/CAD21AoAenVqiMjpN-PvGHL1N9DWnHSq673bfgr6phmBUzx=k...@mail.gmail.com > I did some performance testing and I found some performance impact for the following case: 1. Created a publisher, subscriber set up on a single table, say 'tab_conc1'; 2. Created a second publisher, subscriber set on a single table say 'tp'; 3. Created 'tcount' no. of tables. These tables are not part of any publication. 4. There are two sessions running in parallel, let's say S1 and S2. 5. Begin a transaction in S1. 6. Now in a loop (this loop runs 100 times): S1: Insert a row in table 'tab_conc1' S1: Insert a row in all 'tcount' tables. S2: BEGIN; Alter publication for 2nd publication; COMMIT; The current logic in the patch will call the function 'rel_sync_cache_publication_cb' during invalidation. This will invalidate the cache for all the tables. So cache related to all the tables i.e. table 'tab_conc1', 'tcount' tables will be invalidated. 7. COMMIT the transaction in S1. The performance in this case is: No. of tables | With patch (in ms) | With head (in ms) - tcount = 100 | 101376.4 | 101357.8 tcount = 1000| 994085.4 | 993471.4 For 100 tables the performance is slow by '0.018%' and for 1000 tables performance is slow by '0.06%'. These results are the average of 5 runs. Other than this I tested the following cases but did not find any performance impact: 1. with 'tcount = 10'. But I didn't find any performance impact. 2. with 'tcount = 0' and running the loop 1000 times. But I didn't find any performance impact. I have also attached the test script and the machine configurations on which performance testing was done. Next I am planning to test solely on the logical decoding side and will share the results. Thanks and Regards, Shlok Kyal NAME="Red Hat Enterprise Linux Server" VERSION="7.9 (Maipo)" ID="rhel" ID_LIKE="fedora" VARIANT="Server" VARIANT_ID="server" VERSION_ID="7.9" PRETTY_NAME="Red Hat Enterprise Linux Server 7.9 (Maipo)" ANSI_COLOR="0;31" CPE_NAME="cpe:/o:redhat:enterprise_linux:7.9:GA:server" HOME_URL="https://www.redhat.com/"; BUG_REPORT_URL="https://bugzilla.redhat.com/"; REDHAT_BUGZILLA_PRODUCT="Red Hat Enterprise Linux 7" REDHAT_BUGZILLA_PRODUCT_VERSION=7.9 REDHAT_SUPPORT_PRODUCT="Red Hat Enterprise Linux" REDHAT_SUPPORT_PRODUCT_VERSION="7.9" CPU INFO processor : 119 vendor_id : GenuineIntel cpu family : 6 model : 62 model name : Intel(R) Xeon(R) CPU E7-4890 v2 @ 2.80GHz stepping: 7 microcode : 0x715 cpu MHz : 1505.957 cache size : 38400 KB physical id : 3 siblings: 30 core id : 14 cpu cores : 15 apicid : 125 initial apicid : 125 fpu : yes fpu_exception : yes cpuid level : 13 wp : yes flags : fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca cmov pat pse36 clflush dts acpi mmx fxsr sse sse2 ss ht tm pbe syscall nx pdpe1gb rdtscp lm constant_tsc arch_perfmon pebs bts rep_good nopl xtopology nonstop_tsc aperfmperf eagerfpu pni pclmulqdq dtes64 monitor ds_cpl vmx smx est tm2 ssse3 cx16 xtpr pdcm pcid dca sse4_1 sse4_2 x2apic popcnt tsc_deadline_timer aes xsave avx f16c rdrand lahf_lm epb intel_ppin ssbd ibrs ibpb stibp tpr_shadow vnmi flexpriority ept vpid fsgsbase smep erms xsaveopt dtherm ida arat pln pts md_clear spec_ctrl intel_stibp flush_l1d bogomips: 5629.54 clflush size: 64 cache_alignment : 64 address sizes : 46 bits physical, 48 bits virtual power management: MemTotal: 792237404 kB MemFree:724051992 kB MemAvailable: 762505368 kB Buffers:2108 kB Cached: 43885588 kB SwapCached:0 kB Active: 22276460 kB Inactive: 21761812 kB Active(anon):1199380 kB Inactive(anon): 4228212 kB Active(file): 21077080 kB Inactive(file): 17533600 kB Unevictable: 0 kB Mlocked: 0 kB SwapTotal: 4194300 kB SwapFree:4194300 kB Dirty: 0 kB Writeback: 0 kB AnonPages:150796 kB Mapped: 5283248 kB Shmem: 5277044 kB Slab:1472084 kB SReclaimable:1165144 kB SUnreclaim: 306940 kB KernelStack: 17504 kB PageTables:21540 kB NFS_Unstable: 0 kB Bounce:0 kB WritebackTmp: 0 kB CommitLimit:400313000 kB Committed_AS: 86287072 kB V
Re: Fix memory counter update in reorderbuffer
On Wed, 7 Aug 2024 at 11:48, Amit Kapila wrote: > > On Wed, Aug 7, 2024 at 7:42 AM Masahiko Sawada wrote: > > > > On Tue, Aug 6, 2024 at 1:12 PM Amit Kapila wrote: > > > > > > On Sat, Aug 3, 2024 at 1:21 AM Masahiko Sawada > > > wrote: > > > > > > > > I found a bug in the memory counter update in reorderbuffer. It was > > > > introduced by commit 5bec1d6bc5e, so pg17 and master are affected. > > > > > > > > In ReorderBufferCleanupTXN() we zero the transaction size and then > > > > free the transaction entry as follows: > > > > > > > > /* Update the memory counter */ > > > > ReorderBufferChangeMemoryUpdate(rb, NULL, txn, false, txn->size); > > > > > > > > /* deallocate */ > > > > ReorderBufferReturnTXN(rb, txn); > > > > > > > > > > Why do we need to zero the transaction size explicitly? Shouldn't it > > > automatically become zero after freeing all the changes? > > > > It will become zero after freeing all the changes. However, since > > updating the max-heap when freeing each change could bring some > > overhead, we freed the changes without updating the memory counter, > > and then zerod it. > > > > I think this should be covered in comments as it is not apparent. > > > > > > BTW, commit 5bec1d6bc5e also introduced > > > ReorderBufferChangeMemoryUpdate() in ReorderBufferTruncateTXN() which > > > is also worth considering while fixing the reported problem. It may > > > not have the same problem as you have reported but we can consider > > > whether setting txn size as zero explicitly is required or not. > > > > The reason why it introduced ReorderBufferChangeMemoryUpdate() is the > > same as I mentioned above. And yes, as you mentioned, it doesn't have > > the same problem that I reported here. > > > > I checked again and found that ReorderBufferResetTXN() first calls > ReorderBufferTruncateTXN() and then ReorderBufferToastReset(). After > that, it also tries to free spec_insert change which should have the > same problem. So, what saves this path from the same problem? I tried testing this scenario and I was able to reproduce the crash in HEAD with this scenario. I have created a patch for the testcase. I also tested the same scenario with the latest patch shared by Sawada-san in [1]. And confirm that this resolves the issue. [1]: https://www.postgresql.org/message-id/CAD21AoDHC4Sob%3DNEYTxgu5wd4rzCpSLY_hWapMUqf4WKrAxmyw%40mail.gmail.com Thanks and Regards, Shlok Kyal fix_memory_counter_update_in_reorderbuffer_v2.patch Description: Binary data test.patch Description: Binary data
Re: long-standing data loss bug in initial sync of logical replication
On Thu, 8 Aug 2024 at 16:24, Shlok Kyal wrote: > > On Wed, 31 Jul 2024 at 11:17, Shlok Kyal wrote: > > > > On Wed, 31 Jul 2024 at 09:36, Amit Kapila wrote: > > > > > > On Wed, Jul 31, 2024 at 3:27 AM Masahiko Sawada > > > wrote: > > > > > > > > On Wed, Jul 24, 2024 at 9:53 PM Amit Kapila > > > > wrote: > > > > > > > > > > On Wed, Jul 17, 2024 at 5:25 PM vignesh C wrote: > > > > > > > > > > > > On Wed, 17 Jul 2024 at 11:54, Amit Kapila > > > > > > wrote: > > > > > > > > > > > > > > On Tue, Jul 16, 2024 at 6:54 PM vignesh C > > > > > > > wrote: > > > > > > > > > > > > > > BTW, I noticed that we don't take any table-level locks for Create > > > > > > > Publication .. For ALL TABLES (and Drop Publication). Can that > > > > > > > create > > > > > > > a similar problem? I haven't tested so not sure but even if there > > > > > > > is a > > > > > > > problem for the Create case, it should lead to some ERROR like > > > > > > > missing > > > > > > > publication. > > > > > > > > > > > > I tested these scenarios, and as you expected, it throws an error > > > > > > for > > > > > > the create publication case: > > > > > > 2024-07-17 14:50:01.145 IST [481526] 481526 ERROR: could not > > > > > > receive > > > > > > data from WAL stream: ERROR: publication "pub1" does not exist > > > > > > CONTEXT: slot "sub1", output plugin "pgoutput", in the > > > > > > change > > > > > > callback, associated LSN 0/1510CD8 > > > > > > 2024-07-17 14:50:01.147 IST [481450] 481450 LOG: background worker > > > > > > "logical replication apply worker" (PID 481526) exited with exit > > > > > > code > > > > > > 1 > > > > > > > > > > > > The steps for this process are as follows: > > > > > > 1) Create tables in both the publisher and subscriber. > > > > > > 2) On the publisher: Create a replication slot. > > > > > > 3) On the subscriber: Create a subscription using the slot created > > > > > > by > > > > > > the publisher. > > > > > > 4) On the publisher: > > > > > > 4.a) Session 1: BEGIN; INSERT INTO T1; > > > > > > 4.b) Session 2: CREATE PUBLICATION FOR ALL TABLES > > > > > > 4.c) Session 1: COMMIT; > > > > > > > > > > > > Since we are throwing out a "publication does not exist" error, > > > > > > there > > > > > > is no inconsistency issue here. > > > > > > > > > > > > However, an issue persists with DROP ALL TABLES publication, where > > > > > > data continues to replicate even after the publication is dropped. > > > > > > This happens because the open transaction consumes the invalidation, > > > > > > causing the publications to be revalidated using old snapshot. As a > > > > > > result, both the open transactions and the subsequent transactions > > > > > > are > > > > > > getting replicated. > > > > > > > > > > > > We can reproduce this issue by following these steps in a logical > > > > > > replication setup with an "ALL TABLES" publication: > > > > > > On the publisher: > > > > > > Session 1: BEGIN; INSERT INTO T1 VALUES (val1); > > > > > > In another session on the publisher: > > > > > > Session 2: DROP PUBLICATION > > > > > > Back in Session 1 on the publisher: > > > > > > COMMIT; > > > > > > Finally, in Session 1 on the publisher: > > > > > > INSERT INTO T1 VALUES (val2); > > > > > > > > > > > > Even after dropping the publication, both val1 and val2 are still > > > > > > being replicated to the subscriber. This means that both the > > > > > > in-progress concurrent transaction and the subsequent transactions > > > > > > are > > > > > > being replicated. > > &g
Re: long-standing data loss bug in initial sync of logical replication
On Wed, 31 Jul 2024 at 11:17, Shlok Kyal wrote: > > On Wed, 31 Jul 2024 at 09:36, Amit Kapila wrote: > > > > On Wed, Jul 31, 2024 at 3:27 AM Masahiko Sawada > > wrote: > > > > > > On Wed, Jul 24, 2024 at 9:53 PM Amit Kapila > > > wrote: > > > > > > > > On Wed, Jul 17, 2024 at 5:25 PM vignesh C wrote: > > > > > > > > > > On Wed, 17 Jul 2024 at 11:54, Amit Kapila > > > > > wrote: > > > > > > > > > > > > On Tue, Jul 16, 2024 at 6:54 PM vignesh C > > > > > > wrote: > > > > > > > > > > > > BTW, I noticed that we don't take any table-level locks for Create > > > > > > Publication .. For ALL TABLES (and Drop Publication). Can that > > > > > > create > > > > > > a similar problem? I haven't tested so not sure but even if there > > > > > > is a > > > > > > problem for the Create case, it should lead to some ERROR like > > > > > > missing > > > > > > publication. > > > > > > > > > > I tested these scenarios, and as you expected, it throws an error for > > > > > the create publication case: > > > > > 2024-07-17 14:50:01.145 IST [481526] 481526 ERROR: could not receive > > > > > data from WAL stream: ERROR: publication "pub1" does not exist > > > > > CONTEXT: slot "sub1", output plugin "pgoutput", in the change > > > > > callback, associated LSN 0/1510CD8 > > > > > 2024-07-17 14:50:01.147 IST [481450] 481450 LOG: background worker > > > > > "logical replication apply worker" (PID 481526) exited with exit code > > > > > 1 > > > > > > > > > > The steps for this process are as follows: > > > > > 1) Create tables in both the publisher and subscriber. > > > > > 2) On the publisher: Create a replication slot. > > > > > 3) On the subscriber: Create a subscription using the slot created by > > > > > the publisher. > > > > > 4) On the publisher: > > > > > 4.a) Session 1: BEGIN; INSERT INTO T1; > > > > > 4.b) Session 2: CREATE PUBLICATION FOR ALL TABLES > > > > > 4.c) Session 1: COMMIT; > > > > > > > > > > Since we are throwing out a "publication does not exist" error, there > > > > > is no inconsistency issue here. > > > > > > > > > > However, an issue persists with DROP ALL TABLES publication, where > > > > > data continues to replicate even after the publication is dropped. > > > > > This happens because the open transaction consumes the invalidation, > > > > > causing the publications to be revalidated using old snapshot. As a > > > > > result, both the open transactions and the subsequent transactions are > > > > > getting replicated. > > > > > > > > > > We can reproduce this issue by following these steps in a logical > > > > > replication setup with an "ALL TABLES" publication: > > > > > On the publisher: > > > > > Session 1: BEGIN; INSERT INTO T1 VALUES (val1); > > > > > In another session on the publisher: > > > > > Session 2: DROP PUBLICATION > > > > > Back in Session 1 on the publisher: > > > > > COMMIT; > > > > > Finally, in Session 1 on the publisher: > > > > > INSERT INTO T1 VALUES (val2); > > > > > > > > > > Even after dropping the publication, both val1 and val2 are still > > > > > being replicated to the subscriber. This means that both the > > > > > in-progress concurrent transaction and the subsequent transactions are > > > > > being replicated. > > > > > > > > > > I don't think locking all tables is a viable solution in this case, as > > > > > it would require asking the user to refrain from performing any > > > > > operations on any of the tables in the database while creating a > > > > > publication. > > > > > > > > > > > > > Indeed, locking all tables in the database to prevent concurrent DMLs > > > > for this scenario also looks odd to me. The other alternative > > > > previously suggested by Andres is to distribute catalog modifying > > > > trans
Re: long-standing data loss bug in initial sync of logical replication
> > > invalidations corresponding to DDLs for the already in-progress > > > transactions. We discussed preventing DMLs in the first place when > > > concurrent DDLs like ALTER PUBLICATION ... ADD TABLE ... are in > > > progress. The solution discussed was to acquire > > > ShareUpdateExclusiveLock for all the tables being added via such > > > commands. Further analysis revealed that the same handling is required > > > for ALTER PUBLICATION ... ADD TABLES IN SCHEMA which means locking all > > > the tables in the specified schemas. Then DROP PUBLICATION also seems > > > to have similar symptoms which means in the worst case (where > > > publication is for ALL TABLES) we have to lock all the tables in the > > > database. We are not sure if that is good so the other alternative we > > > can pursue is to distribute invalidations in logical decoding > > > infrastructure [1] which has its downsides. > > > > > > Thoughts? > > > > Thank you for summarizing the problem and solutions! > > > > I think it's worth trying the idea of distributing invalidation > > messages, and we will see if there could be overheads or any further > > obstacles. IIUC this approach would resolve another issue we discussed > > before too[1]. > > > > Yes, and we also discussed having a similar solution at the time when > that problem was reported. So, it is clear that even though locking > tables can work for commands alter ALTER PUBLICATION ... ADD TABLE > ..., we need a solution for distributing invalidations to the > in-progress transactions during logical decoding for other cases as > reported by you previously. > > Thanks for looking into this. > Thanks, I am working on to implement a solution for distributing invalidations. Will share a patch for the same. Thanks and Regards, Shlok Kyal
Re: Pgoutput not capturing the generated columns
On Fri, 19 Jul 2024 at 04:59, Peter Smith wrote: > > Hi, here are some review comments for patch v19-0003 > > == > src/backend/catalog/pg_publication.c > > 1. > /* > * Translate a list of column names to an array of attribute numbers > * and a Bitmapset with them; verify that each attribute is appropriate > * to have in a publication column list (no system or generated attributes, > * no duplicates). Additional checks with replica identity are done later; > * see pub_collist_contains_invalid_column. > * > * Note that the attribute numbers are *not* offset by > * FirstLowInvalidHeapAttributeNumber; system columns are forbidden so this > * is okay. > */ > static void > publication_translate_columns(Relation targetrel, List *columns, > int *natts, AttrNumber **attrs) > > ~ > > I though the above comment ought to change: /or generated > attributes/or virtual generated attributes/ > > IIRC this was already addressed back in v16, but somehow that fix has > been lost (???). Modified the comment > == > src/backend/replication/logical/tablesync.c > > fetch_remote_table_info: > nitpick - missing end space in this comment /* TODO: use > ATTRIBUTE_GENERATED_VIRTUAL*/ > Fixed > == > > 2. > (in patch v19-0001) > +# tab3: > +# publisher-side tab3 has generated col 'b'. > +# subscriber-side tab3 has generated col 'b', using a different computation. > > (here, in patch v19-0003) > # tab3: > -# publisher-side tab3 has generated col 'b'. > -# subscriber-side tab3 has generated col 'b', using a different computation. > +# publisher-side tab3 has stored generated col 'b' but > +# subscriber-side tab3 has DIFFERENT COMPUTATION stored generated col 'b'. > > It has become difficult to review these TAP tests, particularly when > different patches are modifying the same comment. e.g. I post > suggestions to modify comments for patch 0001. Those get addressed OK, > only to vanish in subsequent patches like has happened in the above > example. > > Really this patch 0003 was only supposed to add the word "stored", not > revert the entire comment to something from an earlier version. Please > take care that all comment changes are carried forward correctly from > one patch to the next. Fixed I have addressed the comment in v20-0003 patch. Please refer [1]. [1]: https://www.postgresql.org/message-id/CANhcyEUzUurrX38HGvG30gV92YDz6WmnnwNRYMVY4tiga-8KZg%40mail.gmail.com Thanks and Regards, Shlok Kyal
Re: Pgoutput not capturing the generated columns
On Mon, 15 Jul 2024 at 08:08, Peter Smith wrote: > > Hi, here are some review comments about patch v17-0003 I have addressed the comments in v18-0003 patch [1]. [1]: https://www.postgresql.org/message-id/CANhcyEW3LVJpRPScz6VBa%3DZipEMV7b-u76PDEALNcNDFURCYMA%40mail.gmail.com Thanks and Regards, Shlok Kyal
Re: Pgoutput not capturing the generated columns
On Tue, 9 Jul 2024 at 09:53, Peter Smith wrote:
>
> Hi Shlok, here are my review comments for v16-0003.
>
> ==
> src/backend/replication/logical/proto.c
>
>
> On Mon, Jul 8, 2024 at 10:04 PM Shlok Kyal wrote:
> >
> > On Mon, 8 Jul 2024 at 13:20, Peter Smith wrote:
> > >
> > >
> > > 2. logicalrep_write_tuple and logicalrep_write_attrs
> > >
> > > I thought all the code fragments like this:
> > >
> > > + if (att->attgenerated && att->attgenerated !=
> > > ATTRIBUTE_GENERATED_STORED)
> > > + continue;
> > > +
> > >
> > > don't need to be in the code anymore, because of the BitMapSet (BMS)
> > > processing done to make the "column list" for publication where
> > > disallowed generated cols should already be excluded from the BMS,
> > > right?
> > >
> > > So shouldn't all these be detected by the following statement:
> > > if (!column_in_column_list(att->attnum, columns))
> > > continue;
> > The current BMS logic do not handle the Virtual Generated Columns.
> > There can be cases where we do not want a virtual generated column but
> > it would be present in BMS.
> > To address this I have added the above logic. I have added this logic
> > similar to the checks of 'attr->attisdropped'.
> >
>
> Hmm. I thought the BMS idea of patch 0001 is to discover what columns
> should be replicated up-front. If they should not be replicated (e.g.
> virtual generated columns cannot be) then they should never be in the
> BMS.
>
> So what you said ("There can be cases where we do not want a virtual
> generated column but it would be present in BMS") should not be
> happening. If that is happening then it sounds more like a bug in the
> new BMS logic of pgoutput_column_list_init() function. In other words,
> if what you say is true, then it seems like the current extra
> conditions you have in patch 0004 are just a band-aid to cover a
> problem of the BMS logic of patch 0001. Am I mistaken?
>
We have created a 0004 patch to use the BMS approach. It will be
addressed in the future 0004 patch.
> > > ==
> > > src/backend/replication/pgoutput/pgoutput.c
> > >
> > > 4. send_relation_and_attrs
> > >
> > > (this is a similar comment for #2 above)
> > >
> > > IIUC of the advantages of the BitMapSet (BMS) idea in patch 0001 to
> > > process the generated columns up-front means there is no need to check
> > > them again in code like this.
> > >
> > > They should be discovered anyway in the subsequent check:
> > > /* Skip this attribute if it's not present in the column list */
> > > if (columns != NULL && !bms_is_member(att->attnum, columns))
> > > continue;
> > Same explanation as above.
>
> As above.
>
We have created a 0004 patch to use the BMS approach. It will be
addressed in the future 0004 patch.
> ==
> src/test/subscription/t/011_generated.pl
>
> I'm not sure if you needed to say "STORED" generated cols for the
> subscriber-side columns but anyway, whatever is done needs to be done
> consistently. FYI, below you did *not* say STORED for subscriber-side
> generated cols, but in other comments for subscriber-side generated
> columns, you did say STORED.
>
> # tab3:
> # publisher-side tab3 has STORED generated col 'b' but
> # subscriber-side tab3 has DIFFERENT COMPUTATION generated col 'b'.
>
> ~
>
> # tab4:
> # publisher-side tab4 has STORED generated cols 'b' and 'c' but
> # subscriber-side tab4 has non-generated col 'b', and generated-col 'c'
> # where columns on publisher/subscriber are in a different order
>
Fixed
Please find the updated patch v18-0003 patch at [1].
[1]:
https://www.postgresql.org/message-id/CANhcyEW3LVJpRPScz6VBa%3DZipEMV7b-u76PDEALNcNDFURCYMA%40mail.gmail.com
Thanks and Regards,
Shok Kyal
Re: Pgoutput not capturing the generated columns
On Mon, 8 Jul 2024 at 13:20, Peter Smith wrote: > > Hi Shlok, Here are some review comments for patch v15-0003. > > == > src/backend/catalog/pg_publication.c > > 1. publication_translate_columns > > The function comment says: > * Translate a list of column names to an array of attribute numbers > * and a Bitmapset with them; verify that each attribute is appropriate > * to have in a publication column list (no system or generated attributes, > * no duplicates). Additional checks with replica identity are done later; > * see pub_collist_contains_invalid_column. > > That part about "[no] generated attributes" seems to have gone stale > -- e.g. not quite correct anymore. Should it say no VIRTUAL generated > attributes? Yes, we should use VIRTUAL generated attributes, I have modified it. > == > src/backend/replication/logical/proto.c > > 2. logicalrep_write_tuple and logicalrep_write_attrs > > I thought all the code fragments like this: > > + if (att->attgenerated && att->attgenerated != ATTRIBUTE_GENERATED_STORED) > + continue; > + > > don't need to be in the code anymore, because of the BitMapSet (BMS) > processing done to make the "column list" for publication where > disallowed generated cols should already be excluded from the BMS, > right? > > So shouldn't all these be detected by the following statement: > if (!column_in_column_list(att->attnum, columns)) > continue; The current BMS logic do not handle the Virtual Generated Columns. There can be cases where we do not want a virtual generated column but it would be present in BMS. To address this I have added the above logic. I have added this logic similar to the checks of 'attr->attisdropped'. > == > src/backend/replication/pgoutput/pgoutput.c > > 4. send_relation_and_attrs > > (this is a similar comment for #2 above) > > IIUC of the advantages of the BitMapSet (BMS) idea in patch 0001 to > process the generated columns up-front means there is no need to check > them again in code like this. > > They should be discovered anyway in the subsequent check: > /* Skip this attribute if it's not present in the column list */ > if (columns != NULL && !bms_is_member(att->attnum, columns)) > continue; Same explanation as above. I have addressed all the comments in v16-0003 patch. Please refer [1]. [1]: https://www.postgresql.org/message-id/CANhcyEXw%3DBFFVUqohWES9EPkdq-ZMC5QRBVQqQPzrO%3DQ7uzFQw%40mail.gmail.com Thanks and Regards, Shlok Kyal
Re: Pgoutput not capturing the generated columns
On Wed, 26 Jun 2024 at 08:06, Peter Smith wrote:
>
> Hi Shlok. Here are my review comments for patch v10-0003
>
> ==
> General.
>
> 1.
> The patch has lots of conditions like:
> if (att->attgenerated && (att->attgenerated !=
> ATTRIBUTE_GENERATED_STORED || !include_generated_columns))
> continue;
>
> IMO these are hard to read. Although more verbose, please consider if
> all those (for the sake of readability) would be better re-written
> like below :
>
> if (att->generated)
> {
> if (!include_generated_columns)
> continue;
>
> if (att->attgenerated != ATTRIBUTE_GENERATED_STORED)
> continue;
> }
Fixed
> ==
> contrib/test_decoding/test_decoding.c
>
> tuple_to_stringinfo:
>
> NITPICK = refactored the code and comments a bit here to make it easier
> NITPICK - there is no need to mention "virtual". Instead, say we only
> support STORED
Fixed
> ==
> src/backend/catalog/pg_publication.c
>
> publication_translate_columns:
>
> NITPICK - introduced variable 'att' to simplify this code
Fixed
> ~
>
> 2.
> + ereport(ERROR,
> + errcode(ERRCODE_INVALID_COLUMN_REFERENCE),
> + errmsg("cannot use virtual generated column \"%s\" in publication
> column list",
> +colname));
>
> Is it better to avoid referring to "virtual" at all? Instead, consider
> rearranging the wording to say something like "generated column \"%s\"
> is not STORED so cannot be used in a publication column list"
Fixed
> ~~~
>
> pg_get_publication_tables:
>
> NITPICK - split the condition code for readability
Fixed
> ==
> src/backend/replication/logical/relation.c
>
> 3. logicalrep_rel_open
>
> + if (attr->attgenerated && attr->attgenerated != ATTRIBUTE_GENERATED_STORED)
> + continue;
> +
>
> Isn't this missing some code to say "entry->attrmap->attnums[i] =
> -1;", same as all the other nearby code is doing?
Fixed
> ~~~
>
> 4.
> I felt all the "generated column" logic should be kept together, so
> this new condition should be combined with the other generated column
> condition, like:
>
> if (attr->attgenerated)
> {
> /* comment... */
> if (!MySubscription->includegencols)
> {
> entry->attrmap->attnums[i] = -1;
> continue;
> }
>
> /* comment... */
> if (attr->attgenerated != ATTRIBUTE_GENERATED_STORED)
> {
> entry->attrmap->attnums[i] = -1;
> continue;
> }
> }
Fixed
> ==
> src/backend/replication/logical/tablesync.c
>
> 5.
> + if (gencols_allowed)
> + {
> + /* Replication of generated cols is supported, but not VIRTUAL cols. */
> + appendStringInfo(&cmd, " AND a.attgenerated != 'v'");
> + }
>
> Is it better here to use the ATTRIBUTE_GENERATED_VIRTUAL macro instead
> of the hardwired 'v'? (Maybe add another TODO comment to revisit
> this).
>
> Alternatively, consider if it is more future-proof to rearrange so it
> just says what *is* supported instead of what *isn't* supported:
> e.g. "AND a.attgenerated IN ('', 's')"
I feel we should use ATTRIBUTE_GENERATED_VIRTUAL macro. Added a TODO.
> ==
> src/test/subscription/t/011_generated.pl
>
> NITPICK - some comments are missing the word "stored"
> NITPICK - sometimes "col" should be plural "cols"
> NITPICK = typo "GNERATED"
Add the relevant changes.
> ==
>
> 6.
> In a previous review [1, comment #3] I mentioned that there should be
> some docs updates on the "Logical Replication Message Formats" section
> 53.9. So, I expected patch 0001 would make some changes and then patch
> 0003 would have to update it again to say something about "STORED".
> But all that is missing from the v10* patches.
>
> ==
Will fix in upcoming version
>
> 99.
> See also my nitpicks diff which is a top-up patch addressing all the
> nitpick comments mentioned above. Please apply all of these that you
> agree with.
Applied Relevant changes
Please refer v14 patch for the changes [1].
[1]:
https://www.postgresql.org/message-id/CANhcyEW95M_usF1OJDudeejs0L0%2BYOEb%3DdXmt_4Hs-70%3DCXa-g%40mail.gmail.com
Thanks and Regards,
Shlok Kyal
Re: Pgoutput not capturing the generated columns
On Tue, 25 Jun 2024 at 18:49, Hayato Kuroda (Fujitsu)
wrote:
>
> Dear Shlok,
>
> Thanks for updating patches! Below are my comments, maybe only for 0002.
>
> 01. General
>
> IIUC, we are not discussed why ALTER SUBSCRIPTION ... SET
> include_generated_columns
> is prohibit. Previously, it seems okay because there are exclusive options.
> But now,
> such restrictions are gone. Do you have a reason in your mind? It is just not
> considered
> yet?
We donot support ALTER SUBSCRIPTION to alter
'include_generated_columns'. Suppose initially the user has a logical
replication setup. Publisher has
table t1 with columns (c1 int, c2 int generated always as (c1*2)) and
subscriber has table t1 with columns (c1 int, c2 int). And initially
'incude_generated_column' is true.
Now if we 'ALTER SUBSCRIPTION' to set 'include_generated_columns' as
false. Initial rows will have data for c2 on the subscriber table, but
will not have value after alter. This may be an inconsistent
behaviour.
> 02. General
>
> According to the doc, we allow to alter a column to non-generated one, by
> ALTER
> TABLE ... ALTER COLUMN ... DROP EXPRESSION command. Not sure, what should be
> when the command is executed on the subscriber while copying the data? Should
> we continue the copy or restart? How do you think?
COPY of data will happen in a single transaction, so if we execute
'ALTER TABLE ... ALTER COLUMN ... DROP EXPRESSION' command, It will
take place after the whole COPY command will finish. So I think it
will not create any issue.
> 03. Tes tcode
>
> IIUC, REFRESH PUBLICATION can also lead the table synchronization. Can you add
> a test for that?
Added
> 04. Test code (maybe for 0001)
>
> Please test the combination with TABLE ... ALTER COLUMN ... DROP EXPRESSION
> command.
Added
> 05. logicalrep_rel_open
>
> ```
> +/*
> + * In case 'include_generated_columns' is 'false', we should
> skip the
> + * check of missing attrs for generated columns.
> + * In case 'include_generated_columns' is 'true', we should
> check if
> + * corresponding column for the generated column in publication
> column
> + * list is present in the subscription table.
> + */
> +if (!MySubscription->includegencols && attr->attgenerated)
> +{
> +entry->attrmap->attnums[i] = -1;
> +continue;
> +}
> ```
>
> This comment is not very clear to me, because here we do not skip anything.
> Can you clarify the reason why attnums[i] is set to -1 and how will it be
> used?
This part of the code is removed to address some comments.
> 06. make_copy_attnamelist
>
> ```
> +gencollist = palloc0(MaxTupleAttributeNumber * sizeof(bool));
> ```
>
> I think this array is too large. Can we reduce a size to (desc->natts *
> sizeof(bool))?
> Also, the free'ing should be done.
I have changed the name 'gencollist' to 'localgenlist' to make the
name more consistent. Also
size should be (rel->remoterel.natts * sizeof(bool)) as I am storing
if a column is generated like 'localgenlist[attnum] = true;'
where 'attnum' is corresponding attribute number on publisher side.
> 07. make_copy_attnamelist
>
> ```
> +/* Loop to handle subscription table generated columns. */
> +for (int i = 0; i < desc->natts; i++)
> ```
>
> IIUC, the loop is needed to find generated columns on the subscriber side,
> right?
> Can you clarify as comment?
Fixed
> 08. copy_table
>
> ```
> +/*
> + * Regular table with no row filter and 'include_generated_columns'
> + * specified as 'false' during creation of subscription.
> + */
> ```
>
> I think this comment is not correct. After patching, all tablesync command
> becomes
> like COPY (SELECT ...) if include_genereted_columns is set to true. Is it
> right?
> Can we restrict only when the table has generated ones?
Fixed
Please refer to v14 patch for the changes [1].
[1]:
https://www.postgresql.org/message-id/CANhcyEW95M_usF1OJDudeejs0L0%2BYOEb%3DdXmt_4Hs-70%3DCXa-g%40mail.gmail.com
Thanks and Regards,
Shlok Kyal
Re: pg_createsubscriber: drop pre-existing subscriptions from the converted node
On Mon, 1 Jul 2024 at 11:44, Hayato Kuroda (Fujitsu) wrote: > > Dear Amit, > > Thanks for giving comments! PSA new version. > > > Thanks, this is a better approach. I have changed a few comments and > > made some other cosmetic changes. See attached. > > I checked your attached and LGTM. Based on that, I added some changes > like below: > > - Made dbname be escaped while listing up pre-existing subscriptions > Previous version could not pass tests by recent commits. > - Skipped dropping subscriptions in dry_run mode > I found the issue while poring the test to 040_pg_createsubscriber.pl. > - Added info-level output to follow other drop_XXX functions > > > BTW, why have you created a separate test file for this test? I think > > we should add a new test to one of the existing tests in > > 040_pg_createsubscriber. > > I was separated a test file for just confirmation purpose, I've planned to > merge. > New patch set did that. > > > You can create a dummy subscription on node_p > > and do a test similar to what we are doing in "# Create failover slot > > to test its removal". > > Your approach looks better than mine. I followed the approach. Hi Kuroda-san, I tested the patches on linux and windows and I confirm that it successfully fixes the issue [1]. [1]: https://www.postgresql.org/message-id/CANhcyEWvimA1-f6hSrA%3D9qkfR5SonFb56b36M%2B%2BvT%3DLiFj%3D76g%40mail.gmail.com Thanks and Regards, Shlok Kyal
Re: Logical Replication of sequences
nc worker for subscription
16389 sync 0 (SequencesyncWorkerMain+0x33)[0x5b2a61115fe7]
postgres: logical replication sequencesync worker for subscription
16389 sync 0 (BackgroundWorkerMain+0x4ad)[0x5b2a61029cae]
postgres: logical replication sequencesync worker for subscription
16389 sync 0 (postmaster_child_launch+0x236)[0x5b2a6102fb36]
postgres: logical replication sequencesync worker for subscription
16389 sync 0 (+0x1f1d12a)[0x5b2a6103b12a]
postgres: logical replication sequencesync worker for subscription
16389 sync 0 (+0x1f1df0f)[0x5b2a6103bf0f]
postgres: logical replication sequencesync worker for subscription
16389 sync 0 (+0x1f1bf71)[0x5b2a61039f71]
postgres: logical replication sequencesync worker for subscription
16389 sync 0 (+0x1f16f73)[0x5b2a61034f73]
postgres: logical replication sequencesync worker for subscription
16389 sync 0 (PostmasterMain+0x18fb)[0x5b2a61034445]
postgres: logical replication sequencesync worker for subscription
16389 sync 0 (+0x1ab1ab8)[0x5b2a60bcfab8]
/lib/x86_64-linux-gnu/libc.so.6(+0x29d90)[0x7b76bc629d90]
/lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0x80)[0x7b76bc629e40]
postgres: logical replication sequencesync worker for subscription
16389 sync 0 (_start+0x25)[0x5b2a601491a5]
Analysis:
Suppose there are two sequences (s1, s2) on publisher.
SO, during initial sync.
in loop,
+ foreach(lc, table_states_not_ready)
table_states_not_ready -> it contains both s1 and s2.
So, for s1 a sequence sync will be started. It will sync all sequences
and the sequence sync worker will exit.
Now, for s2 again a sequence sync will start. It will give the above error.
Is this loop required? Instead we can just use a bool like
'is_any_sequence_not_ready'. Thoughts?
= sequencesync.c =
2. function name should be 'LogicalRepSyncSequences' instead of
'LogicalRepSyncSeqeunces'
3. In function 'LogicalRepSyncSeqeunces'
sequencerel = table_open(seqinfo->relid, RowExclusiveLock);\
There is a extra '\' symbol
4. In function LogicalRepSyncSeqeunces:
+ ereport(LOG,
+ errmsg("logical replication synchronization for subscription \"%s\",
sequence \"%s\" has finished",
+ get_subscription_name(subid, false), RelationGetRelationName(sequencerel)));
+ table_close(sequencerel, NoLock);
+
+ currseq++;
+
+ if (currseq % MAX_SEQUENCES_SYNC_PER_BATCH == 0 || currseq ==
list_length(sequences))
+ CommitTransactionCommand();
The above message gets logged even if the changes are not committed.
Suppose the sequence worker exits before commit due to some reason.
Thought the log will show that sequence is synced, the sequence will
be in 'init' state. I think this is not desirable.
Maybe we should log the synced sequences at commit time? Thoughts?
= General
5. We can use other macros like 'foreach_ptr' instead of 'foreach'
Thanks and Regards,
Shlok Kyal
Re: Pgoutput not capturing the generated columns
On Fri, 21 Jun 2024 at 12:51, Peter Smith wrote:
>
> Hi, Here are some review comments for patch v9-0003
>
> ==
> Commit Message
>
> /fix/fixes/
Fixed
> ==
> 1.
> General. Is tablesync enough?
>
> I don't understand why is the patch only concerned about tablesync?
> Does it make sense to prevent VIRTUAL column replication during
> tablesync, if you aren't also going to prevent VIRTUAL columns from
> normal logical replication (e.g. when copy_data = false)? Or is this
> already handled somewhere?
I checked the behaviour during incremental changes. I saw during
decoding 'null' values are present for Virtual Generated Columns. I
made the relevant changes to not support replication of Virtual
generated columns.
> ~~~
>
> 2.
> General. Missing test.
>
> Add some test cases to verify behaviour is different for STORED versus
> VIRTUAL generated columns
I have not added the tests as it would give an error in cfbot.
I have added a TODO note for the same. This can be done once the
VIRTUAL generated columns patch is committted.
> ==
> src/sgml/ref/create_subscription.sgml
>
> NITPICK - consider rearranging as shown in my nitpicks diff
> NITPICK - use sgml markup for STORED
Fixed
> ==
> src/backend/replication/logical/tablesync.c
>
> 3.
> - if ((walrcv_server_version(LogRepWorkerWalRcvConn) >= 12 &&
> - walrcv_server_version(LogRepWorkerWalRcvConn) <= 16) ||
> - !MySubscription->includegencols)
> + if (walrcv_server_version(LogRepWorkerWalRcvConn) < 17)
> + {
> + if (walrcv_server_version(LogRepWorkerWalRcvConn) >= 12)
> appendStringInfo(&cmd, " AND a.attgenerated = ''");
> + }
> + else if (walrcv_server_version(LogRepWorkerWalRcvConn) >= 17)
> + {
> + if(MySubscription->includegencols)
> + appendStringInfo(&cmd, " AND a.attgenerated != 'v'");
> + else
> + appendStringInfo(&cmd, " AND a.attgenerated = ''");
> + }
>
> IMO this logic is too tricky to remain uncommented -- please add some
> comments.
> Also, it seems somewhat complex. I think you can achieve the same more simply:
>
> SUGGESTION
>
> if (walrcv_server_version(LogRepWorkerWalRcvConn) >= 12)
> {
> bool gencols_allowed = walrcv_server_version(LogRepWorkerWalRcvConn) >=
> 17
> && MySubscription->includegencols;
> if (gencols_allowed)
> {
> /* Replication of generated cols is supported, but not VIRTUAL cols. */
> appendStringInfo(&cmd, " AND a.attgenerated != 'v'");
> }
> else
> {
> /* Replication of generated cols is not supported. */
> appendStringInfo(&cmd, " AND a.attgenerated = ''");
> }
> }
Fixed
> ==
>
> 99.
> Please refer also to my attached nitpick diffs and apply those if you agree.
Applied the changes.
I have attached the updated patch v10 here in [1].
[1]:
https://www.postgresql.org/message-id/CANhcyEUMCk6cCbw0vVZWo8FRd6ae9CmKG%3DgKP-9Q67jLn8HqtQ%40mail.gmail.com
Thanks and Regards,
Shlok Kyal
Re: Pgoutput not capturing the generated columns
On Thu, 20 Jun 2024 at 12:52, vignesh C wrote: > > On Wed, 19 Jun 2024 at 21:43, Peter Eisentraut wrote: > > > > On 19.06.24 13:22, Shubham Khanna wrote: > > > All the comments are handled. > > > > > > The attached Patch contains all the suggested changes. > > > > Please also take a look at the proposed patch for virtual generated > > columns [0] and consider how that would affect your patch. I think your > > feature can only replicate *stored* generated columns. So perhaps the > > documentation and terminology in your patch should reflect that. > > This patch is unable to manage virtual generated columns because it > stores NULL values for them. Along with documentation the initial sync > command being generated also should be changed to sync data > exclusively for stored generated columns, omitting virtual ones. I > suggest treating these changes as a separate patch(0003) for future > merging or a separate commit, depending on the order of patch > acceptance. > I have addressed the issue and updated the documentation accordingly. And created a new 0003 patch. Please refer to v9-0003 patch for the same in [1]. [1]: https://www.postgresql.org/message-id/CANhcyEXmjLEPNgOSAtjS4YGb9JvS8w-SO9S%2BjRzzzXo2RavNWw%40mail.gmail.com Thanks and Regards, Shlok Kyal
Re: Pgoutput not capturing the generated columns
On Fri, 14 Jun 2024 at 15:52, Shubham Khanna
wrote:
>
> The attached Patch contains the suggested changes.
>
Hi Shubham, thanks for providing a patch.
I have some comments for v6-0001.
1. create_subscription.sgml
There is repetition of the same line.
+
+ Specifies whether the generated columns present in the tables
+ associated with the subscription should be replicated. If the
+ subscriber-side column is also a generated column then this option
+ has no effect; the replicated data will be ignored and the subscriber
+ column will be filled as normal with the subscriber-side computed or
+ default data.
+ false.
+
+
+
+ This parameter can only be set true if
copy_data is
+ set to false. If the subscriber-side
column is also a
+ generated column then this option has no effect; the
replicated data will
+ be ignored and the subscriber column will be filled as
normal with the
+ subscriber-side computed or default data.
+
==
2. subscriptioncmds.c
2a. The macro name should be in uppercase. We can use a short name
like 'SUBOPT_INCLUDE_GEN_COL'. Thought?
+#define SUBOPT_include_generated_columns 0x0001
2b.Update macro name accordingly
+ if (IsSet(supported_opts, SUBOPT_include_generated_columns))
+ opts->include_generated_columns = false;
2c. Update macro name accordingly
+ else if (IsSet(supported_opts, SUBOPT_include_generated_columns) &&
+ strcmp(defel->defname, "include_generated_columns") == 0)
+ {
+ if (IsSet(opts->specified_opts, SUBOPT_include_generated_columns))
+ errorConflictingDefElem(defel, pstate);
+
+ opts->specified_opts |= SUBOPT_include_generated_columns;
+ opts->include_generated_columns = defGetBoolean(defel);
+ }
2d. Update macro name accordingly
+ SUBOPT_RUN_AS_OWNER | SUBOPT_FAILOVER | SUBOPT_ORIGIN |
+ SUBOPT_include_generated_columns);
==
3. decoding_into_rel.out
3a. In comment, I think it should be "When 'include-generated-columns'
= '1' the generated column 'b' values will be replicated"
+-- When 'include-generated-columns' = '1' the generated column 'b'
values will not be replicated
+INSERT INTO gencoltable (a) VALUES (1), (2), (3);
+SELECT data FROM pg_logical_slot_get_changes('regression_slot', NULL,
NULL, 'include-xids', '0', 'skip-empty-xacts', '1',
'include-generated-columns', '1');
+data
+-
+ BEGIN
+ table public.gencoltable: INSERT: a[integer]:1 b[integer]:2
+ table public.gencoltable: INSERT: a[integer]:2 b[integer]:4
+ table public.gencoltable: INSERT: a[integer]:3 b[integer]:6
+ COMMIT
3b. In comment, I think it should be "When 'include-generated-columns'
= '1' the generated column 'b' values will not be replicated"
+-- When 'include-generated-columns' = '0' the generated column 'b'
values will be replicated
+SELECT data FROM pg_logical_slot_get_changes('regression_slot', NULL,
NULL, 'include-xids', '0', 'skip-empty-xacts', '1',
'include-generated-columns', '0');
+ data
+
+ BEGIN
+ table public.gencoltable: INSERT: a[integer]:4
+ table public.gencoltable: INSERT: a[integer]:5
+ table public.gencoltable: INSERT: a[integer]:6
+ COMMIT
+(5 rows)
=
4. Here names for both the tests are the same. I think we should use
different 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');
Thanks and Regards,
Shlok Kyal
Re: Pgoutput not capturing the generated columns
On Thu, 6 Jun 2024 at 08:29, Hayato Kuroda (Fujitsu) wrote: > > Dear Shlok and Shubham, > > Thanks for updating the patch! > > I briefly checked the v5-0002. IIUC, your patch allows to copy generated > columns unconditionally. I think the behavior affects many people so that it > is > hard to get agreement. > > Can we add a new option like `GENERATED_COLUMNS [boolean]`? If the default is > set > to off, we can keep the current specification. > > Thought? Hi Kuroda-san, I agree that we should not allow to copy generated columns unconditionally. With patch v7-0002, I have used a different approach which does not require any code changes in COPY. Please refer [1] for patch v7-0002. [1]: https://www.postgresql.org/message-id/CANhcyEUz0FcyR3T76b%2BNhtmvWO7o96O_oEwsLZNZksEoPmVzXw%40mail.gmail.com Thanks and Regards, Shlok Kyal
Re: Pgoutput not capturing the generated columns
On Wed, 5 Jun 2024 at 05:49, Peter Smith wrote: > > On Mon, Jun 3, 2024 at 9:52 PM Shlok Kyal wrote: > > > > > > > > The attached Patch contains the suggested changes. > > > > > > > Hi, > > > > Currently, COPY command does not work for generated columns and > > therefore, COPY of generated column is not supported during tablesync > > process. So, in patch v4-0001 we added a check to allow replication of > > the generated column only if 'copy_data = false'. > > > > I am attaching patches to resolve the above issues. > > > > v5-0001: not changed > > v5-0002: Support COPY of generated column > > v5-0003: Support COPY of generated column during tablesync process > > > > Hi Shlok, I have a question about patch v5-0003. > > According to the patch 0001 docs "If the subscriber-side column is > also a generated column then this option has no effect; the replicated > data will be ignored and the subscriber column will be filled as > normal with the subscriber-side computed or default data". > > Doesn't this mean it will be a waste of effort/resources to COPY any > column value where the subscriber-side column is generated since we > know that any copied value will be ignored anyway? > > But I don't recall seeing any comment or logic for this kind of copy > optimisation in the patch 0003. Is this already accounted for > somewhere and I missed it, or is my understanding wrong? Your understanding is correct. With v7-0002, if a subscriber-side column is generated, then we do not include that column in the column list during COPY. This will address the above issue. Patch 7-0002 contains all the changes. Please refer [1] [1]: https://www.postgresql.org/message-id/CANhcyEUz0FcyR3T76b%2BNhtmvWO7o96O_oEwsLZNZksEoPmVzXw%40mail.gmail.com Thanks and Regards, Shlok Kyal
Re: Pgoutput not capturing the generated columns
On Tue, 4 Jun 2024 at 15:01, Peter Smith wrote:
>
> Hi,
>
> Here are some review comments for patch v5-0003.
>
> ==
> 0. Whitespace warnings when the patch was applied.
>
> [postgres@CentOS7-x64 oss_postgres_misc]$ git apply
> ../patches_misc/v5-0003-Support-copy-of-generated-columns-during-tablesyn.patch
> ../patches_misc/v5-0003-Support-copy-of-generated-columns-during-tablesyn.patch:29:
> trailing whitespace.
> has no effect; the replicated data will be ignored and the
> subscriber
> ../patches_misc/v5-0003-Support-copy-of-generated-columns-during-tablesyn.patch:30:
> trailing whitespace.
> column will be filled as normal with the subscriber-side computed or
> ../patches_misc/v5-0003-Support-copy-of-generated-columns-during-tablesyn.patch:189:
> trailing whitespace.
> (walrcv_server_version(LogRepWorkerWalRcvConn) >= 12 &&
> warning: 3 lines add whitespace errors.
>
Fixed
> ==
> src/backend/commands/subscriptioncmds.c
>
> 1.
> - res = walrcv_exec(wrconn, cmd.data, check_columnlist ? 3 : 2, tableRow);
> + column_count = (!include_generated_column && check_gen_col) ? 4 :
> (check_columnlist ? 3 : 2);
> + res = walrcv_exec(wrconn, cmd.data, column_count, tableRow);
>
> The 'column_count' seems out of control. Won't it be far simpler to
> assign/increment the value dynamically only as required instead of the
> tricky calculation at the end which is unnecessarily difficult to
> understand?
>
I have removed this piece of code.
> ~~~
>
> 2.
> + /*
> + * If include_generated_column option is false and all the column of
> the table in the
> + * publication are generated then we should throw an error.
> + */
> + if (!isnull && !include_generated_column && check_gen_col)
> + {
> + attlist = DatumGetArrayTypeP(attlistdatum);
> + gen_col_count = DatumGetInt32(slot_getattr(slot, 4, &isnull));
> + Assert(!isnull);
> +
> + attcount = ArrayGetNItems(ARR_NDIM(attlist), ARR_DIMS(attlist));
> +
> + if (attcount != 0 && attcount == gen_col_count)
> + ereport(ERROR,
> + errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> + errmsg("cannot use only generated column for table \"%s.%s\" in
> publication when generated_column option is false",
> +nspname, relname));
> + }
> +
>
> Why do you think this new logic/error is necessary?
>
> IIUC the 'include_generated_columns' should be false to match the
> existing HEAD behavior. So this scenario where your publisher-side
> table *only* has generated columns is something that could already
> happen, right? IOW, this introduced error could be a candidate for
> another discussion/thread/patch, but is it really required for this
> current patch?
>
Yes, this scenario can also happen in HEAD. For this patch I have
removed this check.
> ==
> src/backend/replication/logical/tablesync.c
>
> 3.
> lrel->remoteid,
> - (walrcv_server_version(LogRepWorkerWalRcvConn) >= 12 ?
> - "AND a.attgenerated = ''" : ""),
> + (walrcv_server_version(LogRepWorkerWalRcvConn) >= 12 &&
> + (walrcv_server_version(LogRepWorkerWalRcvConn) <= 16 ||
> + !MySubscription->includegeneratedcolumn) ? "AND a.attgenerated = ''" : ""),
>
> This ternary within one big appendStringInfo seems quite complicated.
> Won't it be better to split the appendStringInfo into multiple parts
> so the generated-cols calculation can be done more simply?
>
Fixed
> ==
> src/test/subscription/t/011_generated.pl
>
> 4.
> I think there should be a variety of different tablesync scenarios
> (when 'include_generated_columns' is true) tested here instead of just
> one, and all varieties with lots of comments to say what they are
> doing, expectations etc.
>
> a. publisher-side gen-col "a" replicating to subscriber-side NOT
> gen-col "a" (ok, value gets replicated)
> b. publisher-side gen-col "a" replicating to subscriber-side gen-col
> (ok, but ignored)
> c. publisher-side NOT gen-col "b" replicating to subscriber-side
> gen-col "b" (error?)
>
Added the tests
> ~~
>
> 5.
> +$result = $node_subscriber->safe_psql('postgres', "SELECT a, b FROM tab3");
> +is( $result, qq(1|2
> +2|4
> +3|6), 'generated columns initial sync with include_generated_column = true');
>
> Should this say "ORDER BY..." so it will not fail if the row order
> happens to be something unanticipated?
>
Fixed
> ==
>
> 99.
> Also, see the attached file with numerous other nitpicks:
> - plural param- and var-names
> - typos in comments
> - missing spaces
> - SQL keyword should be UPPERCASE
> - etc.
>
> Please apply any/all of these if you agree with them.
Fixed
Patch 7-0002 contains all the changes. Please refer [1]
[1]:
https://www.postgresql.org/message-id/CANhcyEUz0FcyR3T76b%2BNhtmvWO7o96O_oEwsLZNZksEoPmVzXw%40mail.gmail.com
Thanks and Regards,
Shlok Kyal
Re: Pgoutput not capturing the generated columns
On Tue, 4 Jun 2024 at 10:21, Peter Smith wrote: > > Hi, > > Here are some review comments for patch v5-0002. > > == > GENERAL > > G1. > IIUC now you are unconditionally allowing all generated columns to be copied. > > I think this is assuming that the table sync code (in the next patch > 0003?) is going to explicitly name all the columns it wants to copy > (so if it wants to get generated cols then it will name the generated > cols, and if is doesn't want generated cols then it won't name them). > > Maybe that is OK for the logical replication tablesync case, but I am > not sure if it will be desirable to *always* copy generated columns in > other user scenarios. > > e.g. I was wondering if there should be a new COPY command option > introduced here -- INCLUDE_GENERATED_COLUMNS (with default false) so > then the current HEAD behaviour is unaffected unless that option is > enabled. > > ~~~ > > G2. > The current COPY command documentation [1] says "If no column list is > specified, all columns of the table except generated columns will be > copied." > > But this 0002 patch has changed that documented behaviour, and so the > documentation needs to be changed as well, right? > > == > Commit Message > > 1. > Currently COPY command do not copy generated column. With this commit > added support for COPY for generated column. > > ~ > > The grammar/cardinality is not good here. Try some tool (Grammarly or > chatGPT, etc) to help correct it. > > == > src/backend/commands/copy.c > > == > src/test/regress/expected/generated.out > > == > src/test/regress/sql/generated.sql > > 2. > I think these COPY test cases require some explicit comments to > describe what they are doing, and what are the expected results. > > Currently, I have doubts about some of this test input/output > > e.g.1. Why is the 'b' column sometimes specified as 1? It needs some > explanation. Are you expecting this generated col value to be > ignored/overwritten or what? > > COPY gtest1 (a, b) FROM stdin DELIMITER ' '; > 5 1 > 6 1 > \. > > e.g.2. what is the reason for this new 'missing data for column "b"' > error? Or is it some introduced quirk because "b" now cannot be > generated since there is no value for "a"? I don't know if the > expected *.out here is OK or not, so some test comments may help to > clarify it. > > == > [1] https://www.postgresql.org/docs/devel/sql-copy.html > Hi Peter, I have removed the changes in the COPY command. I came up with an approach which requires changes only in tablesync code. We can COPY generated columns during tablesync using syntax 'COPY (SELECT column_name from table) TO STDOUT.' I have attached the patch for the same. v7-0001 : Not Modified v7-0002: Support replication of generated columns during initial sync. Thanks and Regards, Shlok Kyal v7-0002-Support-replication-of-generated-column-during-in.patch Description: Binary data v7-0001-Enable-support-for-include_generated_columns-opti.patch Description: Binary data
Re: 001_rep_changes.pl fails due to publisher stuck on shutdown
On Mon, 10 Jun 2024 at 15:10, Shlok Kyal wrote: > > On Thu, 6 Jun 2024 at 11:49, Kyotaro Horiguchi > wrote: > > > > At Thu, 6 Jun 2024 12:49:45 +1000, Peter Smith > > wrote in > > > Hi, I have reproduced this multiple times now. > > > > > > I confirmed the initial post/steps from Alexander. i.e. The test > > > script provided [1] gets itself into a state where function > > > ReadPageInternal (called by XLogDecodeNextRecord and commented "Wait > > > for the next page to become available") constantly returns > > > XLREAD_FAIL. Ultimately the test times out because WalSndLoop() loops > > > forever, since it never calls WalSndDone() to exit the walsender > > > process. > > > > Thanks for the repro; I believe I understand what's happening here. > > > > During server shutdown, the latter half of the last continuation > > record may fail to be flushed. This is similar to what is described in > > the commit message of commit ff9f111bce. While shutting down, > > WalSndLoop() waits for XLogSendLogical() to consume WAL up to > > flushPtr, but in this case, the last record cannot complete without > > the continuation part starting from flushPtr, which is > > missing. However, in such cases, xlogreader.missingContrecPtr is set > > to the beginning of the missing part, but something similar to > > > > So, I believe the attached small patch fixes the behavior. I haven't > > come up with a good test script for this issue. Something like > > 026_overwrite_contrecord.pl might work, but this situation seems a bit > > more complex than what it handles. > > > > Versions back to 10 should suffer from the same issue and the same > > patch will be applicable without significant changes. > > I tested the changes for PG 12 to master as we do not support prior versions. > The patch applied successfully for master and PG 16. I ran the test > provided in [1] multiple times and it ran successfully each time. > The patch did not apply on PG 15. I did a similar change for PG 15 and > created a patch. I ran the test multiple times and it was successful > every time. > The patch did not apply on PG 14 to PG 12. I did a similar change in > each branch. But the tests did not pass in each branch. I, by mistake, applied wrong changes in PG 14 to PG 12. I tested again for all versions and the test ran successfully for all of them till PG12. I have also attached the patch which applies for PG14 to PG12. Sorry for the inconvenience. Thanks and Regards, Shlok Kyal v2-0001-Fix-infinite-loop-in-walsender-during-publisher-s.patch Description: Binary data v2-0001-Fix-infinite-loop-in-walsender-during-publisher-s-PG-15.patch Description: Binary data v2-0001-Fix-infinite-loop-in-walsender-during-publisher-s-PG-14.patch Description: Binary data
Re: confirmed flush lsn seems to be move backward in certain error cases
On Mon, 10 Jun 2024 at 16:39, Amit Kapila wrote:
>
> On Tue, Feb 20, 2024 at 12:35 PM vignesh C wrote:
> >
> > On Sat, 17 Feb 2024 at 12:03, Amit Kapila wrote:
> > >
> > >
> > > @@ -1839,7 +1839,8 @@ LogicalConfirmReceivedLocation(XLogRecPtr lsn)
> > >
> > > SpinLockAcquire(&MyReplicationSlot->mutex);
> > >
> > > - MyReplicationSlot->data.confirmed_flush = lsn;
> > > + if (lsn > MyReplicationSlot->data.confirmed_flush)
> > > + MyReplicationSlot->data.confirmed_flush = lsn;
> > >
> > > /* if we're past the location required for bumping xmin, do so */
> > > if (MyReplicationSlot->candidate_xmin_lsn != InvalidXLogRecPtr &&
> > > @@ -1904,7 +1905,8 @@ LogicalConfirmReceivedLocation(XLogRecPtr lsn)
> > > else
> > > {
> > > SpinLockAcquire(&MyReplicationSlot->mutex);
> > > - MyReplicationSlot->data.confirmed_flush = lsn;
> > > + if (lsn > MyReplicationSlot->data.confirmed_flush)
> > > + MyReplicationSlot->data.confirmed_flush = lsn;
> > >
> > > BTW, from which code path does it update the prior value of
> > > confirmed_flush?
> >
> > The confirmed_flush is getting set in the else condition for this scenario.
> >
> > If it is through the else check, then can we see if
> > > it may change the confirm_flush to the prior position via the first
> > > code path? I am asking because in the first code path, we can even
> > > flush the re-treated value of confirm_flush LSN.
> >
> > I was not able to find any scenario to set a prior position with the
> > first code path. I tried various scenarios like adding delay in
> > walsender, add delay in apply worker, restart the instances and with
> > various DML operations. It was always setting it to either to the same
> > value as previous or greater value.
> >
>
> Fair enough. This means that in the prior versions, it was never
> possible to move confirmed_flush LSN in the slot to a backward
> position on the disk. So, moving it backward temporarily (in the
> memory) shouldn't create any problem. I would prefer your
> Assert_confirmed_flush_will_always_not_be_less_than_last_saved_confirmed_flush.patch
> to fix this issue.
>
> Thoughts?
I was able to reproduce the issue with the test script provided in
[1]. I ran the script 10 times and I was able to reproduce the issue
4 times. I also tested the patch
Assert_confirmed_flush_will_always_not_be_less_than_last_saved_confirmed_flush.patch.
and it resolves the issue. I ran the test script 20 times and I was
not able to reproduce the issue.
[1]:
https://www.postgresql.org/message-id/CALDaNm3hgow2%2BoEov5jBk4iYP5eQrUCF1yZtW7%2BdV3J__p4KLQ%40mail.gmail.com
Thanks and Regards,
Shlok Kyal
Re: 001_rep_changes.pl fails due to publisher stuck on shutdown
On Thu, 6 Jun 2024 at 11:49, Kyotaro Horiguchi wrote: > > At Thu, 6 Jun 2024 12:49:45 +1000, Peter Smith wrote > in > > Hi, I have reproduced this multiple times now. > > > > I confirmed the initial post/steps from Alexander. i.e. The test > > script provided [1] gets itself into a state where function > > ReadPageInternal (called by XLogDecodeNextRecord and commented "Wait > > for the next page to become available") constantly returns > > XLREAD_FAIL. Ultimately the test times out because WalSndLoop() loops > > forever, since it never calls WalSndDone() to exit the walsender > > process. > > Thanks for the repro; I believe I understand what's happening here. > > During server shutdown, the latter half of the last continuation > record may fail to be flushed. This is similar to what is described in > the commit message of commit ff9f111bce. While shutting down, > WalSndLoop() waits for XLogSendLogical() to consume WAL up to > flushPtr, but in this case, the last record cannot complete without > the continuation part starting from flushPtr, which is > missing. However, in such cases, xlogreader.missingContrecPtr is set > to the beginning of the missing part, but something similar to > > So, I believe the attached small patch fixes the behavior. I haven't > come up with a good test script for this issue. Something like > 026_overwrite_contrecord.pl might work, but this situation seems a bit > more complex than what it handles. > > Versions back to 10 should suffer from the same issue and the same > patch will be applicable without significant changes. I tested the changes for PG 12 to master as we do not support prior versions. The patch applied successfully for master and PG 16. I ran the test provided in [1] multiple times and it ran successfully each time. The patch did not apply on PG 15. I did a similar change for PG 15 and created a patch. I ran the test multiple times and it was successful every time. The patch did not apply on PG 14 to PG 12. I did a similar change in each branch. But the tests did not pass in each branch. I have attached a patch which applies successfully on the PG 15 branch. [1]: https://www.postgresql.org/message-id/f15d665f-4cd1-4894-037c-afdbe3692...@gmail.com Thanks and Regards, Shlok Kyal v2-0001-Fix-infinite-loop-in-walsender-during-publisher-s.patch Description: Binary data v2-0001-Fix-infinite-loop-in-walsender-during-publisher-s-PG-15.patch Description: Binary data
Re: Pgoutput not capturing the generated columns
> > The attached Patch contains the suggested changes. > Hi, Currently, COPY command does not work for generated columns and therefore, COPY of generated column is not supported during tablesync process. So, in patch v4-0001 we added a check to allow replication of the generated column only if 'copy_data = false'. I am attaching patches to resolve the above issues. v5-0001: not changed v5-0002: Support COPY of generated column v5-0003: Support COPY of generated column during tablesync process Thought? Thanks and Regards, Shlok Kyal v5-0001-Enable-support-for-include_generated_columns-opti.patch Description: Binary data v5-0002-Support-COPY-command-for-generated-columns.patch Description: Binary data v5-0003-Support-copy-of-generated-columns-during-tablesyn.patch Description: Binary data
Re: speed up a logical replica setup
On Wed, 22 May 2024 at 16:50, Amit Kapila wrote: > > On Mon, May 20, 2024 at 4:30 PM Shlok Kyal wrote: > > > > > > I was trying to test this utility when 'sync_replication_slots' is on > > > and it gets in an ERROR loop [1] and never finishes. Please find the > > > postgresql.auto used on the standby attached. I think if the standby > > > has enabled sync_slots, you need to pass dbname in > > > GenerateRecoveryConfig(). I couldn't test it further but I wonder if > > > there are already synced slots on the standby (either due to > > > 'sync_replication_slots' or users have used > > > pg_sync_replication_slots() before invoking pg_createsubscriber), > > > those would be retained as it is on new subscriber and lead to > > > unnecessary WAL retention and dead rows. > > > > > > [1] > > > 2024-04-30 11:50:43.239 IST [12536] LOG: slot sync worker started > > > 2024-04-30 11:50:43.247 IST [12536] ERROR: slot synchronization > > > requires dbname to be specified in primary_conninfo > > > > Hi, > > > > I tested the scenario posted by Amit in [1], in which retaining synced > > slots lead to unnecessary WAL retention and ERROR. This is raised as > > the second open point in [2]. > > The steps to reproduce the issue: > > (1) Setup physical replication with sync slot feature turned on by > > setting sync_replication_slots = 'true' or using > > pg_sync_replication_slots() on the standby node. > > For physical replication setup, run pg_basebackup with -R and -d option. > > (2) Create a logical replication slot on primary node with failover > > option as true. A corresponding slot is created on standby as part of > > sync slot feature. > > (3) Run pg_createsubscriber on standby node. > > (4) On Checking for the replication slot on standby node, I noticed > > that the logical slots created in step 2 are retained. > > I have attached the script to reproduce the issue. > > > > I and Kuroda-san worked to resolve open points. Here are patches to > > solve the second and third point in [2]. > > Patches proposed by Euler are also attached just in case, but they > > were not modified. > > > > v2-0001: not changed > > > > Shouldn't we modify it as per the suggestion given in the email [1]? I > am wondering if we can entirely get rid of checking the primary > business and simply rely on recovery_timeout and keep checking > server_is_in_recovery(). If so, we can modify the test to use > non-default recovery_timeout (say 180s or something similar if we have > used it at any other place). As an additional check we can ensure that > constent_lsn is present on standby. > I have made changes as per your suggestion. I have used pg_last_wal_receive_lsn to get lsn last wal received on standby and check if consistent_lsn is already present on the standby. I have only made changes in v3-0001. v3-0002, v3-0003, v3-0004 and v3-0005 are not modified. Thanks and Regards, Shlok Kyal v3-0005-Avoid-outputing-some-messages-in-dry_run-mode.patch Description: Binary data v3-0003-Disable-slot-sync-during-the-convertion.patch Description: Binary data v3-0001-Improve-the-code-that-checks-if-the-recovery-is-f.patch Description: Binary data v3-0004-Drop-replication-slots-which-had-been-synchronize.patch Description: Binary data v3-0002-Improve-the-code-that-checks-if-the-primary-slot-.patch Description: Binary data
Re: State of pg_createsubscriber
> Just to summarize, apart from BF failures for which we had some > discussion, I could recall the following open points: > > 1. After promotion, the pre-existing replication objects should be > removed (either optionally or always), otherwise, it can lead to a new > subscriber not being able to restart or getting some unwarranted data. > [1][2]. > I tried to reproduce the case and found a case where pre-existing replication objects can cause unwanted scenario: Suppose we have a setup of nodes N1, N2 and N3. N1 and N2 are in streaming replication where N1 is primary and N2 is standby. N3 and N1 are in logical replication where N3 is publisher and N1 is subscriber. The subscription created on N1 is replicated to N2 due to streaming replication. Now, after we run pg_createsubscriber on N2 and start the N2 server, we get the following logs repetitively: 2024-05-22 11:37:18.619 IST [27344] ERROR: could not start WAL streaming: ERROR: replication slot "test1" is active for PID 27202 2024-05-22 11:37:18.622 IST [27317] LOG: background worker "logical replication apply worker" (PID 27344) exited with exit code 1 2024-05-22 11:37:23.610 IST [27349] LOG: logical replication apply worker for subscription "test1" has started 2024-05-22 11:37:23.624 IST [27349] ERROR: could not start WAL streaming: ERROR: replication slot "test1" is active for PID 27202 2024-05-22 11:37:23.627 IST [27317] LOG: background worker "logical replication apply worker" (PID 27349) exited with exit code 1 2024-05-22 11:37:28.616 IST [27382] LOG: logical replication apply worker for subscription "test1" has started Note: 'test1' is the name of the subscription created on N1 initially and by default, slot name is the same as the subscription name. Once the N2 server is started after running pg_createsubscriber, the subscription that was earlier replicated by streaming replication will now try to connect to the publisher. Since the subscription name in N2 is the same as the subscription created in N1, it will not be able to start a replication slot as the slot with the same name is active for logical replication between N3 and N1. Also, there would be a case where N1 becomes down for some time. Then in that case subscription on N2 will connect to the publication on N3 and now data from N3 will be replicated to N2 instead of N1. And once N1 is up again, subscription on N1 will not be able to connect to publication on N3 as it is already connected to N2. This can lead to data inconsistency. This error did not happen before running pg_createsubscriber on standby node N2, because there is no 'logical replication launcher' process on standby node. Thanks and Regards, Shlok Kyal
Re: speed up a logical replica setup
Hi,
>
> I was trying to test this utility when 'sync_replication_slots' is on
> and it gets in an ERROR loop [1] and never finishes. Please find the
> postgresql.auto used on the standby attached. I think if the standby
> has enabled sync_slots, you need to pass dbname in
> GenerateRecoveryConfig(). I couldn't test it further but I wonder if
> there are already synced slots on the standby (either due to
> 'sync_replication_slots' or users have used
> pg_sync_replication_slots() before invoking pg_createsubscriber),
> those would be retained as it is on new subscriber and lead to
> unnecessary WAL retention and dead rows.
>
> [1]
> 2024-04-30 11:50:43.239 IST [12536] LOG: slot sync worker started
> 2024-04-30 11:50:43.247 IST [12536] ERROR: slot synchronization
> requires dbname to be specified in primary_conninfo
Hi,
I tested the scenario posted by Amit in [1], in which retaining synced
slots lead to unnecessary WAL retention and ERROR. This is raised as
the second open point in [2].
The steps to reproduce the issue:
(1) Setup physical replication with sync slot feature turned on by
setting sync_replication_slots = 'true' or using
pg_sync_replication_slots() on the standby node.
For physical replication setup, run pg_basebackup with -R and -d option.
(2) Create a logical replication slot on primary node with failover
option as true. A corresponding slot is created on standby as part of
sync slot feature.
(3) Run pg_createsubscriber on standby node.
(4) On Checking for the replication slot on standby node, I noticed
that the logical slots created in step 2 are retained.
I have attached the script to reproduce the issue.
I and Kuroda-san worked to resolve open points. Here are patches to
solve the second and third point in [2].
Patches proposed by Euler are also attached just in case, but they
were not modified.
v2-0001: not changed
v2-0002: not changed
v2-0003: ensures the slot sync is disabled during the conversion. This
resolves the second point.
v2-0004: drops sync slots which may be retained after running. This
resolves the second point.
v2-0005: removes misleading output messages in dry-run. This resolves
the third point.
[1]:
https://www.postgresql.org/message-id/CAA4eK1KdCb%2B5sjYu6qCMXXdCX1y_ihr8kFzMozq0%3DP%3DauYxgog%40mail.gmail.com
[2]:
https://www.postgresql.org/message-id/CAA4eK1J22UEfrqx222h5j9DQ7nxGrTbAa_BC%2B%3DmQXdXs-RCsew%40mail.gmail.com
Thanks and Regards,
Shlok Kyal
# cleanup
rm -rf ../primary ../standby primary.log standby.log
# setup primary node
./initdb -D ../primary
cat << EOF >> ../primary/postgresql.conf
wal_level = logical
EOF
./pg_ctl -D ../primary -l primary.log start
./psql -d postgres -c "CREATE table t1 (c1 int);"
./psql -d postgres -c "Insert into t1 values(1);"
./psql -d postgres -c "Insert into t1 values(2);"
./psql -d postgres -c "INSERT into t1 values(3);"
./psql -d postgres -c "INSERT into t1 values(4);"
# setup standby node with sync slot feature
./pg_basebackup -h localhost -X stream -v -W -R -S 'sb1' -C -D ../standby/ -d 'dbname=postgres'
cat << EOF >> ../standby/postgresql.conf
sync_replication_slots = 'true'
hot_standby_feedback = 'on'
port = 9000
EOF
./pg_ctl -D ../standby -l standby.log start
# create a replication slot on primary with failover option
./psql -d postgres -c "SELECT 'init' FROM pg_create_logical_replication_slot('failover_true_slot_2', 'test_decoding', false, false, true);"
sleep 2
# check if slots are synced on standby
./psql -d postgres -p 9000 -c "select * from pg_replication_slots;"
./pg_ctl -D ../standby -l standby.log stop
# run pg_createsubscriber on standby
./pg_createsubscriber -D ../standby/ -p 9000 -P "host=localhost port=5432 dbname=postgres" -d postgres -v
# check if replication slots are retained after running pg_createsubscriber
./pg_ctl -D ../standby -l standby.log start
./psql -d postgres -p 9000 -c "select * from pg_replication_slots;"
./pg_ctl -D ../standby -l standby.log stop
./pg_ctl -D ../primary -l primary.log stop
v2-0001-Improve-the-code-that-checks-if-the-recovery-is-f.patch
Description: Binary data
v2-0002-Improve-the-code-that-checks-if-the-primary-slot-.patch
Description: Binary data
v2-0003-Disable-slot-sync-during-the-convertion.patch
Description: Binary data
v2-0004-Drop-replication-slots-which-had-been-synchronize.patch
Description: Binary data
v2-0005-Avoid-outputing-some-messages-in-dry_run-mode.patch
Description: Binary data
Re: Pgoutput not capturing the generated columns
Hi Kuroda-san, Thanks for reviewing the patch. I have fixed some of the comments > 2. > Currently, the option is implemented as streaming option. Are there any > reasons > to choose the way? Another approach is to implement as slot option, like > failover > and temporary. I think the current approach is appropriate. The options such as failover and temporary seem like properties of a slot and I think decoding of generated column should not be slot specific. Also adding a new option for slot may create an overhead. > 3. > You said that subscription option is not supported for now. Not sure, is it > mean > that logical replication feature cannot be used for generated columns? If so, > the restriction won't be acceptable. If the combination between this and > initial > sync is problematic, can't we exclude them in CreateSubscrition and > AlterSubscription? > E.g., create_slot option cannot be set if slot_name is NONE. Added an option 'generated_column' for create subscription. Currently it allow to set 'generated_column' option as true only if 'copy_data' is set to false. Also we don't allow user to alter the 'generated_column' option. > 6. logicalrep_write_tuple() > > ``` > -if (!column_in_column_list(att->attnum, columns)) > +if (!column_in_column_list(att->attnum, columns) && > !att->attgenerated) > +continue; > ``` > > Hmm, does above mean that generated columns are decoded even if they are not > in > the column list? If so, why? I think such columns should not be sent. Fixed Thanks and Regards, Shlok Kyal v2-0001-Support-generated-column-capturing-generated-colu.patch Description: Binary data
Re: speed up a logical replica setup
> Hi, > > > I'm attaching a new version (v30) that adds: > > > > * 3 new options (--publication, --subscription, --replication-slot) to > > assign > > names to the objects. The --database option used to ignore duplicate > > names, > > however, since these new options rely on the number of database options to > > match the number of object name options, it is forbidden from now on. The > > duplication is also forbidden for the object names to avoid errors > > earlier. > > * rewrite the paragraph related to unusuable target server after > > pg_createsubscriber fails. > > * Vignesh reported an issue [1] related to reaching the recovery stop point > > before the consistent state is reached. I proposed a simple patch that > > fixes > > the issue. > > > > [1] > > https://www.postgresql.org/message-id/CALDaNm3VMOi0GugGvhk3motghaFRKSWMCSE2t3YX1e%2BMttToxg%40mail.gmail.com > > > > I have added a top-up patch v30-0003. The issue in [1] still exists in > the v30 patch. I was not able to come up with an approach to handle it > in the code, so I have added it to the documentation in the warning > section. Thoughts? > I am not changing the version as I have not made any changes in > v30-0001 and v30-0002. > > [1]: > https://www.postgresql.org/message-id/cahv8rj+5mzk9jt+7ecogjzfm5czvdccd5jo1_rcx0bteypb...@mail.gmail.com There was some whitespace error in the v30-0003 patch. Updated the patch. Thanks and regards, Shlok Kyal From d6717bcaf94302c0d41eabd448802b1fae6167d9 Mon Sep 17 00:00:00 2001 From: Euler Taveira Date: Mon, 5 Jun 2023 14:39:40 -0400 Subject: [PATCH v31 1/3] pg_createsubscriber: creates a new logical replica from a standby server It must be run on the target server and should be able to connect to the source server (publisher) and the target server (subscriber). All tables in the specified database(s) are included in the logical replication setup. A pair of publication and subscription objects are created for each database. The main advantage of pg_createsubscriber over the common logical replication setup is the initial data copy. It also reduces the catchup phase. Some prerequisites must be met to successfully run it. It is basically the logical replication requirements. It starts creating a publication using FOR ALL TABLES and a replication slot for each specified database. Write recovery parameters into the target data directory and start the target server. It specifies the LSN of the last replication slot (replication start point) up to which the recovery will proceed. Wait until the target server is promoted. Create one subscription per specified database (using publication and replication slot created in a previous step) on the target server. Set the replication progress to the replication start point for each subscription. Enable the subscription for each specified database on the target server. And finally, change the system identifier on the target server. --- doc/src/sgml/ref/allfiles.sgml|1 + doc/src/sgml/ref/pg_createsubscriber.sgml | 525 doc/src/sgml/reference.sgml |1 + src/bin/pg_basebackup/.gitignore |1 + src/bin/pg_basebackup/Makefile| 11 +- src/bin/pg_basebackup/meson.build | 19 + src/bin/pg_basebackup/pg_createsubscriber.c | 2131 + .../t/040_pg_createsubscriber.pl | 326 +++ 8 files changed, 3012 insertions(+), 3 deletions(-) create mode 100644 doc/src/sgml/ref/pg_createsubscriber.sgml create mode 100644 src/bin/pg_basebackup/pg_createsubscriber.c create mode 100644 src/bin/pg_basebackup/t/040_pg_createsubscriber.pl diff --git a/doc/src/sgml/ref/allfiles.sgml b/doc/src/sgml/ref/allfiles.sgml index 4a42999b18..f5be638867 100644 --- a/doc/src/sgml/ref/allfiles.sgml +++ b/doc/src/sgml/ref/allfiles.sgml @@ -205,6 +205,7 @@ Complete list of usable sgml source files in this directory. + diff --git a/doc/src/sgml/ref/pg_createsubscriber.sgml b/doc/src/sgml/ref/pg_createsubscriber.sgml new file mode 100644 index 00..642213b5a4 --- /dev/null +++ b/doc/src/sgml/ref/pg_createsubscriber.sgml @@ -0,0 +1,525 @@ + + + + + pg_createsubscriber + + + + pg_createsubscriber + 1 + Application + + + + pg_createsubscriber + convert a physical replica into a new logical replica + + + + + pg_createsubscriber + option + + + -d + --database + +dbname + + -D + --pgdata + +datadir + + -P + --publisher-server + +connstr + + + + + + Description + +pg_createsubscriber creates a new logical +replica from a physical standby server. All tables in the specified +database are included in the logical replication setup. A pair of +publication and subscription objects
Re: speed up a logical replica setup
Hi, > I'm attaching a new version (v30) that adds: > > * 3 new options (--publication, --subscription, --replication-slot) to assign > names to the objects. The --database option used to ignore duplicate names, > however, since these new options rely on the number of database options to > match the number of object name options, it is forbidden from now on. The > duplication is also forbidden for the object names to avoid errors earlier. > * rewrite the paragraph related to unusuable target server after > pg_createsubscriber fails. > * Vignesh reported an issue [1] related to reaching the recovery stop point > before the consistent state is reached. I proposed a simple patch that fixes > the issue. > > [1] > https://www.postgresql.org/message-id/CALDaNm3VMOi0GugGvhk3motghaFRKSWMCSE2t3YX1e%2BMttToxg%40mail.gmail.com > I have added a top-up patch v30-0003. The issue in [1] still exists in the v30 patch. I was not able to come up with an approach to handle it in the code, so I have added it to the documentation in the warning section. Thoughts? I am not changing the version as I have not made any changes in v30-0001 and v30-0002. [1]: https://www.postgresql.org/message-id/cahv8rj+5mzk9jt+7ecogjzfm5czvdccd5jo1_rcx0bteypb...@mail.gmail.com Thanks and regards, Shlok Kyal v30-0001-pg_createsubscriber-creates-a-new-logical-replic.patch Description: Binary data v30-0002-Stop-the-target-server-earlier.patch Description: Binary data v30-0003-Document-a-limitation-of-pg_createsubscriber.patch Description: Binary data
Re: Add publisher and subscriber to glossary documentation.
Hi Andrew, > If there's a movement towards "node" to refer to the database which has the > Subscription object, then perhaps the documentation for > > 31.2. Subscription, Chapter 31. Logical Replication should be updated as > well, since it uses both the "database" and "node" terms on the same page, > and to me referring to the same thing (I could be missing a subtlety). > > > See: > > > "The subscriber database..." > > > "A subscriber node may..." > > > Also, the word "database" in this sentence: "A subscription defines the > connection to another database" to me works, but I think using "node" there > could be more consistent if it’s referring to the server instance running the > database that holds the PUBLICATION. The connection string information > example later on the page shows "host" and "dbname" configured in the > CONNECTION value for the SUBSCRIPTION. This sentence seems like the use of > "database" in casual style to mean the "server instance" (or "node"). > > > Also, the "The node where a subscription is defined". That one actually feels > to me like "The database where a subscription is defined", but then that > contradicts what I just said, and "node" is fine here but I think "node" > should be on the preceding sentence too. > > > Anyway, hopefully these examples show “node” and “database” are mixed and > perhaps others agree using one consistently might help the goals of the docs. For me the existing content looks good, I felt let's keep it as it is unless others feel differently. Thanks and regards, Shlok Kyal
Re: speed up a logical replica setup
Hi,
> 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?
I investigated it and found that:
+ while ((c = getopt_long(argc, argv, "d:D:nP:rS:t:v",
+ long_options, &option_index)) != -1)
+ {
Here 'p', 's' and 'U' options are missing so we are getting the error.
Also, I think the 'S' option should be removed from here.
I also see that specifying long options like --subscriber-port,
--subscriber-username, --socket-directory works fine.
Thoughts?
Thanks and regards,
Shlok Kyal
Re: Add publisher and subscriber to glossary documentation.
> 1. > + > + Publication node > + > + > + A node where a > + linkend="glossary-publication">publication is defined > + for logical > replication. > + > + > + > + > > I felt the word "node" here should link to the glossary term "Node", > instead of directly to the term "Instance". > > ~~ > > 2. > + > + Subscription node > + > + > + A node where a > + linkend="glossary-subscription">subscription is defined > + for logical > replication. > + > + > + > + > > (same comment as above) > > I felt the word "node" here should link to the glossary term "Node", > instead of directly to the term "Instance". I have addressed the comments and have attached the updated version. Thanks and Regards, Shlok Kyal v4-0001-Add-publisher-and-subscriber-to-glossary-document.patch Description: Binary data
Re: Add publisher and subscriber to glossary documentation.
> Here are some comments for patch v2. > > == > > 1. There are whitespace problems > > [postgres@CentOS7-x64 oss_postgres_misc]$ git apply > ../patches_misc/v2-0001-Add-publisher-and-subscriber-to-glossary-document.patch > ../patches_misc/v2-0001-Add-publisher-and-subscriber-to-glossary-document.patch:43: > trailing whitespace. > A node where publication is defined for > ../patches_misc/v2-0001-Add-publisher-and-subscriber-to-glossary-document.patch:45: > trailing whitespace. > It replicates a set of changes from a table or a group of tables in > ../patches_misc/v2-0001-Add-publisher-and-subscriber-to-glossary-document.patch:66: > trailing whitespace. > A node where subscription is defined for > ../patches_misc/v2-0001-Add-publisher-and-subscriber-to-glossary-document.patch:67: > trailing whitespace. > logical > replication. > ../patches_misc/v2-0001-Add-publisher-and-subscriber-to-glossary-document.patch:68: > trailing whitespace. > It subscribes to one or more publications on a publisher node and pulls > warning: squelched 2 whitespace errors > warning: 7 lines add whitespace errors. > > ~~~ > 2. Publisher node > > + > + Publisher node > + > + > + A node where publication is defined for > + logical > replication. > + It replicates a set of changes from a table or a group of tables in > + publication to the subscriber node. > + > + > + For more information, see > + . > + > + > + > + > > > 2a. > I felt the term here should be "Publication node". > > Indeed, there should also be additional glossary terms like > "Publisher" (i.e. it is the same as "Publication node") and > "Subscriber" (i.e. it is the same as "Subscription node"). These > definitions will then be consistent with node descriptions already in > sections 30.1 and 30.2 > > > ~ > > 2b. > "node" should include a link to the glossary term. Same for any other > terms mentioned > > ~ > > 2c. > /A node where publication is defined for/A node where a publication is > defined for/ > > ~ > > 2d. > "It replicates" is misleading because it is the PUBLICATION doing the > replicating, not the node. > > IMO it will be better to include 2 more glossary terms "Publication" > and "Subscription" where you can say this kind of information. Then > the link "logical-replication-publication" also belongs under the > "Publication" term. > > > ~~~ > > 3. > + > + Subscriber node > + > + > + A node where subscription is defined for > + logical > replication. > + It subscribes to one or more publications on a publisher node and pulls > + a set of changes from a table or a group of tables in publications it > + subscribes to. > + > + > + For more information, see > + . > + > + > + > > All comments are similar to those above. > > == > > In summary, IMO it should be a bit more like below: > > SUGGESTION (include all the necessary links etc) > > Publisher > See "Publication node" > > Publication > A publication replicates the changes of one or more tables to a > subscription. For more information, see Section 30.1 > > Publication node > A node where a publication is defined for logical replication. > > Subscriber > See "Subscription node" > > Subscription > A subscription receives the changes of one or more tables from the > publications it subscribes to. For more information, see Section 30.2 > > Subscription node > A node where a subscription is defined for logical replication. I have addressed the comments and added an updated patch. Thanks and Regards, Shlok Kyal v3-0001-Add-publisher-and-subscriber-to-glossary-document.patch Description: Binary data
Re: speed up a logical replica setup
Hi,
On Tue, 20 Feb 2024 at 06:59, Euler Taveira wrote:
>
> On Mon, Feb 19, 2024, at 7:22 AM, Shlok Kyal wrote:
>
> I have reviewed the v21 patch. And found an issue.
>
> Initially I started the standby server with a new postgresql.conf file
> (not the default postgresql.conf that is present in the instance).
> pg_ctl -D ../standby start -o "-c config_file=/new_path/postgresql.conf"
>
> And I have made 'max_replication_slots = 1' in new postgresql.conf and
> made 'max_replication_slots = 0' in the default postgresql.conf file.
> Now when we run pg_createsubscriber on standby we get error:
> pg_createsubscriber: error: could not set replication progress for the
> subscription "pg_createsubscriber_5_242843": ERROR: cannot query or
> manipulate replication origin when max_replication_slots = 0
>
>
> That's by design. See [1]. The max_replication_slots parameter is used as the
> maximum number of subscriptions on the server.
>
> NOTICE: dropped replication slot "pg_createsubscriber_5_242843" on publisher
> pg_createsubscriber: error: could not drop publication
> "pg_createsubscriber_5" on database "postgres": ERROR: publication
> "pg_createsubscriber_5" does not exist
> pg_createsubscriber: error: could not drop replication slot
> "pg_createsubscriber_5_242843" on database "postgres": ERROR:
> replication slot "pg_createsubscriber_5_242843" does not exist
>
>
> That's a bug and should be fixed.
>
> [1]
> https://www.postgresql.org/docs/current/runtime-config-replication.html#RUNTIME-CONFIG-REPLICATION-SUBSCRIBER
>
I think you misunderstood the issue, I reported.
My main concern is that the standby server is using different
'postgresql.conf' file (the default file) after :
+ /* Start subscriber and wait until accepting connections */
+ pg_log_info("starting the subscriber");
+ if (!dry_run)
+ start_standby_server(pg_ctl_path, opt.subscriber_dir, server_start_log);
But we initially started the standby server (before running the
pg_createsubscriber) with a new postgresql.conf file (different from
the default file. for this example created inside the 'new_path'
folder).
pg_ctl -D ../standby -l standby.log start -o "-c
config_file=../new_path/postgresql.conf"
So, when we run the pg_createsubscriber, all the initial checks will
be run on the standby server started using the new postgresql.conf
file. But during pg_createsubscriber, it will restart the standby
server using the default postgresql.conf file. And this might create
some issues.
Thanks and Regards,
Shlok Kyal
Re: speed up a logical replica setup
Hi, I have reviewed the v21 patch. And found an issue. Initially I started the standby server with a new postgresql.conf file (not the default postgresql.conf that is present in the instance). pg_ctl -D ../standby start -o "-c config_file=/new_path/postgresql.conf" And I have made 'max_replication_slots = 1' in new postgresql.conf and made 'max_replication_slots = 0' in the default postgresql.conf file. Now when we run pg_createsubscriber on standby we get error: pg_createsubscriber: error: could not set replication progress for the subscription "pg_createsubscriber_5_242843": ERROR: cannot query or manipulate replication origin when max_replication_slots = 0 NOTICE: dropped replication slot "pg_createsubscriber_5_242843" on publisher pg_createsubscriber: error: could not drop publication "pg_createsubscriber_5" on database "postgres": ERROR: publication "pg_createsubscriber_5" does not exist pg_createsubscriber: error: could not drop replication slot "pg_createsubscriber_5_242843" on database "postgres": ERROR: replication slot "pg_createsubscriber_5_242843" does not exist I observed that when we run the pg_createsubscriber command, it will stop the standby instance (the non-default postgres configuration) and restart the standby instance which will now be started with default postgresql.conf, where the 'max_replication_slot = 0' and pg_createsubscriber will now fail with the error given above. I have added the script file with which we can reproduce this issue. Also similar issues can happen with other configurations such as port, etc. The possible solution would be 1) allow to run pg_createsubscriber if standby is initially stopped . I observed that pg_logical_createsubscriber also uses this approach. 2) read GUCs via SHOW command and restore them when server restarts I would prefer the 1st solution. Thanks and Regards, Shlok Kyal sudo pkill -9 postgres rm -rf ../primary ../standby ../new_path rm -rf primary.log standby.log ./initdb -D ../primary cat << EOF >> ../primary/postgresql.conf wal_level = 'logical' EOF ./pg_ctl -D ../primary -l primary.log start ./psql -d postgres -c "CREATE table t1 (c1 int);" ./psql -d postgres -c "Insert into t1 values(1);" ./psql -d postgres -c "Insert into t1 values(2);" ./psql -d postgres -c "INSERT into t1 values(3);" ./psql -d postgres -c "INSERT into t1 values(4);" ./pg_basebackup -h localhost -X stream -v -W -R -D ../standby/ # postgresql.conf file in new location mkdir ../new_path cat << EOF >> ../new_path/postgresql.conf max_replication_slots = 1 port = 9000 EOF # postgresql.conf file in default location cat << EOF >> ../standby/postgresql.conf max_replication_slots = 0 port = 9000 EOF ./pg_ctl -D ../standby -l standby.log start -o "-c config_file=../new_path/postgresql.conf" ./pg_createsubscriber -D ../standby -S 'host=localhost port=9000 dbname=postgres' -P 'host=localhost port=5432 dbname=postgres' -d postgres -r
Re: Add publisher and subscriber to glossary documentation.
Hi, I addressed the comments and updated the patch. > Should these be "publisher node" and "subscriber node" instead? Do we > want to define the term "node"? I think in everyday conversations we > use "node" quite a lot, so maybe we do need an entry for it. (Maybe > just suffices, plus add under instance > "also called a node".) Modified > + Publisher > + > + > + A node where publication is defined. > + It replicates the publication changes to the subscriber node. > > Apart from deciding what to do with "node", what are "changes"? This > doesn't seem very specific. Modified > + Subscriber > + > + > + A node where subscription is defined. > + It subscribe to one or more publications on a publisher node and pull > the data > + from the publications they subscribe to. > > Same issues as above, plus there are some grammar issues. Modified > I think these definitions should use the term "logical replication", > which we don't currently define. We do have "replication" where we > provide an overview of "logical replication". Maybe that's enough, but > we should consider whether we want a separate definition of logical > replication (I'm leaning towards not having one, but it's worth asking.) Modified. Added the term "logical replication" in the definitions. Used reference to "replication". Thanks and Regards, Shlok Kyal v2-0001-Add-publisher-and-subscriber-to-glossary-document.patch Description: Binary data
Add publisher and subscriber to glossary documentation.
Hi, There are several places where publisher and subscriber terms are used across the documentation. But the publisher and subscriber were missing in the documentation. I felt this should be added in the glossary. I have created a patch for the same. Thanks and Regards Shlok Kyal v1-0001-Add-publisher-and-subscriber-to-glossary-document.patch Description: Binary data
Re: speed up a logical replica setup
Hi, I have some comments for the v5 patch: 1. ``` + base_dir = (char *) pg_malloc0(MAXPGPATH); + len = snprintf(base_dir, MAXPGPATH, "%s/%s", subscriber_dir, PGS_OUTPUT_DIR); ``` Before these lines, I think we should use 'canonicalize_path(subscriber_dir)' to remove extra unnecessary characters. This function is used in many places like initdb.c, pg_ctl.c, pg_basebakup.c, etc 2. I also feels that there are many global variables and can be arranged as structures as suggested by Kuroda-san in [1] [1] https://www.postgresql.org/message-id/TY3PR01MB9889C362FF76102C88FA1C29F56F2%40TY3PR01MB9889.jpnprd01.prod.outlook.com Thanks and Regards Shlok Kyal On Fri, 12 Jan 2024 at 03:46, Euler Taveira wrote: > > On Thu, Jan 11, 2024, at 9:18 AM, Hayato Kuroda (Fujitsu) wrote: > > I have been concerned that the patch has not been tested by cfbot due to the > application error. Also, some comments were raised. Therefore, I created a > patch > to move forward. > > > Let me send an updated patch to hopefully keep the CF bot happy. The following > items are included in this patch: > > * drop physical replication slot if standby is using one [1]. > * cleanup small changes (copyright, .gitignore) [2][3] > * fix getopt_long() options [2] > * fix format specifier for some messages > * move doc to Server Application section [4] > * fix assert failure > * ignore duplicate database names [2] > * store subscriber server log into a separate file > * remove MSVC support > > I'm still addressing other reviews and I'll post another version that includes > it soon. > > [1] > https://www.postgresql.org/message-id/e02a2c17-22e5-4ba6-b788-de696ab74f1e%40app.fastmail.com > [2] > https://www.postgresql.org/message-id/CALDaNm1joke42n68LdegN5wCpaeoOMex2EHcdZrVZnGD3UhfNQ%40mail.gmail.com > [3] > https://www.postgresql.org/message-id/TY3PR01MB98895BA6C1D72CB8582CACC4F5682%40TY3PR01MB9889.jpnprd01.prod.outlook.com > [4] > https://www.postgresql.org/message-id/TY3PR01MB988978C7362A101927070D29F56A2%40TY3PR01MB9889.jpnprd01.prod.outlook.com > > > -- > Euler Taveira > EDB https://www.enterprisedb.com/ >
Re: Extend pgbench partitioning to pgbench_history
Sorry, I did not intend to send this message for this email. I by mistake sent this mail. Please ignore this mail On Wed, 10 Jan 2024 at 15:27, Shlok Kyal wrote: > > Hi, > > There are some test failures reported by Cfbot at [1]: > > [09:15:01.794] 192/276 postgresql:pgbench / > pgbench/001_pgbench_with_server ERROR 7.48s exit status 3 > [09:15:01.794] >>> > INITDB_TEMPLATE=/tmp/cirrus-ci-build/build/tmp_install/initdb-template > LD_LIBRARY_PATH=/tmp/cirrus-ci-build/build/tmp_install//usr/local/pgsql/lib > REGRESS_SHLIB=/tmp/cirrus-ci-build/build/src/test/regress/regress.so > PATH=/tmp/cirrus-ci-build/build/tmp_install//usr/local/pgsql/bin:/tmp/cirrus-ci-build/build/src/bin/pgbench:/tmp/cirrus-ci-build/build/src/bin/pgbench/test:/usr/local/bin:/sbin:/bin:/usr/sbin:/usr/bin > PG_REGRESS=/tmp/cirrus-ci-build/build/src/test/regress/pg_regress > MALLOC_PERTURB_=67 /usr/local/bin/python3 > /tmp/cirrus-ci-build/build/../src/tools/testwrap --basedir > /tmp/cirrus-ci-build/build --srcdir > /tmp/cirrus-ci-build/src/bin/pgbench --testgroup pgbench --testname > 001_pgbench_with_server -- /usr/local/bin/perl -I > /tmp/cirrus-ci-build/src/test/perl -I > /tmp/cirrus-ci-build/src/bin/pgbench > /tmp/cirrus-ci-build/src/bin/pgbench/t/001_pgbench_with_server.pl > [09:15:01.794] ― ✀ > ― > [09:15:01.794] stderr: > [09:15:01.794] # Failed test 'transaction format for 001_pgbench_log_2' > [09:15:01.794] # at > /tmp/cirrus-ci-build/src/bin/pgbench/t/001_pgbench_with_server.pl line > 1247. > [09:15:01.794] # Failed test 'transaction count for > /tmp/cirrus-ci-build/build/testrun/pgbench/001_pgbench_with_server/data/t_001_pgbench_with_server_main_data/001_pgbench_log_3.25193 > (11)' > [09:15:01.794] # at > /tmp/cirrus-ci-build/src/bin/pgbench/t/001_pgbench_with_server.pl line > 1257. > [09:15:01.794] # Failed test 'transaction format for 001_pgbench_log_3' > [09:15:01.794] # at > /tmp/cirrus-ci-build/src/bin/pgbench/t/001_pgbench_with_server.pl line > 1257. > [09:15:01.794] # Looks like you failed 3 tests of 439. > [09:15:01.794] > [09:15:01.794] (test program exited with status code 3) > > [1] - https://cirrus-ci.com/task/5139049757802496 > > Thanks and regards > Shlok Kyal
Re: Extend pgbench partitioning to pgbench_history
Hi, There are some test failures reported by Cfbot at [1]: [09:15:01.794] 192/276 postgresql:pgbench / pgbench/001_pgbench_with_server ERROR 7.48s exit status 3 [09:15:01.794] >>> INITDB_TEMPLATE=/tmp/cirrus-ci-build/build/tmp_install/initdb-template LD_LIBRARY_PATH=/tmp/cirrus-ci-build/build/tmp_install//usr/local/pgsql/lib REGRESS_SHLIB=/tmp/cirrus-ci-build/build/src/test/regress/regress.so PATH=/tmp/cirrus-ci-build/build/tmp_install//usr/local/pgsql/bin:/tmp/cirrus-ci-build/build/src/bin/pgbench:/tmp/cirrus-ci-build/build/src/bin/pgbench/test:/usr/local/bin:/sbin:/bin:/usr/sbin:/usr/bin PG_REGRESS=/tmp/cirrus-ci-build/build/src/test/regress/pg_regress MALLOC_PERTURB_=67 /usr/local/bin/python3 /tmp/cirrus-ci-build/build/../src/tools/testwrap --basedir /tmp/cirrus-ci-build/build --srcdir /tmp/cirrus-ci-build/src/bin/pgbench --testgroup pgbench --testname 001_pgbench_with_server -- /usr/local/bin/perl -I /tmp/cirrus-ci-build/src/test/perl -I /tmp/cirrus-ci-build/src/bin/pgbench /tmp/cirrus-ci-build/src/bin/pgbench/t/001_pgbench_with_server.pl [09:15:01.794] ― ✀ ― [09:15:01.794] stderr: [09:15:01.794] # Failed test 'transaction format for 001_pgbench_log_2' [09:15:01.794] # at /tmp/cirrus-ci-build/src/bin/pgbench/t/001_pgbench_with_server.pl line 1247. [09:15:01.794] # Failed test 'transaction count for /tmp/cirrus-ci-build/build/testrun/pgbench/001_pgbench_with_server/data/t_001_pgbench_with_server_main_data/001_pgbench_log_3.25193 (11)' [09:15:01.794] # at /tmp/cirrus-ci-build/src/bin/pgbench/t/001_pgbench_with_server.pl line 1257. [09:15:01.794] # Failed test 'transaction format for 001_pgbench_log_3' [09:15:01.794] # at /tmp/cirrus-ci-build/src/bin/pgbench/t/001_pgbench_with_server.pl line 1257. [09:15:01.794] # Looks like you failed 3 tests of 439. [09:15:01.794] [09:15:01.794] (test program exited with status code 3) [1] - https://cirrus-ci.com/task/5139049757802496 Thanks and regards Shlok Kyal
Re: [PATCH] Reuse Workers and Replication Slots during Logical Replication
Hi, This patch is not applying on the HEAD. Please rebase and share the updated patch. Thanks and Regards Shlok Kyal On Wed, 10 Jan 2024 at 14:55, Peter Smith wrote: > > Oops - now with attachments > > On Mon, Aug 21, 2023 at 5:56 PM Peter Smith wrote: >> >> Hi Melih, >> >> Last week we revisited your implementation of design#2. Vignesh rebased it, >> and then made a few other changes. >> >> PSA v28* >> >> The patch changes include: >> * changed the logic slightly by setting recv_immediately(new variable), if >> this variable is set the main apply worker loop will not wait in this case. >> * setting the relation state to ready immediately if there are no more >> incremental changes to be synced. >> * receive the incremental changes if applicable and set the relation state >> to ready without waiting. >> * reuse the worker if the worker is free before trying to start a new table >> sync worker >> * restarting the tablesync worker only after wal_retrieve_retry_interval >> >> ~ >> >> FWIW, we just wanted to share with you the performance measurements seen >> using this design#2 patch set: >> >> == >> >> RESULTS (not busy tests) >> >> -- >> 10 empty tables >> 2w 4w 8w 16w >> HEAD: 125 119 140 133 >> HEAD+v28*: 92 93 123 134 >> %improvement: 27% 22% 12% -1% >> -- >> 100 empty tables >> 2w 4w 8w 16w >> HEAD: 1037843 11091155 >> HEAD+v28*: 591 625 26162569 >> %improvement: 43% 26% -136% -122% >> -- >> 1000 empty tables >> 2w 4w 8w 16w >> HEAD: 15874 10047 991910338 >> HEAD+v28*: 33673 12199 90949896 >> %improvement: -112% -21%8% 4% >> -- >> 2000 empty tables >> 2w 4w 8w 16w >> HEAD: 45266 24216 19395 19820 >> HEAD+v28*: 88043 21550 21668 22607 >> %improvement: -95% 11% -12%-14% >> >> ~~~ >> >> Note - the results were varying quite a lot in comparison to the HEAD >> e.g. HEAD results are very consistent, but the v28* results observed are not >> HEAD 1000 (2w): 15861, 15777, 16007, 15950, 15886, 15740, 15846, 15740, >> 15908, 15940 >> v28* 1000 (2w): 34214, 13679, 8792, 33289, 31976, 56071, 57042, 56163, >> 34058, 11969 >> >> -- >> Kind Regards, >> Peter Smith. >> Fujitsu Australia
Re: speed up a logical replica setup
On Fri, 5 Jan 2024 at 12:19, Shlok Kyal wrote:
>
> On Thu, 4 Jan 2024 at 16:46, Amit Kapila wrote:
> >
> > On Thu, Jan 4, 2024 at 12:22 PM Shlok Kyal wrote:
> > >
> > > Hi,
> > > I was testing the patch with following test cases:
> > >
> > > Test 1 :
> > > - Create a 'primary' node
> > > - Setup physical replica using pg_basebackup "./pg_basebackup –h
> > > localhost –X stream –v –R –W –D ../standby "
> > > - Insert data before and after pg_basebackup
> > > - Run pg_subscriber and then insert some data to check logical
> > > replication "./pg_subscriber –D ../standby -S “host=localhost
> > > port=9000 dbname=postgres” -P “host=localhost port=9000
> > > dbname=postgres” -d postgres"
> > > - Also check pg_publication, pg_subscriber and pg_replication_slots
> > > tables.
> > >
> > > Observation:
> > > Data is not lost. Replication is happening correctly. Pg_subscriber is
> > > working as expected.
> > >
> > > Test 2:
> > > - Create a 'primary' node
> > > - Use normal pg_basebackup but don’t set up Physical replication
> > > "./pg_basebackup –h localhost –v –W –D ../standby"
> > > - Insert data before and after pg_basebackup
> > > - Run pg_subscriber
> > >
> > > Observation:
> > > Pg_subscriber command is not completing and is stuck with following
> > > log repeating:
> > > LOG: waiting for WAL to become available at 0/3000168
> > > LOG: invalid record length at 0/3000150: expected at least 24, got 0
> > >
> >
> > I think probably the required WAL is not copied. Can you use the -X
> > option to stream WAL as well and then test? But I feel in this case
> > also, we should wait for some threshold time and then exit with
> > failure, removing new objects created, if any.
>
> I have tested with -X stream option in pg_basebackup as well. In this
> case also the pg_subscriber command is getting stuck.
> logs:
> 2024-01-05 11:49:34.436 IST [61948] LOG: invalid resource manager ID
> 102 at 0/3000118
> 2024-01-05 11:49:34.436 IST [61948] LOG: waiting for WAL to become
> available at 0/3000130
>
> >
> > > Test 3:
> > > - Create a 'primary' node
> > > - Use normal pg_basebackup but don’t set up Physical replication
> > > "./pg_basebackup –h localhost –v –W –D ../standby"
> > > -Insert data before pg_basebackup but not after pg_basebackup
> > > -Run pg_subscriber
> > >
> > > Observation:
> > > Pg_subscriber command is not completing and is stuck with following
> > > log repeating:
> > > LOG: waiting for WAL to become available at 0/3000168
> > > LOG: invalid record length at 0/3000150: expected at least 24, got 0
> > >
> >
> > This is similar to the previous test and you can try the same option
> > here as well.
> For this test as well tried with -X stream option in pg_basebackup.
> It is getting stuck here as well with similar log.
>
> Will investigate the issue further.
I noticed that the pg_subscriber get stuck when we run it on node
which is not a standby. It is because the of the code:
+ conn = connect_database(dbinfo[0].pubconninfo);
+ if (conn == NULL)
+ exit(1);
+ consistent_lsn = create_logical_replication_slot(conn, &dbinfo[0],
+temp_replslot);
+
.
+else
+ {
+ appendPQExpBuffer(recoveryconfcontents, "recovery_target_lsn = '%s'\n",
+ consistent_lsn);
+ WriteRecoveryConfig(conn, subscriber_dir, recoveryconfcontents);
+ }
Here the standby node would be waiting for the 'consistent_lsn' wal
during recovery but this wal will not be present on standby if no
physical replication is setup. Hence the command will be waiting
infinitely for the wal.
To solve this added a timeout of 60s for the recovery process and also
added a check so that pg_subscriber would give a error when it called
for node which is not in physical replication.
Have attached the patch for the same. It is a top-up patch of the
patch shared by Euler at [1].
Please review the changes and merge the changes if it looks ok.
[1] -
https://www.postgresql.org/message-id/e02a2c17-22e5-4ba6-b788-de696ab74f1e%40app.fastmail.com
Thanks and regards
Shlok Kyal
v1-0001-Restrict-pg_subscriber-to-standby-node.patch
Description: Binary data
Re: speed up a logical replica setup
On Thu, 4 Jan 2024 at 16:46, Amit Kapila wrote: > > On Thu, Jan 4, 2024 at 12:22 PM Shlok Kyal wrote: > > > > Hi, > > I was testing the patch with following test cases: > > > > Test 1 : > > - Create a 'primary' node > > - Setup physical replica using pg_basebackup "./pg_basebackup –h > > localhost –X stream –v –R –W –D ../standby " > > - Insert data before and after pg_basebackup > > - Run pg_subscriber and then insert some data to check logical > > replication "./pg_subscriber –D ../standby -S “host=localhost > > port=9000 dbname=postgres” -P “host=localhost port=9000 > > dbname=postgres” -d postgres" > > - Also check pg_publication, pg_subscriber and pg_replication_slots tables. > > > > Observation: > > Data is not lost. Replication is happening correctly. Pg_subscriber is > > working as expected. > > > > Test 2: > > - Create a 'primary' node > > - Use normal pg_basebackup but don’t set up Physical replication > > "./pg_basebackup –h localhost –v –W –D ../standby" > > - Insert data before and after pg_basebackup > > - Run pg_subscriber > > > > Observation: > > Pg_subscriber command is not completing and is stuck with following > > log repeating: > > LOG: waiting for WAL to become available at 0/3000168 > > LOG: invalid record length at 0/3000150: expected at least 24, got 0 > > > > I think probably the required WAL is not copied. Can you use the -X > option to stream WAL as well and then test? But I feel in this case > also, we should wait for some threshold time and then exit with > failure, removing new objects created, if any. I have tested with -X stream option in pg_basebackup as well. In this case also the pg_subscriber command is getting stuck. logs: 2024-01-05 11:49:34.436 IST [61948] LOG: invalid resource manager ID 102 at 0/3000118 2024-01-05 11:49:34.436 IST [61948] LOG: waiting for WAL to become available at 0/3000130 > > > Test 3: > > - Create a 'primary' node > > - Use normal pg_basebackup but don’t set up Physical replication > > "./pg_basebackup –h localhost –v –W –D ../standby" > > -Insert data before pg_basebackup but not after pg_basebackup > > -Run pg_subscriber > > > > Observation: > > Pg_subscriber command is not completing and is stuck with following > > log repeating: > > LOG: waiting for WAL to become available at 0/3000168 > > LOG: invalid record length at 0/3000150: expected at least 24, got 0 > > > > This is similar to the previous test and you can try the same option > here as well. For this test as well tried with -X stream option in pg_basebackup. It is getting stuck here as well with similar log. Will investigate the issue further. Thanks and regards Shlok Kyal
Re: speed up a logical replica setup
Hi, I was testing the patch with following test cases: Test 1 : - Create a 'primary' node - Setup physical replica using pg_basebackup "./pg_basebackup –h localhost –X stream –v –R –W –D ../standby " - Insert data before and after pg_basebackup - Run pg_subscriber and then insert some data to check logical replication "./pg_subscriber –D ../standby -S “host=localhost port=9000 dbname=postgres” -P “host=localhost port=9000 dbname=postgres” -d postgres" - Also check pg_publication, pg_subscriber and pg_replication_slots tables. Observation: Data is not lost. Replication is happening correctly. Pg_subscriber is working as expected. Test 2: - Create a 'primary' node - Use normal pg_basebackup but don’t set up Physical replication "./pg_basebackup –h localhost –v –W –D ../standby" - Insert data before and after pg_basebackup - Run pg_subscriber Observation: Pg_subscriber command is not completing and is stuck with following log repeating: LOG: waiting for WAL to become available at 0/3000168 LOG: invalid record length at 0/3000150: expected at least 24, got 0 Test 3: - Create a 'primary' node - Use normal pg_basebackup but don’t set up Physical replication "./pg_basebackup –h localhost –v –W –D ../standby" -Insert data before pg_basebackup but not after pg_basebackup -Run pg_subscriber Observation: Pg_subscriber command is not completing and is stuck with following log repeating: LOG: waiting for WAL to become available at 0/3000168 LOG: invalid record length at 0/3000150: expected at least 24, got 0 I was not clear about how to use pg_basebackup in this case, can you let me know if any changes need to be made for test2 and test3. Thanks and regards Shlok Kyal
Re: Intermittent failure with t/003_logical_slots.pl test on windows
Hi,
Apart of these I am getting following some intermittent failure as below:
131/272 postgresql:pg_basebackup / pg_basebackup/010_pg_basebackup
ERROR30.51s (exit status 255 or
signal 127 SIGinvalid)
114/272 postgresql:libpq / libpq/001_uri
ERROR 9.66s exit status 8
34/272 postgresql:pg_upgrade / pg_upgrade/003_logical_slots
ERROR99.14s exit status 1
186/272 postgresql:pg_upgrade / pg_upgrade/002_pg_upgrade
ERROR 306.22s exit status 1
29/272 postgresql:recovery / recovery/002_archiving
ERROR89.62s (exit status 255 or
signal 127 SIGinvalid)
138/272 postgresql:pg_resetwal / pg_resetwal/001_basic
ERROR 3.93s (exit status 255 or
signal 127 SIGinvalid)
Have attached the regress logs for the same as well.
Thanks and Regards
Shlok Kyal
On Tue, 26 Dec 2023 at 17:39, Shlok Kyal wrote:
>
> Hi,
> The same intermittent failure is reproducible on my system.
> For the intermittent issues I found that many issues are due to errors
> where commands like 'psql -V' are not returning any output.
> To reproduce it in an easy way, I wrote a script (.bat file) with
> '--version' option for different binaries. And found out that it was
> not giving any output for some command (varies for each run).
> Then I tried to run the same script after adding 'fflush(stdout)' in
> the function called with '--version' option and it started to give
> output for each command.
> I noticed the same for '--help' option and did the changes for the same.
>
> I have attached the test script(changes the extension to .txt as gmail
> is blocking it), output of test before the changes.
> I have also attached the patch with changes which resolved the above issue.
>
> This change has resolved most of the intermittent issues for me. I am
> facing some more intermittent issues. Will analyse and share it as
> well.
>
> Thanks and regards
> Shlok Kyal
>
> On Tue, 7 Nov 2023 at 11:05, Kyotaro Horiguchi
> wrote:
> >
> > At Mon, 6 Nov 2023 19:42:21 +0530, Nisha Moond
> > wrote in
> > > > Appending '2>&1 test:
> > > > The command still results in NULL and ends up failing as no data is
> > > > returned. Which means even no error message is returned. The error log
> >
> > Thanks for confirmation. So, at least the child process was launced
> > successfully in the cmd.exe's view.
> >
> > Upon a quick check on my end with Windows' _popen, I have obseved the
> > following:
> >
> > - Once a child process is started, it seems to go undetected as an
> > error by _popen or subsequent fgets calls if the process ends
> > abnormally, with a non-zero exit status or even with a SEGV.
> >
> > - After the child process has flushed data to stdout, it is possible
> > to read from the pipe even if the child process crashes or ends
> > thereafter.
> >
> > - Even if fgets is called before the program starts, it will correctly
> > block until the program outputs something. Specifically, when I used
> > popen("sleep 5 & target.exe") and immediately performed fgets on the
> > pipe, I was able to read the output of target.exe as the first line.
> >
> > Therefore, based on the information available, it is conceivable that
> > the child process was killed by something right after it started, or
> > the program terminated on its own without any error messages.
> >
> > By the way, in the case of aforementioned SEGV, Application Errors
> > corresponding to it were identifiable in the Event
> > Viewer. Additionally, regarding the exit statuses, they can be
> > captured by using a wrapper batch file (.bat) that records
> > %ERRORLEVEL% after running the target program. This may yield
> > insights, aothough its effectiveness is not guaranteed.
> >
> > regards.
> >
> > --
> > Kyotaro Horiguchi
> > NTT Open Source Software Center
> >
> >
regress_log_001_basic
Description: Binary data
regress_log_003_logical_slots
Description: Binary data
regress_log_002_archiving
Description: Binary data
regress_log_002_pg_upgrade
Description: Binary data
regress_log_001_uri
Description: Binary data
regress_log_010_pg_basebackup
Description: Binary data
Re: Intermittent failure with t/003_logical_slots.pl test on windows
Hi,
The same intermittent failure is reproducible on my system.
For the intermittent issues I found that many issues are due to errors
where commands like 'psql -V' are not returning any output.
To reproduce it in an easy way, I wrote a script (.bat file) with
'--version' option for different binaries. And found out that it was
not giving any output for some command (varies for each run).
Then I tried to run the same script after adding 'fflush(stdout)' in
the function called with '--version' option and it started to give
output for each command.
I noticed the same for '--help' option and did the changes for the same.
I have attached the test script(changes the extension to .txt as gmail
is blocking it), output of test before the changes.
I have also attached the patch with changes which resolved the above issue.
This change has resolved most of the intermittent issues for me. I am
facing some more intermittent issues. Will analyse and share it as
well.
Thanks and regards
Shlok Kyal
On Tue, 7 Nov 2023 at 11:05, Kyotaro Horiguchi wrote:
>
> At Mon, 6 Nov 2023 19:42:21 +0530, Nisha Moond
> wrote in
> > > Appending '2>&1 test:
> > > The command still results in NULL and ends up failing as no data is
> > > returned. Which means even no error message is returned. The error log
>
> Thanks for confirmation. So, at least the child process was launced
> successfully in the cmd.exe's view.
>
> Upon a quick check on my end with Windows' _popen, I have obseved the
> following:
>
> - Once a child process is started, it seems to go undetected as an
> error by _popen or subsequent fgets calls if the process ends
> abnormally, with a non-zero exit status or even with a SEGV.
>
> - After the child process has flushed data to stdout, it is possible
> to read from the pipe even if the child process crashes or ends
> thereafter.
>
> - Even if fgets is called before the program starts, it will correctly
> block until the program outputs something. Specifically, when I used
> popen("sleep 5 & target.exe") and immediately performed fgets on the
> pipe, I was able to read the output of target.exe as the first line.
>
> Therefore, based on the information available, it is conceivable that
> the child process was killed by something right after it started, or
> the program terminated on its own without any error messages.
>
> By the way, in the case of aforementioned SEGV, Application Errors
> corresponding to it were identifiable in the Event
> Viewer. Additionally, regarding the exit statuses, they can be
> captured by using a wrapper batch file (.bat) that records
> %ERRORLEVEL% after running the target program. This may yield
> insights, aothough its effectiveness is not guaranteed.
>
> regards.
>
> --
> Kyotaro Horiguchi
> NTT Open Source Software Center
>
>
D:\project\pg_meson_64\bin>pg_ctl -V
pg_ctl (PostgreSQL) 17devel
D:\project\pg_meson_64\bin>pg_controldata -V
pg_controldata (PostgreSQL) 17devel
D:\project\pg_meson_64\bin>pg_dump -V
pg_dump (PostgreSQL) 17devel
D:\project\pg_meson_64\bin>pg_dumpall -V
pg_dumpall (PostgreSQL) 17devel
D:\project\pg_meson_64\bin>pg_ctl -V
pg_ctl (PostgreSQL) 17devel
D:\project\pg_meson_64\bin>pg_controldata -V
pg_controldata (PostgreSQL) 17devel
D:\project\pg_meson_64\bin>pg_dump -V
pg_dump (PostgreSQL) 17devel
D:\project\pg_meson_64\bin>pg_dumpall -V
pg_dumpall (PostgreSQL) 17devel
D:\project\pg_meson_64\bin>pg_ctl -V
pg_ctl (PostgreSQL) 17devel
D:\project\pg_meson_64\bin>pg_controldata -V
pg_controldata (PostgreSQL) 17devel
D:\project\pg_meson_64\bin>pg_dump -V
pg_dump (PostgreSQL) 17devel
D:\project\pg_meson_64\bin>pg_dumpall -V
pg_dumpall (PostgreSQL) 17devel
D:\project\pg_meson_64\bin>pg_ctl -V
pg_ctl (PostgreSQL) 17devel
D:\project\pg_meson_64\bin>pg_controldata -V
pg_controldata (PostgreSQL) 17devel
D:\project\pg_meson_64\bin>pg_dump -V
pg_dump (PostgreSQL) 17devel
D:\project\pg_meson_64\bin>pg_dumpall -V
pg_dumpall (PostgreSQL) 17devel
D:\project\pg_meson_64\bin>pg_ctl -V
pg_ctl (PostgreSQL) 17devel
D:\project\pg_meson_64\bin>pg_controldata -V
pg_controldata (PostgreSQL) 17devel
D:\project\pg_meson_64\bin>pg_dump -V
pg_dump (PostgreSQL) 17devel
D:\project\pg_meson_64\bin>pg_dumpall -V
pg_dumpall (PostgreSQL) 17devel
D:\project\pg_meson_64\bin>pg_ctl -V
pg_ctl (PostgreSQL) 17devel
D:\project\pg_meson_64\bin>pg_controldata -V
D:\project\pg_meson_64\bin>pg_dump -V
pg_dump (PostgreSQL) 17devel
D:\project\pg_meson_64\bin>pg_dumpall -V
pg_dumpall (PostgreSQL) 17devel
D:\project\pg_meson_64\bin>pg_ctl -V
pg_ctl (PostgreSQL) 17devel
D:\project\pg_meson_64\bin>pg_controldata -V
Re: speed up a logical replica setup
Hi, On Wed, 6 Dec 2023 at 12:53, Euler Taveira wrote: > > On Thu, Nov 9, 2023, at 8:12 PM, Michael Paquier wrote: > > On Thu, Nov 09, 2023 at 03:41:53PM +0100, Peter Eisentraut wrote: > > On 08.11.23 00:12, Michael Paquier wrote: > >> - Should the subdirectory pg_basebackup be renamed into something more > >> generic at this point? All these things are frontend tools that deal > >> in some way with the replication protocol to do their work. Say > >> a replication_tools? > > > > Seems like unnecessary churn. Nobody has complained about any of the other > > tools in there. > > Not sure. We rename things across releases in the tree from time to > time, and here that's straight-forward. > > > Based on this discussion it seems we have a consensus that this tool should be > in the pg_basebackup directory. (If/when we agree with the directory renaming, > it could be done in a separate patch.) Besides this move, the v3 provides a > dry > run mode. It basically executes every routine but skip when should do > modifications. It is an useful option to check if you will be able to run it > without having issues with connectivity, permission, and existing objects > (replication slots, publications, subscriptions). Tests were slightly > improved. > Messages were changed to *not* provide INFO messages by default and --verbose > provides INFO messages and --verbose --verbose also provides DEBUG messages. I > also refactored the connect_database() function into which the connection will > always use the logical replication mode. A bug was fixed in the transient > replication slot name. Ashutosh review [1] was included. The code was also > indented. > > There are a few suggestions from Ashutosh [2] that I will reply in another > email. > > I'm still planning to work on the following points: > > 1. improve the cleanup routine to point out leftover objects if there is any >connection issue. > 2. remove the physical replication slot if the standby is using one >(primary_slot_name). > 3. provide instructions to promote the logical replica into primary, I mean, >stop the replication between the nodes and remove the replication setup >(publications, subscriptions, replication slots). Or even include another >action to do it. We could add both too. > > Point 1 should be done. Points 2 and 3 aren't essential but will provide a > nice > UI for users that would like to use it. > > > [1] > https://postgr.es/m/CAExHW5sCAU3NvPKd7msScQKvrBN-x_AdDQD-ZYAwOxuWG%3Doz1w%40mail.gmail.com > [2] > https://postgr.es/m/caexhw5vhfemfvtuhe+7xwphvzjxrexz5h3dd4uqi7cwmdmj...@mail.gmail.com > The changes in the file 'src/tools/msvc/Mkvcbuild.pm' seems unnecessary as the folder 'msvc' is removed due to the commit [1]. To review the changes, I did 'git reset --hard' to the commit previous to commit [1]. I tried to build the postgres on my Windows machine using two methods: i. building using Visual Studio ii. building using Meson When I built the code using Visual Studio, on installing postgres, pg_subscriber binary was not created. But when I built the code using Meson, on installing postgres, pg_subscriber binary was created. Is this behaviour intentional? [1] https://github.com/postgres/postgres/commit/1301c80b2167feb658a738fa4ceb1c23d0991e23 Thanks and Regards, Shlok Kyal
Re: undetected deadlock in ALTER SUBSCRIPTION ... REFRESH PUBLICATION
Hi, > Then let's go with the original patch only. BTW, it took almost the > same time (105 wallclock secs) in my environment (CentOs VM) to run > tests in src/test/subscription both with and without the patch. I took > a median of five runs. I have slightly adjusted the comments and > commit message in the attached. If you are fine with this, we can > commit and backpatch this. I have tested the patch for all the branches from PG 17 to PG 12. The same patch applies cleanly on all branches. Also, the same patch resolves the issue on all the branches. I ran all the tests and all the tests passed on each branch. I also reviewed the patch and it looks good to me. Thanks and Regards, Shlok Kyal
Re: undetected deadlock in ALTER SUBSCRIPTION ... REFRESH PUBLICATION
Hi,
> I mean to commit the open transaction at the below place in
> wait_for_relation_state_change()
>
> wait_for_relation_state_change()
> {
> ...
> -- commit the xact
> WaitLatch();
> ...
> }
>
> Then start after the wait is over. This is just to test whether it
> improves the difference in regression test timing.
I tried the above approach and observed that the performance of this
approach is nearly same as the previous approach.
For Linux VM:
Summary| Subscription | 100 tables in pub | 1000 tables in pub
| Test (sec) | and Sub (sec) | and Sub (sec)
--
old patch | 107.4545 | 6.911 | 77.918
alternate | 108.3985| 6.9835 | 78.111
approach
For Performance Machine:
Summary| Subscription | 100 tables in pub | 1000 tables in pub
| Test (sec) | and Sub (sec) | and Sub (sec)
--
old patch | 115.922 | 6.7305 | 81.1525
alternate | 115.8215 | 6.7685| 81.2335
approach
I have attached the patch for this approach as 'alternate_approach.patch'.
Since the performance is the same, I think that the previous approach
is better. As in this approach we are using CommitTransactionCommand()
and StartTransactionCommand() inside a 'for loop'.
I also fixed the comment in previous approach and attached here as
'v2-0001-Deadlock-when-apply-worker-tablesync-worker-and-c.patch'
Thanks and Regards
Shlok Kyal
From 11072d138d900227b963b54d1a3626cf256db721 Mon Sep 17 00:00:00 2001
From: Shlok Kyal
Date: Fri, 24 Nov 2023 16:37:14 +0530
Subject: [PATCH v2] Deadlock when apply worker,tablesync worker and client
backend run parallelly.
Apply worker holds a lock on the table pg_subscription_rel and waits for
notification from the tablesync workers where the relation is synced, which
happens through updating the pg_subscription_rel row. And that wait happens in
wait_for_relation_state_change, which simply checks the row in a loop, with a
sleep by WaitLatch(). Unfortunately, the tablesync workers can't update the row
because the client backend executing ALTER SUBSCRIPTION ... REFRESH PUBLICATION
sneaked in, and waits for an AccessExclusiveLock. So the tablesync workers are
stuck in the queue and can't proceed.
The client backend is waiting for a lock held by the apply worker. The tablesync
workers can't proceed because their lock request is stuck behind the
AccessExclusiveLock request. And the apply worker is waiting for status update
from the tablesync workers. And the deadlock is undetected, because the apply
worker is not waiting on a lock, but sleeping on a latch.
We have resolved the issue by releasing the lock by commiting the transaction
before the apply worker goes to wait state.
---
src/backend/replication/logical/tablesync.c | 17 ++---
1 file changed, 14 insertions(+), 3 deletions(-)
diff --git a/src/backend/replication/logical/tablesync.c b/src/backend/replication/logical/tablesync.c
index df3c42eb5d..b71234d998 100644
--- a/src/backend/replication/logical/tablesync.c
+++ b/src/backend/replication/logical/tablesync.c
@@ -542,15 +542,26 @@ process_syncing_tables_for_apply(XLogRecPtr current_lsn)
LWLockRelease(LogicalRepWorkerLock);
/*
- * Enter busy loop and wait for synchronization worker to
- * reach expected state (or die trying).
+ * If we have a transaction, we must commit it to release
+ * any locks we have. Then start a new transaction so we
+ * can examine catalog state.
*/
- if (!started_tx)
+ if (started_tx)
+ {
+ CommitTransactionCommand();
+ pgstat_report_stat(false);
+ StartTransactionCommand();
+ }
+ else
{
StartTransactionCommand();
started_tx = true;
}
+ /*
+ * Enter busy loop and wait for synchronization worker to
+ * reach expected state (or die trying).
+ */
wait_for_relation_state_change(rstate->relid,
SUBREL_STATE_SYNCDONE);
}
--
2.34.1
From ce261399d22a7fd9ee7ccdd3b9d1cc7f4f72ce4b Mon Sep 17 00:00:00 2001
From: Shlok Kyal
Date: Thu, 7 Dec 2023 10:55:26 +0530
Subject: [PATCH] Deadlock when apply worker,tablesync worker and client
backend run parallelly.
Apply worker holds a lock on the table pg_subscription_rel and waits for
notification from the tablesync workers where the relation is synced, which
happens through updating the pg_subscription_rel row. And that wait happens in
wait_for_relation_state_change, which simply checks the row in a loop, with a
sleep by WaitLatch(). Unfortunately, the tablesync workers can't update the row
because the client backend executing ALTER SUBSCRIPTION ... REFRESH PUBLICATION
sneaked in, and waits for an
Re: undetected deadlock in ALTER SUBSCRIPTION ... REFRESH PUBLICATION
On Tue, 5 Dec 2023 at 17:18, Tomas Vondra wrote: > > On 12/5/23 08:14, Shlok Kyal wrote: > > Hi, > > > >> As for the test results, I very much doubt the differences are not > >> caused simply by random timing variations, or something like that. And I > >> don't understand what "Performance Machine Linux" is, considering those > >> timings are slower than the other two machines. > > > > The machine has Total Memory of 755.536 GB, 120 CPUs and RHEL 7 Operating > > System > > Also find the detailed info of the performance machine attached. > > > > Thanks for the info. I don't think the tests really benefit from this > much resources, I would be rather surprised if it was faster beyond 8 > cores or so. The CPU frequency likely matters much more. Which probably > explains why this machine was the slowest. > > Also, I wonder how much the results vary between the runs. I suppose you > only did s single run for each, right? I did 10 runs for each of the cases and reported the median result in the previous thread. I have documented the result of the runs and have attached the same here. Thanks and Regards, Shlok Kyal Performance_Test.xlsx Description: MS-Excel 2007 spreadsheet
Re: undetected deadlock in ALTER SUBSCRIPTION ... REFRESH PUBLICATION
Hi, > As for the test results, I very much doubt the differences are not > caused simply by random timing variations, or something like that. And I > don't understand what "Performance Machine Linux" is, considering those > timings are slower than the other two machines. The machine has Total Memory of 755.536 GB, 120 CPUs and RHEL 7 Operating System Also find the detailed info of the performance machine attached. Thanks and Regards, Shlok Kyal MemTotal: 755.536 GB MemFree: 748.281 GB MemAvailable: 748.581 GB Buffers: 0.002 GB Cached: 1.366 GB SwapCached: 0.000 GB Active: 0.834 GB Inactive: 0.745 GB Active(anon): 0.221 GB Inactive(anon): 0.028 GB Active(file): 0.614 GB Inactive(file): 0.717 GB Unevictable: 0.000 GB Mlocked: 0.000 GB SwapTotal: 4.000 GB SwapFree: 4.000 GB Dirty: 0.000 GB Writeback: 0.000 GB AnonPages: 0.212 GB Mapped: 0.142 GB Shmem: 0.038 GB Slab: 0.468 GB SReclaimable: 0.139 GB SUnreclaim: 0.329 GB KernelStack: 0.020 GB PageTables: 0.023 GB NFS_Unstable: 0.000 GB Bounce: 0.000 GB WritebackTmp: 0.000 GB CommitLimit: 381.768 GB Committed_AS: 0.681 GB VmallocTotal: 32768.000 GB VmallocUsed: 1.852 GB VmallocChunk: 32189.748 GB Percpu: 0.035 GB HardwareCorrupted: 0.000 GB AnonHugePages: 0.025 GB CmaTotal: 0.000 GB CmaFree: 0.000 GB HugePages_Total: 0.000 GB HugePages_Free: 0.000 GB HugePages_Rsvd: 0.000 GB HugePages_Surp: 0.000 GB Hugepagesize: 0.002 GB DirectMap4k: 0.314 GB DirectMap2M: 6.523 GB DirectMap1G: 763.000 GB processor : 0 vendor_id : GenuineIntel cpu family : 6 model : 62 model name : Intel(R) Xeon(R) CPU E7-4890 v2 @ 2.80GHz stepping: 7 microcode : 0x715 cpu MHz : 1762.646 cache size : 38400 KB physical id : 0 siblings: 30 core id : 0 cpu cores : 15 apicid : 0 initial apicid : 0 fpu : yes fpu_exception : yes cpuid level : 13 wp : yes flags : fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca cmov pat pse36 clflush dts acpi mmx fxsr sse sse2 ss ht tm pbe syscall nx pdpe1gb rdtscp lm constant_tsc arch_perfmon pebs bts rep_good nopl xtopology nonstop_tsc aperfmperf eagerfpu pni pclmulqdq dtes64 monitor ds_cpl vmx smx est tm2 ssse3 cx16 xtpr pdcm pcid dca sse4_1 sse4_2 x2apic popcnt tsc_deadline_timer aes xsave avx f16c rdrand lahf_lm epb intel_ppin ssbd ibrs ibpb stibp tpr_shadow vnmi flexpriority ept vpid fsgsbase smep erms xsaveopt dtherm ida arat pln pts md_clear spec_ctrl intel_stibp flush_l1d bogomips: 5586.07 clflush size: 64 cache_alignment : 64 address sizes : 46 bits physical, 48 bits virtual power management:NAME="Red Hat Enterprise Linux Server" VERSION="7.8 (Maipo)" ID="rhel" ID_LIKE="fedora" VARIANT="Server" VARIANT_ID="server" VERSION_ID="7.8" PRETTY_NAME="Red Hat Enterprise Linux Server 7.8 (Maipo)" ANSI_COLOR="0;31" CPE_NAME="cpe:/o:redhat:enterprise_linux:7.8:GA:server" HOME_URL="https://www.redhat.com/"; BUG_REPORT_URL="https://bugzilla.redhat.com/"; REDHAT_BUGZILLA_PRODUCT="Red Hat Enterprise Linux 7" REDHAT_BUGZILLA_PRODUCT_VERSION=7.8 REDHAT_SUPPORT_PRODUCT="Red Hat Enterprise Linux" REDHAT_SUPPORT_PRODUCT_VERSION="7.8"
Re: undetected deadlock in ALTER SUBSCRIPTION ... REFRESH PUBLICATION
Hi, > thread. I think you can compare the timing of regression tests in > subscription, with and without the patch to show there is no > regression. And probably some tests with a large number of tables for > sync with very little data. I have tested the regression test timings for subscription with and without patch. I also did the timing test for sync of subscription with the publisher for 100 and 1000 tables respectively. I have attached the test script and results of the timing test are as follows: Time taken for test to run in Linux VM Summary| Subscription Test (sec) |100 tables in pub and Sub (sec)| 1000 tables in pub and Sub (sec) Without patch Release | 95.564 | 7.877 | 58.919 With patch Release| 96.513 | 6.533 | 45.807 Time Taken for test to run in another Linux VM Summary| Subscription Test (sec) |100 tables in pub and Sub (sec)| 1000 tables in pub and Sub (sec) Without patch Release | 109.8145 |6.4675 |83.001 With patch Release| 113.162 |7.947 | 87.113 Time Taken for test to run in Performance Machine Linux Summary| Subscription Test (sec) |100 tables in pub and Sub (sec)| 1000 tables in pub and Sub (sec) Without patch Release | 115.871 | 6.656 | 81.157 With patch Release| 115.922 | 6.7305 | 81.1525 thoughts? Thanks and Regards, Shlok Kyal 034_tmp.pl Description: Binary data
Re: undetected deadlock in ALTER SUBSCRIPTION ... REFRESH PUBLICATION
Hi, > I tried to reproduce the issue and was able to reproduce it with > scripts shared by Tomas. > I tried testing it from PG17 to PG 11. This issue is reproducible for > each version. > > Next I would try to test with the patch in the thread shared by Amit. I have created the v1 patch to resolve the issue. Have tested the patch on HEAD to PG12. The same patch applies to all the versions. The changes are similar to the one posted in the thread https://www.postgresql.org/message-id/1412708.1674417574%40sss.pgh.pa.us Thanks and Regards, Shlok Kyal v1-0001-Deadlock-when-apply-worker-tablesync-worker-and-c.patch Description: Binary data
Re: undetected deadlock in ALTER SUBSCRIPTION ... REFRESH PUBLICATION
Hi, I tried to reproduce the issue and was able to reproduce it with scripts shared by Tomas. I tried testing it from PG17 to PG 11. This issue is reproducible for each version. Next I would try to test with the patch in the thread shared by Amit. Thanks, Shlok Kumar Kyal
Re: pg_upgrade and logical replication
On Wed, 22 Nov 2023 at 06:48, Peter Smith wrote:
> ==
> doc/src/sgml/ref/pgupgrade.sgml
>
> 1.
> +
> + Create all the new tables that were created in the publication during
> + upgrade and refresh the publication by executing
> + ALTER
> SUBSCRIPTION ... REFRESH PUBLICATION.
> +
>
> "Create all ... that were created" sounds a bit strange.
>
> SUGGESTION (maybe like this or similar?)
> Create equivalent subscriber tables for anything that became newly
> part of the publication during the upgrade and
Modified
> ==
> src/bin/pg_dump/pg_dump.c
>
> 2. getSubscriptionTables
>
> +/*
> + * getSubscriptionTables
> + * Get information about subscription membership for dumpable tables. This
> + *will be used only in binary-upgrade mode.
> + */
> +void
> +getSubscriptionTables(Archive *fout)
> +{
> + DumpOptions *dopt = fout->dopt;
> + SubscriptionInfo *subinfo = NULL;
> + SubRelInfo *subrinfo;
> + PQExpBuffer query;
> + PGresult *res;
> + int i_srsubid;
> + int i_srrelid;
> + int i_srsubstate;
> + int i_srsublsn;
> + int ntups;
> + Oid last_srsubid = InvalidOid;
> +
> + if (dopt->no_subscriptions || !dopt->binary_upgrade ||
> + fout->remoteVersion < 17)
> + return;
>
> This function comment says "used only in binary-upgrade mode." and the
> Assert says the same. But, is this compatible with the other function
> dumpSubscriptionTable() where it says "used only in binary-upgrade
> mode and for PG17 or later versions"?
>
Modified
> ==
> src/bin/pg_upgrade/check.c
>
> 3. check_new_cluster_subscription_configuration
>
> +static void
> +check_new_cluster_subscription_configuration(void)
> +{
> + PGresult *res;
> + PGconn*conn;
> + int nsubs_on_old;
> + int max_replication_slots;
> +
> + /* Logical slots can be migrated since PG17. */
> + if (GET_MAJOR_VERSION(old_cluster.major_version) <= 1600)
> + return;
>
> IMO it is better to say < 1700 in this check, instead of <= 1600.
>
Modified
> ~~~
>
> 4.
> + /* Quick return if there are no subscriptions to be migrated */
> + if (nsubs_on_old == 0)
> + return;
>
> Missing period in comment.
>
Modified
> ~~~
>
> 5.
> +/*
> + * check_old_cluster_subscription_state()
> + *
> + * Verify that each of the subscriptions has all their corresponding tables
> in
> + * i (initialize), r (ready) or s (synchronized) state.
> + */
> +static void
> +check_old_cluster_subscription_state(ClusterInfo *cluster)
>
> This function is only for the old cluster (hint: the function name) so
> there is no need to pass the 'cluster' parameter here. Just directly
> use old_cluster in the function body.
>
Modified
> ==
> src/bin/pg_upgrade/t/004_subscription.pl
>
> 6.
> +# Add tab_not_upgraded1 to the publication. Now publication has tab_upgraded1
> +# and tab_upgraded2 tables.
> +$publisher->safe_psql('postgres',
> + "ALTER PUBLICATION regress_pub ADD TABLE tab_upgraded2");
>
> Typo in comment. You added tab_not_upgraded2, not tab_not_upgraded1
>
Modified
> ~~
>
> 7.
> +# Subscription relations should be preserved. The upgraded won't know
> +# about 'tab_not_upgraded1' because the subscription is not yet refreshed.
>
> Typo or missing word in comment?
>
> "The upgraded" ??
>
Modified
Attached the v17 patch which have the same changes
Thanks,
Shlok Kumar Kyal
v17-0001-Preserve-the-full-subscription-s-state-during-pg.patch
Description: Binary data
Failure during Building Postgres in Windows with Meson
Hi, I am trying to build postgres with meson on Windows. And I am stuck in the process. Steps I followed: 1. I clone postgres repo 2.Installed meson and ninja pip install meson ninja 3. Then running following command: meson setup build --buildtype debug 4. Then I ran cd build ninja Got following error D:\project\repo\pg_meson\postgres\build>C:\Users\kyals\AppData\Roaming\Python\Python311\Scripts\ninja ninja: error: 'src/backend/postgres_lib.a.p/meson_pch-c.obj', needed by 'src/backend/postgres.exe', missing and no known rule to make it. Any thoughts on how to resolve this error? Thanks Shlok Kumar Kyal
Re: [PoC] Implementation of distinct in Window Aggregates: take two
Hi, I went through the Cfbot, and some of the test cases are failing for this patch. It seems like some tests are crashing: https://api.cirrus-ci.com/v1/artifact/task/6291153444667392/crashlog/crashlog-postgres.exe_03b0_2023-11-07_10-41-39-624.txt [10:46:56.546] Summary of Failures: [10:46:56.546] [10:46:56.546] 87/270 postgresql:postgres_fdw / postgres_fdw/regress ERROR 11.10s exit status 1 [10:46:56.546] 82/270 postgresql:regress / regress/regress ERROR 248.55s exit status 1 [10:46:56.546] 99/270 postgresql:recovery / recovery/027_stream_regress ERROR 161.40s exit status 29 [10:46:56.546] 98/270 postgresql:pg_upgrade / pg_upgrade/002_pg_upgrade ERROR 253.31s exit status 29 link of tests failing: https://cirrus-ci.com/task/664299716712 https://cirrus-ci.com/task/4602303584403456 https://cirrus-ci.com/task/5728203491246080 https://cirrus-ci.com/task/5165253537824768?logs=test_world#L511 https://cirrus-ci.com/task/6291153444667392 Thanks Shlok Kumar Kyal
Re: [ psql - review request ] review request for \d+ tablename, \d+ indexname indenting
Hi, On Mon, 6 Nov 2023 at 13:47, 쿼리트릭스 wrote: > > The error was corrected and a new diff file was created. > The diff file was created based on 16 RC1. > We confirmed that 5 places where errors occurred when performing make check > were changed to ok. > I went through Cfbot and still see that some tests are failing. links: https://cirrus-ci.com/task/6408253983162368 https://cirrus-ci.com/task/5000879099609088 https://cirrus-ci.com/task/6126779006451712 https://cirrus-ci.com/task/5563829053030400 https://cirrus-ci.com/task/6689728959873024 Failure: [16:42:37.674] Summary of Failures: [16:42:37.674] [16:42:37.674] 5/270 postgresql:regress / regress/regress ERROR 28.88s exit status 1 [16:42:37.674] 7/270 postgresql:pg_upgrade / pg_upgrade/002_pg_upgrade ERROR 46.73s exit status 1 [16:42:37.674] 56/270 postgresql:recovery / recovery/027_stream_regress ERROR 38.51s exit status 1 Thanks Shlok Kumar Kyal
Re: POC: Extension for adding distributed tracing - pg_tracing
Hi, On Thu, 12 Oct 2023 at 14:32, Anthonin Bonnefoy wrote: > > Hi! > > I've made a new batch of changes and improvements. > New features: > - Triggers are now correctly supported. They were not correctly > attached to the ExecutorFinish Span before. > - Additional configuration: exporting parameter values in span > metadata can be disabled. Deparsing can also be disabled now. > - Dbid and userid are now exported in span's metadata > - New Notify channel and threshold parameters. This channel will > receive a notification when the span buffer usage crosses the > threshold. > - Explicit transaction with extended protocol is now correctly > handled. This is done by keeping 2 different trace contexts, one for > parser/planner trace context at the root level and the other for > nested queries and executors. The spans now correctly show the parse > of the next statement happening before the ExecutorEnd of the previous > statement (see screenshot). > > Changes: > - Parse span is now outside of the top span. When multiple statements > are processed, they are parsed together so it didn't make sense to > attach Parse to a specific statement. > - Deparse info is now exported in its own column. This will leave the > possibility to the trace forwarder to either use it as metadata or put > it in the span name. > - Renaming: Spans now have a span_type (Select, Executor, Parser...) > and a span_operation (ExecutorRun, Select $1...) > - For spans created without propagated trace context, the same traceid > will be used for statements within the same transaction. > - pg_tracing_consume_spans and pg_tracing_peek_spans are now > restricted to users with pg_read_all_stats role > - In instrument.h, instr->firsttime is only set if timer was requested > > The code should be more stable from now on. Most of the features I had > planned are implemented. > I've also started writing the module's documentation. It's still a bit > bare but I will be working on completing this. In CFbot, I see that build is failing for this patch (link: https://cirrus-ci.com/task/5378223450619904?logs=build#L1532) with following error: [07:58:05.037] In file included from ../src/include/executor/instrument.h:16, [07:58:05.037] from ../src/include/jit/jit.h:14, [07:58:05.037] from ../contrib/pg_tracing/span.h:16, [07:58:05.037] from ../contrib/pg_tracing/pg_tracing_explain.h:4, [07:58:05.037] from ../contrib/pg_tracing/pg_tracing.c:43: [07:58:05.037] ../contrib/pg_tracing/pg_tracing.c: In function ‘add_node_counters’: [07:58:05.037] ../contrib/pg_tracing/pg_tracing.c:2330:70: error: ‘BufferUsage’ has no member named ‘blk_read_time’; did you mean ‘temp_blk_read_time’? [07:58:05.037] 2330 | blk_read_time = INSTR_TIME_GET_MILLISEC(node_counters->buffer_usage.blk_read_time); [07:58:05.037] | ^ [07:58:05.037] ../src/include/portability/instr_time.h:126:12: note: in definition of macro ‘INSTR_TIME_GET_NANOSEC’ [07:58:05.037] 126 | ((int64) (t).ticks) [07:58:05.037] | ^ [07:58:05.037] ../contrib/pg_tracing/pg_tracing.c:2330:18: note: in expansion of macro ‘INSTR_TIME_GET_MILLISEC’ [07:58:05.037] 2330 | blk_read_time = INSTR_TIME_GET_MILLISEC(node_counters->buffer_usage.blk_read_time); [07:58:05.037] | ^~~ [07:58:05.037] ../contrib/pg_tracing/pg_tracing.c:2331:71: error: ‘BufferUsage’ has no member named ‘blk_write_time’; did you mean ‘temp_blk_write_time’? [07:58:05.037] 2331 | blk_write_time = INSTR_TIME_GET_MILLISEC(node_counters->buffer_usage.blk_write_time); [07:58:05.037] | ^~ [07:58:05.037] ../src/include/portability/instr_time.h:126:12: note: in definition of macro ‘INSTR_TIME_GET_NANOSEC’ [07:58:05.037] 126 | ((int64) (t).ticks) [07:58:05.037] | ^ [07:58:05.037] ../contrib/pg_tracing/pg_tracing.c:2331:19: note: in expansion of macro ‘INSTR_TIME_GET_MILLISEC’ [07:58:05.037] 2331 | blk_write_time = INSTR_TIME_GET_MILLISEC(node_counters->buffer_usage.blk_write_time); [07:58:05.037] | ^~~ I also tried to build this patch locally and the build is failing with the same error. Thanks Shlok Kumar Kyal
Re: [PoC] Federated Authn/z with OAUTHBEARER
Hi, On Fri, 3 Nov 2023 at 17:14, Jacob Champion wrote: > > v12 implements a first draft of a client hook, so applications can > replace either the device prompt or the entire OAuth flow. (Andrey and > Mahendrakar: hopefully this is close to what you need.) It also cleans > up some of the JSON tech debt. I went through CFbot and found that build is failing, links: https://cirrus-ci.com/task/6061898244816896 https://cirrus-ci.com/task/6624848198238208 https://cirrus-ci.com/task/5217473314684928 https://cirrus-ci.com/task/6343373221527552 Just want to make sure you are aware of these failures. Thanks, Shlok Kumar Kyal
Re: Force the old transactions logs cleanup even if checkpoint is skipped
Hi,
I went through the Cfbot and saw that some test are failing for it
(link: https://cirrus-ci.com/task/4631357628874752):
test: postgresql:recovery / recovery/019_replslot_limit
# test failed
--- stderr ---
# poll_query_until timed out executing this query:
# SELECT '0/15000D8' <= replay_lsn AND state = 'streaming'
# FROM pg_catalog.pg_stat_replication
# WHERE application_name IN ('standby_1', 'walreceiver')
# expecting this output:
# t
# last actual query output:
#
# with stderr:
# Tests were run but no plan was declared and done_testing() was not seen.
# Looks like your test exited with 29 just after 7.
I tried to test it locally and this test is timing out in my local
machine as well.
Thanks
Shlok Kumar Kyal
Re: [HACKERS] Allow INSTEAD OF DELETE triggers to modify the tuple for RETURNING
Hi, On Thu, 19 Oct 2023 at 16:35, Marko Tiikkaja wrote: > > Hi, > > Thank you for the feedback. > > Apparently it took me six years, but I've attached the latest version > of the patch which I believe addresses all issues. I'll add it to the > open commitfest. > > > .m I went through the CFbot and found that docs build run is giving some error (link: https://cirrus-ci.com/task/5712582359646208): [07:37:19.769] trigger.sgml:266: parser error : Opening and ending tag mismatch: command line 266 and COMMAND [07:37:19.769] DELETE operations, INSTEAD OF [07:37:19.769] ^ [07:37:19.769] trigger.sgml:408: parser error : Opening and ending tag mismatch: para line 266 and sect1 [07:37:19.769] [07:37:19.769] ^ [07:37:19.769] trigger.sgml:1034: parser error : Opening and ending tag mismatch: sect1 line 266 and chapter [07:37:19.769] [07:37:19.769] ^ [07:37:19.769] trigger.sgml:1035: parser error : chunk is not well balanced [07:37:19.769] [07:37:19.769] ^ [07:37:19.769] postgres.sgml:222: parser error : Failure to process entity trigger [07:37:19.769] &trigger; [07:37:19.769] ^ [07:37:19.769] postgres.sgml:222: parser error : Entity 'trigger' not defined [07:37:19.769] &trigger; [07:37:19.769] ^ [07:37:19.956] make[2]: *** [Makefile:73: postgres-full.xml] Error 1 [07:37:19.957] make[1]: *** [Makefile:8: all] Error 2 [07:37:19.957] make: *** [Makefile:16: all] Error 2 [07:37:19.957] [07:37:19.957] real 0m0.529s [07:37:19.957] user 0m0.493s [07:37:19.957] sys 0m0.053s [07:37:19.957] [07:37:19.957] Exit status: 2 Just wanted to make sure you are aware of the issue. Thanks Shlok Kumar Kyal
Re: psql not responding to SIGINT upon db reconnection
Hi, > That sounds like a much better solution. Attached you will find a v4 > that implements your suggestion. Please let me know if there is > something that I missed. I can confirm that the patch works. > > $ ./build/src/bin/psql/psql -h pg.neon.tech > NOTICE: Welcome to Neon! > Authenticate by visiting: > https://console.neon.tech/psql_session/xxx > > > NOTICE: Connecting to database. > psql (17devel, server 15.3) > SSL connection (protocol: TLSv1.3, cipher: TLS_AES_256_GCM_SHA384, > compression: off) > Type "help" for help. > > tristan957=> \c > NOTICE: Welcome to Neon! > Authenticate by visiting: > https://console.neon.tech/psql_session/yyy > > > ^Cconnection to server at "pg.neon.tech" (3.18.6.96), port 5432 > failed: > Previous connection kept > tristan957=> I went through CFbot and found out that some tests are timing out. Links: https://cirrus-ci.com/task/6735437444677632 https://cirrus-ci.com/task/4536414189125632 https://cirrus-ci.com/task/5046587584413696 https://cirrus-ci.com/task/6172487491256320 https://cirrus-ci.com/task/5609537537835008 Some tests which are timing out are as follows: [00:48:49.243] Summary of Failures: [00:48:49.243] [00:48:49.243] 4/270 postgresql:regress / regress/regress TIMEOUT 1000.01s [00:48:49.243] 6/270 postgresql:pg_upgrade / pg_upgrade/002_pg_upgrade TIMEOUT 1000.02s [00:48:49.243] 34/270 postgresql:recovery / recovery/027_stream_regress TIMEOUT 1000.02s [00:48:49.243] 48/270 postgresql:plpgsql / plpgsql/regress TIMEOUT 1000.02s [00:48:49.243] 49/270 postgresql:plperl / plperl/regress TIMEOUT 1000.01s [00:48:49.243] 61/270 postgresql:dblink / dblink/regress TIMEOUT 1000.03s [00:48:49.243] 89/270 postgresql:postgres_fdw / postgres_fdw/regress TIMEOUT 1000.01s [00:48:49.243] 93/270 postgresql:test_decoding / test_decoding/regress TIMEOUT 1000.02s [00:48:49.243] 110/270 postgresql:test_extensions / test_extensions/regress TIMEOUT 1000.03s [00:48:49.243] 123/270 postgresql:unsafe_tests / unsafe_tests/regress TIMEOUT 1000.02s [00:48:49.243] 152/270 postgresql:pg_dump / pg_dump/010_dump_connstr TIMEOUT 1000.03s Just want to make sure you are aware of the issue. Thanks Shlok Kumar Kyal
Re: [patch] pg_basebackup: mention that spread checkpoints are the default in --help
Hi,
On Thu, 26 Oct 2023 at 13:58, Michael Banck wrote:
>
> Hi,
>
> On Wed, Oct 25, 2023 at 04:36:41PM +0200, Peter Eisentraut wrote:
> > On 19.10.23 11:39, Michael Banck wrote:
> > > Hi,
> > >
> > > I believed that spread (not fast) checkpoints are the default in
> > > pg_basebackup, but noticed that --help does not specify which is which -
> > > contrary to the reference documentation.
> > >
> > > So I propose the small attached patch to clarify that.
> >
> > > printf(_(" -c, --checkpoint=fast|spread\n"
> > >- " set fast or spread checkpointing\n"));
> > >+ " set fast or spread (default)
> > checkpointing\n"));
> >
> > Could we do like
> >
> > -c, --checkpoint=fast|spread
> > set fast or spread checkpointing
> > (default: spread)
> >
> > This seems to be easier to read.
>
> Yeah, we could do that. But then again the question pops up what to do
> about the other option that mentions defaults (-F) and the others which
> have a default but it is not spelt out yet (-X, -Z at least) (output is
> still from v15, additional options have been added since):
>
> -F, --format=p|t output format (plain (default), tar)
> -X, --wal-method=none|fetch|stream
> include required WAL files with specified method
> -Z, --compress=0-9 compress tar output with given compression level
>
> So, my personal opinion is that we should really document -c because it
> is quite user-noticable compared to the others.
>
> So attached is a new version with just your proposed change for now.
>
>
> Michael
I went through the Cfbot for this patch and found out that the build
is failing with the following error (Link:
https://cirrus-ci.com/task/4648506929971200?logs=build#L1217):
[08:34:47.625] FAILED: src/bin/pg_basebackup/pg_basebackup.p/pg_basebackup.c.o
[08:34:47.625] ccache cc -Isrc/bin/pg_basebackup/pg_basebackup.p
-Isrc/include -I../src/include -Isrc/interfaces/libpq
-I../src/interfaces/libpq -Isrc/include/catalog -Isrc/include/nodes
-Isrc/include/utils -fdiagnostics-color=always -pipe
-D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -g -fno-strict-aliasing
-fwrapv -fexcess-precision=standard -D_GNU_SOURCE -Wmissing-prototypes
-Wpointer-arith -Werror=vla -Wendif-labels -Wmissing-format-attribute
-Wimplicit-fallthrough=3 -Wcast-function-type
-Wshadow=compatible-local -Wformat-security
-Wdeclaration-after-statement -Wno-format-truncation
-Wno-stringop-truncation -pthread -MD -MQ
src/bin/pg_basebackup/pg_basebackup.p/pg_basebackup.c.o -MF
src/bin/pg_basebackup/pg_basebackup.p/pg_basebackup.c.o.d -o
src/bin/pg_basebackup/pg_basebackup.p/pg_basebackup.c.o -c
../src/bin/pg_basebackup/pg_basebackup.c
[08:34:47.625] ../src/bin/pg_basebackup/pg_basebackup.c: In function ‘usage’:
[08:34:47.625] ../src/bin/pg_basebackup/pg_basebackup.c:411:5:
warning: statement with no effect [-Wunused-value]
[08:34:47.625] 411 | " (default: spread)\n"));
[08:34:47.625] | ^~
[08:34:47.625] ../src/bin/pg_basebackup/pg_basebackup.c:411:51: error:
expected ‘;’ before ‘)’ token
[08:34:47.625] 411 | " (default: spread)\n"));
[08:34:47.625] | ^
[08:34:47.625] | ;
[08:34:47.625] ../src/bin/pg_basebackup/pg_basebackup.c:411:51: error:
expected statement before ‘)’ token
[08:34:47.625] ../src/bin/pg_basebackup/pg_basebackup.c:411:52: error:
expected statement before ‘)’ token
[08:34:47.625] 411 | " (default: spread)\n"));
[08:34:47.625] | ^
[08:34:47.629] [1210/1832] Compiling C object
src/bin/pg_dump/libpgdump_common.a.p/parallel.c.o
[08:34:47.639] [1211/1832] Compiling C object
src/bin/pg_basebackup/pg_recvlogical.p/pg_recvlogical.c.o
[08:34:47.641] [1212/1832] Linking static target
src/bin/pg_basebackup/libpg_basebackup_common.a
[08:34:47.658] [1213/1832] Compiling C object
src/bin/pg_dump/libpgdump_common.a.p/compress_io.c.o
[08:34:47.669] [1214/1832] Compiling C object
src/bin/pg_dump/libpgdump_common.a.p/compress_lz4.c.o
[08:34:47.678] [1215/1832] Compiling C object
src/bin/pg_dump/libpgdump_common.a.p/compress_zstd.c.o
[08:34:47.692] [1216/1832] Compiling C object
src/bin/pg_dump/libpgdump_common.a.p/dumputils.c.o
[08:34:47.692] ninja: build stopped: subcommand failed.
I also see that patch is marked 'Ready for Committer' on commitfest.
Just wanted to make sure, you are aware of this error.
Thanks,
Shlok Kumar Kyal
Re: [PoC] pg_upgrade: allow to upgrade publisher node
I tested a test scenario:
I started a new publisher with 'max_replication_slots' parameter set
to '1' and created a streaming replication with the new publisher as
primary node.
Then I did a pg_upgrade from old publisher to new publisher. The
upgrade failed with following error:
Restoring logical replication slots in the new cluster
SQL command failed
SELECT * FROM pg_catalog.pg_create_logical_replication_slot('test1',
'pgoutput', false, false);
ERROR: all replication slots are in use
HINT: Free one or increase max_replication_slots.
Failure, exiting
Should we document that the existing replication slots are taken in
consideration while setting 'max_replication_slots' value in the new
publisher?
Thanks
Shlok Kumar Kyal
On Wed, 18 Oct 2023 at 15:01, Hayato Kuroda (Fujitsu)
wrote:
>
> Dear Peter,
>
> Thank you for reviewing! PSA new version.
> Note that 0001 and 0002 are combined into one patch.
>
> > Here are some review comments for v51-0001
> >
> > ==
> > src/bin/pg_upgrade/check.c
> >
> > 0.
> > +check_old_cluster_for_valid_slots(bool live_check)
> > +{
> > + char output_path[MAXPGPATH];
> > + FILE*script = NULL;
> > +
> > + prep_status("Checking for valid logical replication slots");
> > +
> > + snprintf(output_path, sizeof(output_path), "%s/%s",
> > + log_opts.basedir,
> > + "invalid_logical_relication_slots.txt");
> >
> > 0a
> > typo /invalid_logical_relication_slots/invalid_logical_replication_slots/
>
> Fixed.
>
> > 0b.
> > Since the non-upgradable slots are not strictly "invalid", is this an
> > appropriate filename for the bad ones?
> >
> > But I don't have very good alternatives. Maybe:
> > - non_upgradable_logical_replication_slots.txt
> > - problem_logical_replication_slots.txt
>
> Per discussion [1], I kept current style.
>
> > src/bin/pg_upgrade/t/003_upgrade_logical_replication_slots.pl
> >
> > 1.
> > +# --
> > +# TEST: Confirm pg_upgrade fails when wrong GUC is set on new cluster
> > +#
> > +# There are two requirements for GUCs - wal_level and
> > max_replication_slots,
> > +# but only max_replication_slots will be tested here. This is because to
> > +# reduce the execution time of the test.
> >
> >
> > SUGGESTION
> > # TEST: Confirm pg_upgrade fails when the new cluster has wrong GUC values.
> > #
> > # Two GUCs are required - 'wal_level' and 'max_replication_slots' - but to
> > # reduce the test execution time, only 'max_replication_slots' is tested
> > here.
>
> First part was fixed. Second part was removed per [1].
>
> > 2.
> > +# Preparations for the subsequent test:
> > +# 1. Create two slots on the old cluster
> > +$old_publisher->start;
> > +$old_publisher->safe_psql('postgres',
> > + "SELECT pg_create_logical_replication_slot('test_slot1',
> > 'test_decoding', false, true);"
> > +);
> > +$old_publisher->safe_psql('postgres',
> > + "SELECT pg_create_logical_replication_slot('test_slot2',
> > 'test_decoding', false, true);"
> > +);
> >
> >
> > Can't you combine those SQL in the same $old_publisher->safe_psql.
>
> Combined.
>
> > 3.
> > +# Clean up
> > +rmtree($new_publisher->data_dir . "/pg_upgrade_output.d");
> > +# Set max_replication_slots to the same value as the number of slots. Both
> > of
> > +# slots will be used for subsequent tests.
> > +$new_publisher->append_conf('postgresql.conf', "max_replication_slots =
> > 1");
> >
> > The code doesn't seem to match the comment - is this correct? The
> > old_publisher created 2 slots, so why are you setting new_publisher
> > "max_replication_slots = 1" again?
>
> Fixed to "max_replication_slots = 2" Note that previous test worked well
> because
> GUC checking on new cluster is done after checking the status of slots.
>
> > 4.
> > +# Preparations for the subsequent test:
> > +# 1. Generate extra WAL records. Because these WAL records do not get
> > consumed
> > +# it will cause the upcoming pg_upgrade test to fail.
> > +$old_publisher->start;
> > +$old_publisher->safe_psql('postgres',
> > + "CREATE TABLE tbl AS SELECT generate_series(1, 10) AS a;");
> > +
> > +# 2. Advance the slot test_slot2 up to the current WAL location
> > +$old_publisher->safe_psql('postgres',
> > + "SELECT pg_replication_slot_advance('test_slot2', NULL);");
> > +
> > +# 3. Emit a non-transactional message. test_slot2 detects the message so
> > that
> > +# this slot will be also reported by upcoming pg_upgrade.
> > +$old_publisher->safe_psql('postgres',
> > + "SELECT count(*) FROM pg_logical_emit_message('false', 'prefix',
> > 'This is a non-transactional message');"
> > +);
> >
> >
> > I felt this test would be clearer if you emphasised the state of the
> > test_slot1 also. e.g.
> >
> > 4a.
> > BEFORE
> > +# 1. Generate extra WAL records. Because these WAL records do not get
> > consumed
> > +# it will cause the upcoming pg_upgrade test to fail.
> >
> > SUGGESTION
> > # 1. Generate extra WAL records. At this point neither test_slot1 nor
> > test_slot2
> > #has consumed them.
>
> Fixed.
>
> > 4b.
> > BEFORE
>
Re: [PoC] pg_upgrade: allow to upgrade publisher node
> Few comments: > 1) We will be able to override the value of max_slot_wal_keep_size by > using --new-options like '--new-options "-c > max_slot_wal_keep_size=val"': > + /* > +* Use max_slot_wal_keep_size as -1 to prevent the WAL removal by the > +* checkpointer process. If WALs required by logical replication > slots > +* are removed, the slots are unusable. This setting prevents the > +* invalidation of slots during the upgrade. We set this option when > +* cluster is PG17 or later because logical replication slots > can only be > +* migrated since then. Besides, max_slot_wal_keep_size is > added in PG13. > +*/ > + if (GET_MAJOR_VERSION(cluster->major_version) >= 1700) > + appendPQExpBufferStr(&pgoptions, " -c > max_slot_wal_keep_size=-1"); > > Should there be a check to throw an error if this option is specified > or do we need some documentation that this option should not be > specified? I have tested the above scenario. We are able to override the max_slot_wal_keep_size by using '--new-options "-c max_slot_wal_keep_size=val"'. And also with some insert statements during pg_upgrade, old WAL file were deleted and logical replication slots were invalidated. Since the slots were invalidated replication was not happening after the upgrade. Thanks, Shlok Kumar Kyal
Re: pg_ctl start may return 0 even if the postmaster has been already started on Windows
On Fri, 6 Oct 2023 at 11:38, Hayato Kuroda (Fujitsu) wrote: > > Dear Horiguchi-san, > > Thank you for making a patch! They can pass ci. > I'm still not sure what should be, but I can respond a part. > > > Another issue is.. that I haven't been able to cause the false > > positive of pg_ctl start.. Do you have a concise reproducer of the > > issue? > > I found a short sleep in pg_ctl/t/001_start_stop.pl. This was introduced in > 6bcce2580 to ensure waiting more than 2 seconds. I've tested on my CI and > found that removing the sleep can trigger the failure. Also, I confirmed your > patch > fixes the problem. PSA the small patch for cfbot. 0001 and 0002 were not > changed. I have tested the patches on my windows setup. I am trying to start two postgres servers with an interval of 5 secs. with HEAD (when same server is started after an interval of 5 secs): D:\project\pg\bin>pg_ctl -D ../data -l data2.log start pg_ctl: another server might be running; trying to start server anyway waiting for server to start stopped waiting pg_ctl: could not start server Examine the log output. with Patch:(when same server is started after an interval of 5 secs) D:\project\pg_dev\bin>pg_ctl -D ../data -l data2.log start pg_ctl: another server might be running; trying to start server anyway waiting for server to startpg_ctl: launcher shell died The output message after patch is different from the HEAD. I felt that with patch as well we should get the message "pg_ctl: could not start server". Is this message change intentional? Thanks, Shlok Kumar Kyal
Re: Invalidate the subscription worker in cases where a user loses their superuser status
On Wed, 4 Oct 2023 at 16:56, Peter Smith wrote: > > On Tue, Oct 3, 2023 at 5:42 PM vignesh C wrote: > > > > Thanks for the comments, the attached v6 version patch has the changes > > for the same. > > > > v6 LGTM. > I have verified the patch and it is working fine for me.
