RE: [Proposal] Add foreign-server health checks infrastructure
On Monday, October 17, 2022 9:25 PM Kuroda, Hayato/黒田 隼人 wrote: > > I mainly followed the steps there and > > replaced the command "SELECT" for the remote table at 6-9 with "INSERT" > > command. > > Then, after waiting for few seconds, the "COMMIT" succeeded like below > > output, even after the server stop of the worker side. > I understood that we should not call disconnect_pg_server(entry->conn) even if > we detect the disconnection. > If should be controlled in Xact callback. This will be modified in next > version. Hi, FYI, I quickly rechecked the patch's behavior on that scenario with v17. Now, the last "COMMIT" above failed expectedly, which looks reasonable behavior. Thank you for having modified it. Best Regards, Takamichi Osumi
RE: [Proposal] Add foreign-server health checks infrastructure
On Wednesday, October 5, 2022 6:27 PM kuroda.hay...@fujitsu.com
wrote:
> Thanks for giving many comments! Based on off and on discussions, I modified
> my patch.
Here are my other quick review comments on v16.
(1) v16-0001 : definition of a new structure
CheckingRemoteServersCallbackItem can be added to typedefs.list.
(2) v16-0001 : UnRegisterCheckingRemoteServersCallback's header comment
+void
+UnRegisterCheckingRemoteServersCallback(CheckingRemoteServersCallback callback,
+
void *arg)
+{
Looks require a header comment for this function,
because in this file, all other functions have each one.
(3) v16-0002 : better place to declare new variables
New variables 'old' and 'server' in GetConnection() can be moved to
a scope of 'if' statement where those are used.
@@ -138,6 +149,8 @@ GetConnection(UserMapping *user, bool will_prep_stmt,
PgFdwConnState **state)
ConnCacheEntry *entry;
ConnCacheKey key;
MemoryContext ccxt = CurrentMemoryContext;
+ MemoryContext old;
+ ForeignServer *server = GetForeignServer(user->serverid);
(4) v16-0002 : typo in pgfdw_connection_check_internal()
+ /* return false is if the socket seems to be closed. */
It should be "return false if the socket seems to be closed" ?
(5) v16-0002 : pgfdw_connection_check's message
When I tested the new feature, I got a below message.
"ERROR: Foreign Server my_external_server might be down."
I think we can wrap the server name with double quotes
so that the message is more aligned with existing error message
like connect_pg_server.
(6) v16-003 : typo
+ Authors needs to register the callback function via following API.
Should be "Authors need to register the ...".
(7) v16-0004 : unnecessary file
I think we can remove a new file which looks like a log file.
.../postgres_fdw/expected/postgres_fdw_1.out
Best Regards,
Takamichi Osumi
RE: [Proposal] Add foreign-server health checks infrastructure
Hi, On Wednesday, October 5, 2022 6:27 PM kuroda.hay...@fujitsu.com wrote: > Thanks for giving many comments! Based on off and on discussions, I modified > my patch. Thank you for your patch set ! While reviewing and testing the new v16, I've met a possible issue by slightly adjusting the scenario written in [1]. I mainly followed the steps there and replaced the command "SELECT" for the remote table at 6-9 with "INSERT" command. Then, after waiting for few seconds, the "COMMIT" succeeded like below output, even after the server stop of the worker side. After the transaction itself, any reference to the remote table fails. Note that the local table has some data initially in my test. postgres=# begin; BEGIN postgres=*# insert into remote values (-1000); INSERT 0 1 postgres=*# select * from local; number 101 102 103 104 105 (5 rows) postgres=*# commit; COMMIT postgres=# select * from remote; ERROR: could not connect to server "my_external_server" DETAIL: connection to server on socket "/tmp/.s.PGSQL." failed: No such file or directory Is the server running locally and accepting connections on that socket? Additionally, the last reference "SELECT" which failed above can succeed, if I restart the worker server before the "SELECT" command to the remote table. This means the transaction looks successful but the data isn't there ? Could you please have a look at this issue ? [1] - https://www.postgresql.org/message-id/TYAPR01MB58662CD4FD98AA475B3D10F9F59B9%40TYAPR01MB5866.jpnprd01.prod.outlook.com Best Regards, Takamichi Osumi
RE: possible typo for CREATE PUBLICATION description
Hi, Alvaro-san On Thursday, October 13, 2022 8:38 PM Alvaro Herrera wrote: > On 2022-Oct-13, osumi.takami...@fujitsu.com wrote: > > > I noticed a possible typo in the doc for create publication. > > This applies to PG15 as well. > > Kindly have a look at the attached patch for it. > > Yeah, looks good. It actually applies all the way back to 10, so I pushed it > to > all branches. I included Peter's wording changes as well. > > Thanks You are right and thank you so much for taking care of this ! Best Regards, Takamichi Osumi
possible typo for CREATE PUBLICATION description
Hi, I noticed a possible typo in the doc for create publication. This applies to PG15 as well. Kindly have a look at the attached patch for it. Best Regards, Takamichi Osumi v1-0001-modify-a-typo.patch Description: v1-0001-modify-a-typo.patch
RE: Allow logical replication to copy tables in binary format
Hi, On Monday, October 3, 2022 8:50 PM Melih Mutlu wrote: > (4) Error message of the binary format copy > > I've gotten below message from data type contradiction (between > integer and bigint). > Probably, this is unclear for the users to understand the direct cause > and needs to be adjusted ? > This might be a similar comment Euler mentioned in [1]. > > 2022-09-16 11:54:54.835 UTC [4570] ERROR: insufficient data left in > message > 2022-09-16 11:54:54.835 UTC [4570] CONTEXT: COPY tab, line 1, > column id > > It's already unclear for users to understand what's the issue if they're > copying > data between different column types via the COPY command. > This issue comes from COPY, and logical replication just utilizes COPY. > I don't think it would make sense to adjust an error message from a > functionality > which logical replication only uses and has no direct impact on. > It might be better to do this in a separate patch. What do you think? Yes, makes sense. It should be a separate patch. > I agree with the direction to support binary copy for v16 and later. > > IIUC, the binary format replication with different data types fails even > during apply phase on HEAD. > I thought that means, the upgrade concern only applies to a scenario > that the user executes > only initial table synchronizations between the publisher and subscriber > and doesn't replicate any data at apply phase after that. I would say > this isn't a valid scenario and your proposal makes sense. > > No, logical replication in binary does not fail on apply phase if data types > are > different. With HEAD, I observe in some case we fail at apply phase because of different data types like integer vs. bigint as written scenario in [1]. In short, I think we can slightly adjust your documentation and make it more general so that the description applies to both table sync phase and apply phase. I'll suggest a below change for your sentence of logical-replication.sgml. FROM: In binary case, it is not allowed to replicate data between different types due to restrictions inherited from COPY. TO: Binary format is type specific and does not allow to replicate data between different types according to its restrictions. If my idea above is correct, then I feel we can remove all the fixes for create_subscription.sgml. I'm not sure if I should pursue this perspective of the document improvement any further after this email, since this isn't essentially because of this patch. > The concern with upgrade (if data types are not the same) would be not being > able to create a new subscription with binary enabled or replicate new tables > added into publication. > Replication of tables from existing subscriptions would not be affected by > this > change since they will already be in the apply phase, not tablesync. > Do you think this would still be an issue? Okay, thanks for explaining this. I understand that the upgrade concern applies to the table sync that is executed between text format (before the patch) and binary format (after the patch). [1] - binary format test that we fail for different types on apply phase on HEAD create table tab (id integer); insert into tab values (100); create publication mypub for table tab; create table tab (id bigint); create subscription mysub connection '...' publication mypub with (copy_data = false, binary = true, disable_on_error = true); -- wait for several seconds select srsubid, srrelid, srrelid::regclass, srsubstate, srsublsn from pg_subscription_rel; -- check the status as 'r' for the relation select * from tab; -- confirm we don't copy the initial data on the pub insert into tab values (1), (2); -- wait for several seconds select subname, subenabled from pg_subscription; -- shows 'f' for the 2nd column because of an error select * from tab -- no records This error doesn't happen when we adopt 'integer' on the subscriber aligned with the publisher and we can see the two records on the subscriber. Best Regards, Takamichi Osumi
RE: Data is copied twice when specifying both child and parent table in publication
On Wednesday, September 28, 2022 5:36 PM Wang, Wei/王 威 wrote: > Also rebased the patch because the change in the HEAD (20b6847). > > Attach the new patches. Hi, thank you for the updated patches! Here are my minor review comments for HEAD v12. (1) typo & suggestion to reword one comment +* Publications support partitioned tables. If +* publish_via_partition_root is false, all changes are replicated +* using leaf partition identity and schema, so we only need +* those. Otherwise, If publish_via_partition_root is true, get +* the partitioned table itself. The last sentence has "If" in the middle of the sentence. We can use the lower letter for it. Or, I think "Otherwise" by itself means "If publish_via_partition_root is true". So, I'll suggest a below change. FROM: Otherwise, If publish_via_partition_root is true, get the partitioned table itself. TO: Otherwise, get the partitioned table itself. (2) Do we need to get "attnames" column from the publisher in the fetch_table_list() ? When I was looking at v16 path, I didn't see any codes that utilize the "attnames" column information returned from the publisher. If we don't need it, could we remove it ? I can miss something greatly, but this might be affected by HEAD codes ? Best Regards, Takamichi Osumi
RE: Data is copied twice when specifying both child and parent table in publication
On Tuesday, September 20, 2022 3:18 PM Wang, Wei/王 威
wrote:
> Rebased the patch based on the changes in HEAD (20b6847).
> Attach the new patches.
Hi, thank you for updating the patchset.
FYI, I noticed that the patch for head is no longer applicable.
$ git apply --check
HEAD_v10-0001-Fix-data-replicated-twice-when-specifying-publis.patch
error: patch failed: src/backend/catalog/pg_publication.c:1097
error: src/backend/catalog/pg_publication.c: patch does not apply
Also, one minor comment on the change in src/include/catalog/pg_proc.dat.
# publications
-{ oid => '6119', descr => 'get information of tables in a publication',
- proname => 'pg_get_publication_tables', prorows => '1000', proretset => 't',
- provolatile => 's', prorettype => 'record', proargtypes => 'text',
- proallargtypes => '{text,oid,int2vector,pg_node_tree}',
- proargmodes => '{i,o,o,o}', proargnames => '{pubname,relid,attrs,qual}',
+{ oid => '6119',
+ descr => 'get information of the tables belonging to the specified
publications.',
Please remove the period at the end of 'descr' string.
It seems we don't write it in this file and removing it makes the code more
aligned.
Best Regards,
Takamichi Osumi
RE: Allow logical replication to copy tables in binary format
Hi Few more minor comments. On Tuesday, August 16, 2022 2:04 AM Melih Mutlu wrote: > > > My main concern is to break a scenario that was previously working (14 > -> 15) but after a subscriber upgrade > it won't (14 -> 16). > > Fair concern. Some cases that might break the logical replication with version > upgrade would be: ... > 3- Copying in binary format would work with the same schemas. Currently, > logical replication does not require the exact same schemas in publisher and > subscriber. > This is an additional restriction that comes with the COPY command. > > If a logical replication has been set up with different schemas and > subscription > is created with the binary option, then yes this would break things. > This restriction can be clearly stated and wouldn't be unexpected though. > > I'm also okay with allowing binary copy only for v16 or later, if you think > it would > be safer and no one disagrees with that. > What are your thoughts? I agree with the direction to support binary copy for v16 and later. IIUC, the binary format replication with different data types fails even during apply phase on HEAD. I thought that means, the upgrade concern only applies to a scenario that the user executes only initial table synchronizations between the publisher and subscriber and doesn't replicate any data at apply phase after that. I would say this isn't a valid scenario and your proposal makes sense. Best Regards, Takamichi Osumi
RE: Allow logical replication to copy tables in binary format
On Tuesday, August 16, 2022 2:04 AM Melih Mutlu wrote: > Attached patch also includes some additions to the doc along with the tests. Hi, thank you for updating the patch. Minor review comments for the v2. (1) whitespace issues Please fix below whitespace errors. $ git apply v2-0001-Allow-logical-replication-to-copy-table-in-binary.patch v2-0001-Allow-logical-replication-to-copy-table-in-binary.patch:39: trailing whitespace. binary format.(See .) v2-0001-Allow-logical-replication-to-copy-table-in-binary.patch:120: trailing whitespace. v2-0001-Allow-logical-replication-to-copy-table-in-binary.patch:460: trailing whitespace. ); warning: 3 lines add whitespace errors. (2) Suggestion to update another general description about the subscription Kindly have a look at doc/src/sgml/logical-replication.sgml. "The data types of the columns do not need to match, as long as the text representation of the data can be converted to the target type. For example, you can replicate from a column of type integer to a column of type bigint." With the patch, I think we have an impact about those descriptions since enabling the binary option for a subscription and executing the initial synchronization requires the same data types for binary format. I suggest that we update those descriptions as well. (3) shouldn't test that we fail expectedly with binary copy for different types ? How about having a test that we correctly fail with different data types between the publisher and the subscriber, for instance ? (4) Error message of the binary format copy I've gotten below message from data type contradiction (between integer and bigint). Probably, this is unclear for the users to understand the direct cause and needs to be adjusted ? This might be a similar comment Euler mentioned in [1]. 2022-09-16 11:54:54.835 UTC [4570] ERROR: insufficient data left in message 2022-09-16 11:54:54.835 UTC [4570] CONTEXT: COPY tab, line 1, column id (5) Minor adjustment of the test comment in 002_types.pl. +is( $result, $sync_result, 'check initial sync on subscriber'); +is( $result_binary, $sync_result, 'check initial sync on subscriber in binary'); # Insert initial test data There are two same comments which say "Insert initial test data" in this file. We need to update them, one for the initial table sync and the other for the application of changes. [1] - https://www.postgresql.org/message-id/f1d58324-8df4-4bb5-a546-8c741c2e6fa8%40www.fastmail.com Best Regards, Takamichi Osumi
test_decoding assertion failure for the loss of top-sub transaction relationship
Hi, hackers
I've met an assertion failure of logical decoding with below scenario on HEAD.
---
create table tab1 (val integer);
select 'init' from pg_create_logical_replication_slot('regression_slot',
'test_decoding');
begin;
savepoint sp1;
insert into tab1 values (1);
checkpoint; -- for RUNNING_XACT
select data FROM pg_logical_slot_get_changes('regression_slot', NULL, NULL,
'include-xids', '0', 'skip-empty-xacts', '1');
truncate tab1; -- for NEW_CID
commit;
begin;
insert into tab1 values (3);
checkpoint;
select data FROM pg_logical_slot_get_changes('regression_slot', NULL, NULL,
'include-xids', '0', 'skip-empty-xacts', '1');
commit;
select data FROM pg_logical_slot_get_changes('regression_slot', NULL, NULL,
'include-xids', '0', 'skip-empty-xacts', '1');
---
Here, it's not a must but is advisable to make LOG_SNAPSHOT_INTERVAL_MS bigger
so that
we can issue RUNNING_XACT according to our checkpoint commands explicitly.
In the above scenario, the first checkpoint generates RUNNING_XACT after the
wal record
(for ReorderBufferAssignChild) that associates sub transaction with its top
transaction.
This means that once we restart from RUNNING_XACT, we lose the association
between top
transaction and sub transaction and then we can't mark the top transaction as
catalog
modifying transaction by decoding NEW_CID (written after RUNNING_XACT), if the
sub transaction changes the catalog.
Therefore, this leads to the failure for the assert that can check
the consistency that when one sub transaction modifies the catalog,
its top transaction should be marked so as well.
I feel we need to remember the relationship between top transaction and sub
transaction
in the serialized snapshot even before changing catalog at decoding
RUNNING_XACT,
so that we can keep track of the association after the restart. What do you
think ?
The stack call of this failure and related information is below.
(gdb) bt
#0 0x7f2632588387 in raise () from /lib64/libc.so.6
#1 0x7f2632589a78 in abort () from /lib64/libc.so.6
#2 0x00b3eba1 in ExceptionalCondition (conditionName=0xd137e0
"!needs_snapshot || needs_timetravel",
errorType=0xd130c5 "FailedAssertion", fileName=0xd130b9 "snapbuild.c",
lineNumber=1116) at assert.c:69
#3 0x00911257 in SnapBuildCommitTxn (builder=0x23f0638, lsn=22386632,
xid=728, nsubxacts=1,
subxacts=0x2bfcc88, xinfo=79) at snapbuild.c:1116
#4 0x008fa420 in DecodeCommit (ctx=0x23e0108, buf=0x7fff4a1f9220,
parsed=0x7fff4a1f9020, xid=728,
two_phase=false) at decode.c:630
#5 0x008f9953 in xact_decode (ctx=0x23e0108, buf=0x7fff4a1f9220) at
decode.c:216
#6 0x008f967d in LogicalDecodingProcessRecord (ctx=0x23e0108,
record=0x23e04a0) at decode.c:119
#7 0x00900b63 in pg_logical_slot_get_changes_guts (fcinfo=0x23d80a8,
confirm=true, binary=false)
at logicalfuncs.c:271
#8 0x00900ca0 in pg_logical_slot_get_changes (fcinfo=0x23d80a8) at
logicalfuncs.c:338
...
(gdb) frame 3
#3 0x00911257 in SnapBuildCommitTxn (builder=0x23f0638, lsn=22386632,
xid=728, nsubxacts=1,
subxacts=0x2bfcc88, xinfo=79) at snapbuild.c:1116
1116Assert(!needs_snapshot || needs_timetravel);
(gdb) list
{
1112/* record that we cannot export a general snapshot
anymore */
1113builder->committed.includes_all_transactions = false;
1114}
1115
1116Assert(!needs_snapshot || needs_timetravel);
1117
1118/*
1119 * Adjust xmax of the snapshot builder, we only do that for
committed,
1120 * catalog modifying, transactions, everything else isn't
interesting for
Best Regards,
Takamichi Osumi
RE: [PATCH] Use indexes on the subscriber when REPLICA IDENTITY is full on the publisher
On Tuesday, August 9, 2022 12:59 AM Önder Kalacı wrote: > Attaching v5 of the patch which reflects the review on this email, also few > minor test improvements. Hi, Thank you for the updated patch. FYI, I noticed that v5 causes cfbot failure in [1]. Could you please fix it in the next version ? [19:44:38.420] execReplication.c: In function ‘RelationFindReplTupleByIndex’: [19:44:38.420] execReplication.c:186:24: error: ‘eq’ may be used uninitialized in this function [-Werror=maybe-uninitialized] [19:44:38.420] 186 | if (!indisunique && !tuples_equal(outslot, searchslot, eq)) [19:44:38.420] | ^ [19:44:38.420] cc1: all warnings being treated as errors [1] - https://cirrus-ci.com/task/6544573026533376 Best Regards, Takamichi Osumi
RE: logical replication restrictions
On Tuesday, August 9, 2022 6:47 AM Euler Taveira wrote: > I attached a v6. Hi, thank you for posting the updated patch. Minor review comments for v6. (1) commit message "If the subscriber sets min_apply_delay parameter, ..." I suggest we use subscription rather than subscriber, because this parameter refers to and is used for one subscription. My suggestion is "If one subscription sets min_apply_delay parameter, ..." In case if you agree, there are other places to apply this change. (2) commit message It might be better to write a note for committer like "Bump catalog version" at the bottom of the commit message. (3) unit alignment between recovery_min_apply_delay and min_apply_delay The former interprets input number as milliseconds in case of no units, while the latter takes it as seconds without units. I feel it would be better to make them aligned. (4) catalogs.sgml + Delay the application of changes by a specified amount of time. The + unit is in milliseconds. As a column explanation, it'd be better to use a noun in the first sentence to make this description aligned with other places. My suggestion is "Application delay of changes by ". (5) pg_subscription.c There is one missing blank line before writing if statement. It's written in the AlterSubscription for other cases. @@ -1100,6 +1130,12 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt, replaces[Anum_pg_subscription_subdisableonerr - 1] = true; } + if (IsSet(opts.specified_opts, SUBOPT_MIN_APPLY_DELAY)) (6) tab-complete.c The order of tab-complete parameters listed in the COMPLETE_WITH should follow alphabetical order. "min_apply_delay" can come before "origin". We can refer to d547f7c commit. (7) 032_apply_delay.pl There are missing whitespaces after comma in the mod functions. UPDATE test_tab SET b = md5(b) WHERE mod(a,2) = 0; DELETE FROM test_tab WHERE mod(a,3) = 0; Best Regards, Takamichi Osumi
RE: [BUG] Logical replication failure "ERROR: could not map filenode "base/13237/442428" to relation OID" with catalog modifying txns
On Sunday, July 17, 2022 9:59 PM Masahiko Sawada wrote: > I've attached patches for all supported branches including the master. Hi, Minor comments for REL14. (1) There are some foreign characters in the patches (in the commit message) When I had a look at your patch for back branches with some editor, I could see some unfamiliar full-width characters like below two cases, mainly around "single quotes" in the sentences. Could you please check the entire patches, probably by some tool that helps you to detect this kind of characters ? * the 2nd paragraph of the commit message ...mark the transaction as containing catalog changes if it窶冱 in the list of the initial running transactions ... * the 3rd paragraph of the same It doesn窶冲 have the information on which (sub) transaction has catalog changes FYI, this comment applies to other patches for REL13, REL12, REL11, REL10. (2) typo in the commit message FROM: To fix this problem, this change the reorder buffer so that... TO: To fix this problem, this changes the reorder buffer so that... (3) typo in ReorderBufferProcessInitialXacts + /* +* Remove transactions that would have been processed and we don't need to +* keep track off anymore. Kindly change FROM: keep track off TO: keep track of Best Regards, Takamichi Osumi
RE: [BUG] Logical replication failure "ERROR: could not map filenode "base/13237/442428" to relation OID" with catalog modifying txns
On Thursday, July 14, 2022 10:31 AM Masahiko Sawada wrote: > I've attached an updated patch that incorporated comments from Amit and Shi. Hi, Minor comments for v4. (1) typo in the commit message "When decoding a COMMIT record, we check both the list and the ReorderBuffer to see if if the transaction has modified catalogs." There are two 'if's in succession in the last sentence of the second paragraph. (2) The header comment for the spec test +# Test that decoding only the commit record of the transaction that have +# catalog-changed. Rewording of this part looks required, because "test that ... " requires a complete sentence after that, right ? (3) SnapBuildRestore snapshot_not_interesting: if (ondisk.builder.committed.xip != NULL) pfree(ondisk.builder.committed.xip); return false; } Do we need to add pfree for ondisk.builder.catchange.xip after the 'snapshot_not_interesting' label ? (4) SnapBuildPurgeOlderTxn + elog(DEBUG3, "purged catalog modifying transactions from %d to %d", +(uint32) builder->catchange.xcnt, surviving_xids); To make this part more aligned with existing codes, probably we can have a look at another elog for debug in the same function. We should use %u for casted xcnt & surviving_xids, while adding a format for xmin if necessary ? Best Regards, Takamichi Osumi
RE: Re-order "disable_on_error" in tab-complete COMPLETE_WITH
On Tuesday, July 5, 2022 1:05 PM Amit Kapila wrote: > On Tue, Jul 5, 2022 at 4:03 AM Peter Smith wrote: > > > > On Mon, Jul 4, 2022 at 10:29 PM Euler Taveira wrote: > > > > > > On Mon, Jul 4, 2022, at 5:37 AM, Amit Kapila wrote: > > > > > > Yeah, it seems we have overlooked this point. I think we can do this > > > just for HEAD but as the feature is introduced in PG-15 so there is > > > no harm in pushing it to PG-15 as well especially because it is a > > > straightforward change. What do you or others think? > > > > > > No objection. It is a good thing for future backpatches. > > > > > > > Since there is no function change or bugfix here I thought it was only > > applicable for HEAD. This change is almost in the same category as a > > code comment typo patch - do those normally get backpatched? - maybe > > follow the same convention here. OTOH, if you think it may be helpful > > for future backpatches then I am also fine if you wanted to push it to > > PG15. > > > > It can help if there is any bug-fix in the same code path or if some other > code > adjustment in the same area is required in the back branch. > I feel the chances of both are less but I just wanted to keep the code > consistent > for such a possibility. Anyway, I'll wait for a day or so and see if anyone > has > objections to it. Thank you all for catching and discussing this fix. I also agree with pushing it to PG-15 for comfortability of future backpatches. Best Regards, Takamichi Osumi
RE: Build-farm - intermittent error in 031_column_list.pl
On Thursday, May 26, 2022 11:37 AM Amit Kapila wrote: > On Wed, May 25, 2022 at 6:54 PM Tomas Vondra > wrote: > > > > On 5/25/22 13:26, Amit Kapila wrote: > > > On Wed, May 25, 2022 at 8:16 AM Kyotaro Horiguchi > > > wrote: > > >> > > >> It does "fix" the case of [1]. But AFAIS > > >> RelationSyncEntry.replicate_valid is only used to inhibit repeated > > >> loading in get_rel_sync_entry and the function doesn't seem to be > > >> assumed to return a invalid entry. (Since the flag is not checked > > >> nowhere else.) > > >> > > >> For example pgoutput_change does not check for the flag of the > > >> entry returned from the function before uses it, which is not > > >> seemingly safe. (I didn't check further, though) > > >> > > >> Don't we need to explicitly avoid using invalid entries outside the > > >> function? > > >> > > > > > > We decide that based on pubactions in the callers, so even if entry > > > is valid, it won't do anything. Actually, we don't need to avoid > > > setting replication_valid flag as some of the publications for the > > > table may be already present. We can check if the publications_valid > > > flag is set while trying to validate the entry. Now, even if we > > > don't find any publications the replicate_valid flag will be set but > > > none of the actions will be set, so it won't do anything in the > > > caller. Is this better than the previous approach? > > > > > > > For the record, I'm not convinced this is the right way to fix the > > issue, as it may easily mask the real problem. > > > > We do silently ignore missing objects in various places, but only when > > either requested or when it's obvious it's expected and safe to ignore. > > But I'm not sure that applies here, in a clean way. > > > > Imagine you have a subscriber using two publications p1 and p2, and > > someone comes around and drops p1 by mistake. With the proposed patch, > > the subscription will notice this, but it'll continue sending data > > ignoring the missing publication. Yes, it will continue working, but > > it's quite possible this breaks the subscriber and it's be better to > > fail and stop replicating. > > > > Ideally, shouldn't we disallow drop of publication in such cases where it is > part > of some subscription? I know it will be tricky because some subscriptions > could be disabled. > > > The other aspect I dislike is that we just stop caching publication > > info, forcing us to reload it for every replicated change/row. So even > > if dropping the publication happens not to "break" the subscriber (i.e. > > the data makes sense), this may easily cause performance issues, lag > > in the replication, and so on. And the users will have no idea why > > and/or how to fix it, because we just do this silently. > > > > Yeah, this is true that if there are missing publications, it needs to reload > all the > publications info again unless we build a mechanism to change the existing > cached entry by loading only required info. The other thing we could do here > is > to LOG the info for missing publications to make users aware of the fact. I > think > we can also introduce a new option while defining/altering subscription to > indicate whether to continue on missing publication or not, that way by > default > we will stop replication as we are doing now but users will have a way to move > replication. > > BTW, what are the other options we have to fix the cases where replication is > broken (or the user has no clue on how to proceed) as we are discussing the > case here or the OP reported yet another case on pgsql-bugs [1]? Hi, FYI, I've noticed that after the last report by Peter-san we've gotten the same errors on Build Farm. We need to keep discussing to conclude this. 1. Details for system "xenodermus" failure at stage subscriptionCheck, snapshot taken 2022-05-31 13:00:04 https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=xenodermus=2022-05-31%2013%3A00%3A04 2. Details for system "phycodurus" failure at stage subscriptionCheck, snapshot taken 2022-05-26 17:30:04 https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=phycodurus=2022-05-26%2017%3A30%3A04 Best Regards, Takamichi Osumi
RE: Perform streaming logical transactions by background workers and parallel apply
On Wednesday, May 25, 2022 11:25 AM wangw.f...@fujitsu.com
wrote:
> Attach the patches. (Did not change v6-0001 and v6-0002.)
Hi,
Some review comments on the new patches from v6-0001 to v6-0004.
(1) create_subscription.sgml
+ the transaction is committed. Note that if an error happens when
+ applying changes in a background worker, it might not report the
+ finish LSN of the remote transaction in the server log.
I suggest to add a couple of sentences like below
to the section of logical-replication-conflicts in logical-replication.sgml.
"
Setting streaming mode to 'apply' can export invalid LSN as
finish LSN of failed transaction. Changing the streaming mode and
making the same conflict writes the finish LSN of the
failed transaction in the server log if required.
"
(2) ApplyBgworkerMain
+ PG_TRY();
+ {
+ LogicalApplyBgwLoop(mqh, pst);
+ }
+ PG_CATCH();
+ {
...
+ pgstat_report_subscription_error(MySubscription->oid, false);
+
+ PG_RE_THROW();
+ }
+ PG_END_TRY();
When I stream a transaction in-progress and it causes an error(duplication
error),
seemingly the subscription stats (values in pg_stat_subscription_stats) don't
get updated properly. The 2nd argument should be true for apply error.
Also, I observe that both apply_error_count and sync_error_count
get updated together by error. I think we need to check this point as well.
(3) logicalrep_write_attrs
+ if (rel->rd_rel->relhasindex)
+ {
+ List *indexoidlist = RelationGetIndexList(rel);
+ ListCell *indexoidscan;
+ foreach(indexoidscan, indexoidlist)
and
+ if (indexRel->rd_index->indisunique)
+ {
+ int i;
+ /* Add referenced attributes to idindexattrs */
+ for (i = 0; i < indexRel->rd_index->indnatts;
i++)
We don't have each blank line after variable declarations.
There might be some other codes where this point can be applied.
Please check.
(4)
+ /*
+* If any unique index exist, check that they are same as remoterel.
+*/
+ if (!rel->sameunique)
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+errmsg("cannot replicate relation with
different unique index"),
+errhint("Please change the streaming option to
'on' instead of 'apply'.")));
When I create a logical replication setup with different constraints
and let streaming of in-progress transaction run,
I keep getting this error.
This should be documented as a restriction or something,
to let users know the replication progress can't go forward by
any differences written like in the commit-message in v6-0003.
Also, it would be preferable to test this as well, if we
don't dislike having TAP tests for this.
Best Regards,
Takamichi Osumi
RE: Skipping schema changes in publication
On Monday, May 23, 2022 2:13 PM vignesh C wrote:
> Attached v7 patch which fixes the buildfarm warning for an unused warning in
> release mode as in [1].
Hi, thank you for the patches.
I'll share several review comments.
For v7-0001.
(1) I'll suggest some minor rewording.
+
+ The RESET clause will reset the publication to the
+ default state which includes resetting the publication options, setting
+ ALL TABLES flag to false and
+ dropping all relations and schemas that are associated with the publication.
My suggestion is
"The RESET clause will reset the publication to the
default state. It resets the publication operations,
sets ALL TABLES flag to false and drops all relations
and schemas associated with the publication."
(2) typo and rewording
+/*
+ * Reset the publication.
+ *
+ * Reset the publication options, setting ALL TABLES flag to false and drop
+ * all relations and schemas that are associated with the publication.
+ */
The "setting" in this sentence should be "set".
How about changing like below ?
FROM:
"Reset the publication options, setting ALL TABLES flag to false and drop
all relations and schemas that are associated with the publication."
TO:
"Reset the publication operations, set ALL TABLES flag to false and drop
all relations and schemas associated with the publication."
(3) AlterPublicationReset
Do we need to call CacheInvalidateRelcacheAll() or
InvalidatePublicationRels() at the end of
AlterPublicationReset() like AlterPublicationOptions() ?
For v7-0002.
(4)
+ if (stmt->for_all_tables)
+ {
+ boolisdefault = CheckPublicationDefValues(tup);
+
+ if (!isdefault)
+ ereport(ERROR,
+
errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
+ errmsg("adding ALL TABLES requires the
publication to have default publication options, no tables/
+ errhint("Use ALTER PUBLICATION ...
RESET to reset the publication"));
The errmsg string has three messages for user and is a bit long
(we have two sentences there connected by 'and').
Can't we make it concise and split it into a couple of lines for code
readability ?
I'll suggest a change below.
FROM:
"adding ALL TABLES requires the publication to have default publication
options, no tables/schemas associated and ALL TABLES flag should not be set"
TO:
"adding ALL TABLES requires the publication defined not for ALL TABLES"
"to have default publish actions without any associated tables/schemas"
(5) typo
+EXCEPT TABLE
+
+
+ This clause specifies a list of tables to exclude from the publication.
+ It can only be used with FOR ALL TABLES.
+
+
+
+
Kindly change
FROM:
This clause specifies a list of tables to exclude from the publication.
TO:
This clause specifies a list of tables to be excluded from the publication.
or
This clause specifies a list of tables excluded from the publication.
(6) Minor suggestion for an expression change
Marks the publication as one that replicates changes for all tables in
- the database, including tables created in the future.
+ the database, including tables created in the future. If
+ EXCEPT TABLE is specified, then exclude replicating
+ the changes for the specified tables.
I'll suggest a minor rewording.
FROM:
...exclude replicating the changes for the specified tables
TO:
...exclude replication changes for the specified tables
(7)
(7-1)
+/*
+ * Check if the publication has default values
+ *
+ * Check the following:
+ * a) Publication is not set with "FOR ALL TABLES"
+ * b) Publication is having default options
+ * c) Publication is not associated with schemas
+ * d) Publication is not associated with relations
+ */
+static bool
+CheckPublicationDefValues(HeapTuple tup)
I think this header comment can be improved.
FROM:
Check the following:
TO:
Returns true if the publication satisfies all the following conditions:
(7-2)
b) should be changed as well
FROM:
Publication is having default options
TO:
Publication has the default publish operations
Best Regards,
Takamichi Osumi
RE: Build-farm - intermittent error in 031_column_list.pl
On Tuesday, May 24, 2022 9:50 PM Amit Kapila wrote: > On Sat, May 21, 2022 at 9:03 AM Amit Kapila > wrote: > > > > On Fri, May 20, 2022 at 4:01 PM Tomas Vondra > > wrote: > > > > > Also, we'd probably have to ignore RelationSyncEntry for a while, > > > which seems quite expensive. > > > > > > > Yet another option could be that we continue using a historic snapshot > > but ignore publications that are not found for the purpose of > > computing RelSyncEntry attributes. We won't mark such an entry as > > valid till all the publications are loaded without anything missing. I > > think such cases in practice won't be enough to matter. This means we > > won't publish operations on tables corresponding to that publication > > till we found such a publication and that seems okay. > > > > Attached, find the patch to show what I have in mind for this. Today, we have > received a bug report with a similar symptom [1] and that should also be fixed > with this. The reported bug should also be fixed with this. > > Thoughts? Hi, I agree with this direction. I think this approach solves the issue fundamentally and is better than the first approach to add several calls of wait_for_catchup in the test, since taking the first one means we need to care about avoiding the same issue, whenever we write a new (similar) test, even after the modification. I've used the patch to check below things. 1. The patch can be applied and make check-world has passed without failure. 2. HEAD applied with the patch passed all tests in src/test/subscription (including 031_column_list.pl), after commenting out of WalSndWaitForWal's WalSndKeepalive. 3. The new bug fix report in 'How is this possible "publication does not exist"' thread has been fixed. FYI, after I execute the script's function, I also conduct additional insert to the publisher, and this was correctly replicated on the subscriber. Best Regards, Takamichi Osumi
RE: Build-farm - intermittent error in 031_column_list.pl
On Thursday, May 19, 2022 8:13 PM Amit Kapila wrote:
> On Thu, May 19, 2022 at 3:16 PM Amit Kapila
> wrote:
> >
> > On Thu, May 19, 2022 at 12:28 PM Kyotaro Horiguchi
> > wrote:
> > >
> > > At Thu, 19 May 2022 14:26:56 +1000, Peter Smith
> > > wrote in
> > > > Hi hackers.
> > > >
> > > > FYI, I saw that there was a recent Build-farm error on the
> > > > "grison" machine [1] [1]
> > > > https://buildfarm.postgresql.org/cgi-bin/show_history.pl?nm=grison
> > > > =HEAD
> > > >
> > > > The error happened during "subscriptionCheck" phase in the TAP
> > > > test t/031_column_list.pl This test file was added by this [2]
> > > > commit.
> > > > [2]
> > > >
> https://github.com/postgres/postgres/commit/923def9a533a7d986acfb5
> > > > 24139d8b9e5466d0a5
> > >
> > > 2022-04-17 00:16:04.278 CEST [293659][client
> > > backend][4/270:0][031_column_list.pl] LOG: statement: CREATE
> > > PUBLICATION pub9 FOR TABLE test_part_d (a) WITH
> > > (publish_via_partition_root = true);
> > > 2022-04-17 00:16:04.279 CEST [293659][client
> > > backend][:0][031_column_list.pl] LOG: disconnection: session time:
> > > 0:00:00.002 user=bf database=postgres host=[local]
> > >
> > > "CREATE PUBLICATION pub9" is executed at 00:16:04.278 on 293659 then
> > > the session has been disconnected. But the following request for the
> > > same publication fails due to the absense of the publication.
> > >
> > > 2022-04-17 00:16:08.147 CEST [293856][walsender][3/0:0][sub1]
> > > STATEMENT: START_REPLICATION SLOT "sub1" LOGICAL 0/153DB88
> > > (proto_version '3', publication_names '"pub9"')
> > > 2022-04-17 00:16:08.148 CEST [293856][walsender][3/0:0][sub1] ERROR:
> > > publication "pub9" does not exist
> > >
> >
> > This happens after "ALTER SUBSCRIPTION sub1 SET PUBLICATION pub9".
> The
> > probable theory is that ALTER SUBSCRIPTION will lead to restarting of
> > apply worker (which we can see in LOGS as well) and after the restart,
> > the apply worker will use the existing slot and replication origin
> > corresponding to the subscription. Now, it is possible that before
> > restart the origin has not been updated and the WAL start location
> > points to a location prior to where PUBLICATION pub9 exists which can
> > lead to such an error. Once this error occurs, apply worker will never
> > be able to proceed and will always return the same error. Does this
> > make sense?
> >
> > Unless you or others see a different theory, this seems to be the
> > existing problem in logical replication which is manifested by this
> > test. If we just want to fix these test failures, we can create a new
> > subscription instead of altering the existing publication to point to
> > the new publication.
> >
>
> If the above theory is correct then I think allowing the publisher to catch
> up with
> "$node_publisher->wait_for_catchup('sub1');" before ALTER SUBSCRIPTION
> should fix this problem. Because if before ALTER both publisher and
> subscriber are in sync then the new publication should be visible to
> WALSender.
Hi,
I've attached a patch for the fix proposed here.
First of all, thank you so much for helping me offlist, Amit-san.
I reproduced the failure like [1] by commenting out
WalSndWaitForWal's call of WalSndKeepalive and running the test.
This comment out intends to suppress the advance of confirmed_flush location
after creating a publication.
In short, my understanding how the bug happened is,
1. we execute 'create publication pubX' and create one publication.
2. 'alter subscription subY set publication pubX' makes the apply worker exit
3. relaunched apply worker searches for pubX. But, the slot
position(confirmed_flush)
doesn't get updated and points to some location before create publication at
the publisher node.
Applying the attached patch have made the test pass.
[1] the subscriber's log
2022-05-20 08:56:50.773 UTC [5153] 031_column_list.pl LOG: statement: ALTER
SUBSCRIPTION sub1 SET PUBLICATION pub6
2022-05-20 08:56:50.801 UTC [5156] 031_column_list.pl LOG: statement: SELECT
count(1) = 0 FROM pg_subscription_rel WHERE srsubstate NOT IN ('r', 's');
2022-05-20 08:56:50.846 UTC [5112] LOG: logical replication apply worker for
subscription "sub1" will restart because of a parameter change
2022-05-20 08:56:50.915 UTC [5158] 031_column_list.pl LOG: statement: SELECT
count(1) = 0 FROM pg_subscription_rel WHERE srsubstate NOT IN ('r', 's');
...
2022-05-20 08:56:51.257 UTC [5164] 031_column_list.pl LOG: statement: SELECT
count(1) = 0 FROM pg_subscription_rel WHERE srsubstate NOT IN ('r', 's');
2022-05-20 08:56:51.353 UTC [5166] LOG: logical replication apply worker for
subscription "sub1" has started
2022-05-20 08:56:51.366 UTC [5168] LOG: logical replication table
synchronization worker for subscription "sub1", table "test_part_a" has started
2022-05-20 08:56:51.370 UTC [5171] 031_column_list.pl LOG: statement: SELECT
count(1) = 0 FROM pg_subscription_rel WHERE srsubstate NOT IN ('r', 's');
2022-05-20 08:56:51.373 UTC [5166]
RE: Skipping schema changes in publication
On Thursday, May 19, 2022 2:45 AM vignesh C wrote: > On Mon, May 16, 2022 at 8:32 AM osumi.takami...@fujitsu.com > wrote: > > (3) src/test/regress/expected/publication.out > > > > +-- Verify that only superuser can reset a publication ALTER > > +PUBLICATION testpub_reset OWNER TO regress_publication_user2; SET > > +ROLE regress_publication_user2; ALTER PUBLICATION testpub_reset > > +RESET; -- fail > > > > > > We have "-- fail" for one case in this patch. > > On the other hand, isn't better to add "-- ok" (or "-- success") for > > other successful statements, when we consider the entire tests > > description consistency ? > > We generally do not mention success comments for all the success cases as > that might be an overkill. I felt it is better to keep it as it is. > Thoughts? Thank you for updating the patches ! In terms of this point, I meant to say we add "-- ok" for each successful "ALTER PUBLICATION testpub_reset RESET;" statement. That means, we'll have just three places to add "--ok" and I thought this was not an overkill. *But*, I'm also OK with your idea. Please don't change the comments and keep them as it is like v6. Best Regards, Takamichi Osumi
RE: bogus: logical replication rows/cols combinations
On Monday, May 16, 2022 9:34 PM houzj.f...@fujitsu.com
wrote:
> Attach the new version patch.
Hi,
I have few minor comments.
For v2-0001.
(1) Unnecessary period at the end of column explanation
+
+
+ rowfilter text
+
+
+ Expression for the table's publication qualifying condition.
+
+
It seems when we write a simple noun to explain a column,
we don't need to put a period at the end of the explanation.
Kindly change
FROM:
"Expression for the table's publication qualifying condition."
TO:
"Expression for the table's publication qualifying condition"
For v2-0002.
(a) typo in the commit message
Kindly change
FROM:
"In both cases, it doesn't seems to make sense to combine column lists."
TO:
"In both cases, it doesn't seem to make sense to combine column lists."
or "In both cases, it doesn't make sense to combine column lists."
(b) fetch_table_list
+ if (list_member(tablelist, rv))
+ ereport(ERROR,
+ errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot use different column
lists for table \"%s.%s\" in different publications",
+ nspname, relname));
Kindly add tests for new error paths, when we add them.
Best Regards,
Takamichi Osumi
RE: Skipping schema changes in publication
On Saturday, May 14, 2022 10:33 PM vignesh C wrote:
> Thanks for the comments, the attached v5 patch has the changes for the same.
> Also I have made the changes for SKIP Table based on the new syntax, the
> changes for the same are available in
> v5-0002-Skip-publishing-the-tables-specified-in-EXCEPT-TA.patch.
Hi,
Several comments on v5-0002.
(1) One unnecessary space before "except_pub_obj_list" syntax definition
+ except_pub_obj_list: ExceptPublicationObjSpec
+ { $$ = list_make1($1); }
+ | except_pub_obj_list ',' ExceptPublicationObjSpec
+ { $$ = lappend($1, $3); }
+ | /*EMPTY*/
{ $$ = NULL; }
+ ;
+
From above part, kindly change
FROM:
" except_pub_obj_list: ExceptPublicationObjSpec"
TO:
"except_pub_obj_list: ExceptPublicationObjSpec"
(2) doc/src/sgml/ref/create_publication.sgml
(2-1)
@@ -22,7 +22,7 @@ PostgreSQL documentation
CREATE PUBLICATION name
-[ FOR ALL TABLES
+[ FOR ALL TABLES [EXCEPT TABLE [ ONLY ] table_name [ * ] [, ... ]]
| FOR publication_object [,
... ] ]
[ WITH ( publication_parameter [= value] [, ... ] ) ]
Here I think we need to add two more whitespaces around square brackets.
Please change
FROM:
"[ FOR ALL TABLES [EXCEPT TABLE [ ONLY ] table_name [ * ] [, ... ]]"
TO:
"[ FOR ALL TABLES [ EXCEPT TABLE [ ONLY ] table_name [ * ] [, ... ] ]"
When I check other documentations, I see whitespaces before/after square
brackets.
(2-2)
This whitespace alignment applies to alter_publication.sgml as well.
(3)
@@ -156,6 +156,24 @@ CREATE PUBLICATION name
+
+
+EXCEPT TABLE
+
+
+ Marks the publication as one that excludes replicating changes for the
+ specified tables.
+
+
+
+ EXCEPT TABLE can be specified only for
+ FOR ALL TABLES publication. It is not supported for
+ FOR ALL TABLES IN SCHEMA publication and
+ FOR TABLE publication.
+
+
+
+
This EXCEPT TABLE clause is only for FOR ALL TABLES.
So, how about extracting the main message from above part and
moving it to an exising paragraph below, instead of having one independent
paragraph ?
FOR ALL TABLES
Marks the publication as one that replicates changes for all tables in
the database, including tables created in the future.
Something like
"Marks the publication as one that replicates changes for all tables in
the database, including tables created in the future. EXCEPT TABLE indicates
excluded tables for the defined publication.
"
(4) One minor confirmation about the syntax
Currently, we allow one way of writing to indicate excluded tables like below.
(example) CREATE PUBLICATION mypub FOR ALL TABLES EXCEPT TABLE tab3, tab4,
EXCEPT TABLE tab5;
This is because we define ExceptPublicationObjSpec with EXCEPT TABLE.
Is it OK to have a room to write duplicate "EXCEPT TABLE" clauses ?
I think there is no harm in having this,
but I'd like to confirm whether this syntax might be better to be adjusted or
not.
(5) CheckAlterPublication
+
+ if (excepttable && !stmt->for_all_tables)
+ ereport(ERROR,
+
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+errmsg("publication \"%s\" is not defined as
FOR ALL TABLES",
+ NameStr(pubform->pubname)),
+errdetail("except table cannot be added to,
dropped from, or set on NON ALL TABLES publications.")));
Could you please add a test for this ?
Best Regards,
Takamichi Osumi
RE: Skipping schema changes in publication
On Saturday, May 14, 2022 10:33 PM vignesh C wrote: > Thanks for the comments, the attached v5 patch has the changes for the same. > Also I have made the changes for SKIP Table based on the new syntax, the > changes for the same are available in > v5-0002-Skip-publishing-the-tables-specified-in-EXCEPT-TA.patch. Hi, Thank you for updating the patch. I'll share few minor review comments on v5-0001. (1) doc/src/sgml/ref/alter_publication.sgml @@ -73,12 +85,13 @@ ALTER PUBLICATION name RENAME TO ADD ALL TABLES IN SCHEMA and SET ALL TABLES IN SCHEMA to a publication requires the - invoking user to be a superuser. To alter the owner, you must also be a - direct or indirect member of the new owning role. The new owner must have - CREATE privilege on the database. Also, the new owner - of a FOR ALL TABLES or FOR ALL TABLES IN - SCHEMA publication must be a superuser. However, a superuser can - change the ownership of a publication regardless of these restrictions. + invoking user to be a superuser. RESET of publication + requires the invoking user to be a superuser. To alter the owner, you must ... I suggest to combine the first part of your change with one existing sentence before your change, to make our description concise. FROM: "The ADD ALL TABLES IN SCHEMA and SET ALL TABLES IN SCHEMA to a publication requires the invoking user to be a superuser. RESET of publication requires the invoking user to be a superuser." TO: "The ADD ALL TABLES IN SCHEMA, SET ALL TABLES IN SCHEMA to a publication and RESET of publication requires the invoking user to be a superuser." (2) typo +++ b/src/backend/commands/publicationcmds.c @@ -53,6 +53,13 @@ #include "utils/syscache.h" #include "utils/varlena.h" +#define PUB_ATION_INSERT_DEFAULT true +#define PUB_ACTION_UPDATE_DEFAULT true Kindly change FROM: "PUB_ATION_INSERT_DEFAULT" TO: "PUB_ACTION_INSERT_DEFAULT" (3) src/test/regress/expected/publication.out +-- Verify that only superuser can reset a publication +ALTER PUBLICATION testpub_reset OWNER TO regress_publication_user2; +SET ROLE regress_publication_user2; +ALTER PUBLICATION testpub_reset RESET; -- fail We have "-- fail" for one case in this patch. On the other hand, isn't better to add "-- ok" (or "-- success") for other successful statements, when we consider the entire tests description consistency ? Best Regards, Takamichi Osumi
RE: First draft of the PG 15 release notes
On Saturday, May 14, 2022 12:07 AM 'Bruce Momjian' wrote: > On Fri, May 13, 2022 at 01:36:04AM +0000, osumi.takami...@fujitsu.com wrote: > > > > > > This is enabled with the subscriber option "disable_on_error" > > > and avoids possible infinite loops during stream application. > > > > > > > > Thank you ! > > > > In this last paragraph, how about replacing "infinite loops" > > with "infinite error loops" ? I think it makes the situation somewhat > > clear for readers. > > Agreed, done. Thanks ! Best Regards, Takamichi Osumi
RE: Data is copied twice when specifying both child and parent table in publication
On Friday, May 13, 2022 6:42 PM Wang, Wei/王 威 wrote:
> Attach the patches.(Only changed the patch for HEAD.).
> 1. Optimize the code. Reduce calls to function filter_partitions.
> [suggestions by
> Amit-san] 2. Improve the alias name in SQL. [suggestions by Amit-san] 3.
> Improve coding alignments. [suggestions by Amit-san] 4. Do some
> optimizations for list Concatenate.
Hi, thank you for updating the patch.
I have one minor comment on fetch_table_list() in HEAD v4.
@@ -1759,17 +1759,22 @@ static List *
fetch_table_list(WalReceiverConn *wrconn, List *publications)
{
WalRcvExecResult *res;
- StringInfoData cmd;
+ StringInfoData cmd,
+ pub_names;
TupleTableSlot *slot;
Oid tableRow[2] = {TEXTOID, TEXTOID};
List *tablelist = NIL;
+ initStringInfo(_names);
+ get_publications_str(publications, _names, true);
+
Kindly free the pub_names's data along with the cmd.data.
Best Regards,
Takamichi Osumi
RE: First draft of the PG 15 release notes
On Thursday, May 12, 2022 11:10 PM 'Bruce Momjian' wrote: > On Thu, May 12, 2022 at 01:35:39PM +0000, osumi.takami...@fujitsu.com > wrote: > > I'd like to suggest that we mention a new option for subscription > 'disable_on_error'. > > > > > > > https://github.com/postgres/postgres/commit/705e20f8550c0e8e47c0b6b20 > b5f5ffd6ffd9e33 > > Yes, I missed that one, added: > > > > > > Allow subscribers to stop logical replication application on error > (Osumi Takamichi, Mark Dilger) > > > > This is enabled with the subscriber option "disable_on_error" > and avoids possible infinite loops during stream application. > > Thank you ! In this last paragraph, how about replacing "infinite loops" with "infinite error loops" ? I think it makes the situation somewhat clear for readers. Best Regards, Takamichi Osumi
RE: First draft of the PG 15 release notes
On Wednesday, May 11, 2022 12:44 AM Bruce Momjian wrote: > I have completed the first draft of the PG 15 release notes and you can see > the > results here: > > https://momjian.us/pgsql_docs/release-15.html > > The feature count is similar to recent major releases: > > release-10 195 > release-11 185 > release-12 198 > release-13 183 > release-14 229 > --> release-15 186 > > I assume there will be major adjustments in the next few weeks based on > feedback. Hi, I'd like to suggest that we mention a new option for subscription 'disable_on_error'. https://github.com/postgres/postgres/commit/705e20f8550c0e8e47c0b6b20b5f5ffd6ffd9e33 Best Regards, Takamichi Osumi
RE: Data is copied twice when specifying both child and parent table in publication
On Wednesday, May 11, 2022 11:33 AM I wrote:
> On Monday, May 9, 2022 10:51 AM wangw.f...@fujitsu.com
> wrote:
> > Attach new patches.
> > The patch for HEAD:
> > 1. Modify the approach. Enhance the API of function
> > pg_get_publication_tables to handle one publication or an array of
> > publications.
> > The patch for REL14:
> > 1. Improve the table sync SQL. [suggestions by Shi yu]
> Hi, thank you for updating the patch !
>
> Minor comments on your patch for HEAD v2.
>
> (1) commit message sentence
>
> I suggest below sentence.
>
> Kindly change from
> "... when subscribing to both publications using one subscription, the data is
> replicated twice in inital copy"
> to "subscribing to both publications from one subscription causes initial copy
> twice".
>
> (2) unused variable
>
> pg_publication.c: In function ‘pg_get_publication_tables’:
> pg_publication.c:1091:11: warning: unused variable ‘pubname’
> [-Wunused-variable]
> char*pubname;
>
> We can remove this.
>
> (3) free of allocated memory
>
> In the pg_get_publication_tables(),
> we don't free 'elems'. Don't we need it ?
>
> (4) some coding alignments
>
> 4-1.
>
> + List *tables_viaroot = NIL,
> ...
> + *current_table = NIL;
>
> I suggest we can put some variables
> into the condition for the first time call of this function, like
> tables_viaroot and
> current_table.
> When you agree, kindly change it.
>
> 4-2.
>
> + /*
> +* Publications support partitioned tables, although
> all changes
> +* are replicated using leaf partition identity and
> schema, so we
> +* only need those.
> +*/
> + if (publication->alltables)
> + {
> + current_table =
> GetAllTablesPublicationRelations(publication->pubviaroot);
> + }
>
> This is not related to the change itself and now we are inheriting the
> previous
> curly brackets, but I think there's no harm in removing it, since it's only
> for one
> statement.
Hi,
One more thing I'd like to add is that
we don't hit the below code by tests.
In the HEAD v2, we add a new filtering logic in pg_get_publication_tables.
Although I'm not sure if this is related to the bug fix itself,
when we want to include it in this patch, then
I feel it's better to add some simple test for this part,
to cover all the new main paths and check if
new logic works correctly.
+ /*
+* If a partition table is published in a publication with
viaroot,
+* and its parent or child table is published in another
publication
+* without viaroot. Then we need to move the parent or child
table
+* from tables to tables_viaroot.
+*
+* If all publication(s)'s viaroot are the same, then skip this
part.
+*/
if (ancestor_viaroot == ancestor)
+ {
+ tables =
foreach_delete_current(tables, lc2);
+ change_tables =
list_append_unique_oid(change_tables,
+
relid);
+ }
Best Regards,
Takamichi Osumi
RE: Data is copied twice when specifying both child and parent table in publication
On Monday, May 9, 2022 10:51 AM wangw.f...@fujitsu.com
wrote:
> Attach new patches.
> The patch for HEAD:
> 1. Modify the approach. Enhance the API of function
> pg_get_publication_tables to handle one publication or an array of
> publications.
> The patch for REL14:
> 1. Improve the table sync SQL. [suggestions by Shi yu]
Hi, thank you for updating the patch !
Minor comments on your patch for HEAD v2.
(1) commit message sentence
I suggest below sentence.
Kindly change from
"... when subscribing to both publications using one subscription, the data is
replicated
twice in inital copy"
to "subscribing to both publications from one subscription causes initial copy
twice".
(2) unused variable
pg_publication.c: In function ‘pg_get_publication_tables’:
pg_publication.c:1091:11: warning: unused variable ‘pubname’ [-Wunused-variable]
char*pubname;
We can remove this.
(3) free of allocated memory
In the pg_get_publication_tables(),
we don't free 'elems'. Don't we need it ?
(4) some coding alignments
4-1.
+ List *tables_viaroot = NIL,
...
+ *current_table = NIL;
I suggest we can put some variables
into the condition for the first time call of this function,
like tables_viaroot and current_table.
When you agree, kindly change it.
4-2.
+ /*
+* Publications support partitioned tables, although
all changes
+* are replicated using leaf partition identity and
schema, so we
+* only need those.
+*/
+ if (publication->alltables)
+ {
+ current_table =
GetAllTablesPublicationRelations(publication->pubviaroot);
+ }
This is not related to the change itself and now
we are inheriting the previous curly brackets, but
I think there's no harm in removing it, since it's only for one statement.
Best Regards,
Takamichi Osumi
RE: Skipping schema changes in publication
On Wednesday, April 27, 2022 9:50 PM vignesh C wrote:
> Thanks for the comments, the attached v3 patch has the changes for the same.
Hi
Thank you for updating the patch. Several minor comments on v3.
(1) commit message
The new syntax allows specifying schemas. For example:
CREATE PUBLICATION pub1 FOR ALL TABLES EXCEPT TABLE t1,t2;
OR
ALTER PUBLICATION pub1 ADD EXCEPT TABLE t1,t2;
We have above sentence, but it looks better
to make the description a bit more accurate.
Kindly change
From :
"The new syntax allows specifying schemas"
To :
"The new syntax allows specifying excluded relations"
Also, kindly change "OR" to "or",
because this description is not syntax.
(2) publication_add_relation
@@ -396,6 +400,9 @@ publication_add_relation(Oid pubid, PublicationRelInfo *pri,
ObjectIdGetDatum(pubid);
values[Anum_pg_publication_rel_prrelid - 1] =
ObjectIdGetDatum(relid);
+ values[Anum_pg_publication_rel_prexcept - 1] =
+ BoolGetDatum(pri->except);
+
/* Add qualifications, if available */
It would be better to remove the blank line,
because with this change, we'll have two blank
lines in a row.
(3) pg_dump.h & pg_dump_sort.c
@@ -80,6 +80,7 @@ typedef enum
DO_REFRESH_MATVIEW,
DO_POLICY,
DO_PUBLICATION,
+ DO_PUBLICATION_EXCEPT_REL,
DO_PUBLICATION_REL,
DO_PUBLICATION_TABLE_IN_SCHEMA,
DO_SUBSCRIPTION
@@ -90,6 +90,7 @@ enum dbObjectTypePriorities
PRIO_FK_CONSTRAINT,
PRIO_POLICY,
PRIO_PUBLICATION,
+ PRIO_PUBLICATION_EXCEPT_REL,
PRIO_PUBLICATION_REL,
PRIO_PUBLICATION_TABLE_IN_SCHEMA,
PRIO_SUBSCRIPTION,
@@ -144,6 +145,7 @@ static const int dbObjectTypePriority[] =
PRIO_REFRESH_MATVIEW, /* DO_REFRESH_MATVIEW */
PRIO_POLICY,/* DO_POLICY */
PRIO_PUBLICATION, /* DO_PUBLICATION */
+ PRIO_PUBLICATION_EXCEPT_REL,/* DO_PUBLICATION_EXCEPT_REL */
PRIO_PUBLICATION_REL, /* DO_PUBLICATION_REL */
PRIO_PUBLICATION_TABLE_IN_SCHEMA, /*
DO_PUBLICATION_TABLE_IN_SCHEMA */
PRIO_SUBSCRIPTION /* DO_SUBSCRIPTION */
How about having similar order between
pg_dump.h and pg_dump_sort.c, like
we'll add DO_PUBLICATION_EXCEPT_REL
after DO_PUBLICATION_REL in pg_dump.h ?
(4) GetAllTablesPublicationRelations
+ /*
+* pg_publication_rel and pg_publication_namespace will only have
except
+* tables in case of all tables publication, no need to pass except flag
+* to get the relations.
+*/
+ List *exceptpubtablelist = GetPublicationRelations(pubid,
PUBLICATION_PART_ALL);
+
There is one unnecessary space in a comment
"...pg_publication_namespace will only have...". Kindly remove it.
Then, how about diving the variable declaration and
the insertion of the return value of GetPublicationRelations ?
That might be aligned with other places in this file.
(5) GetTopMostAncestorInPublication
@@ -302,8 +303,9 @@ GetTopMostAncestorInPublication(Oid puboid, List
*ancestors, int *ancestor_level
foreach(lc, ancestors)
{
Oid ancestor = lfirst_oid(lc);
- List *apubids = GetRelationPublications(ancestor);
+ List *apubids = GetRelationPublications(ancestor, false);
List *aschemaPubids = NIL;
+ List *aexceptpubids;
level++;
@@ -317,7 +319,9 @@ GetTopMostAncestorInPublication(Oid puboid, List
*ancestors, int *ancestor_level
else
{
aschemaPubids =
GetSchemaPublications(get_rel_namespace(ancestor));
- if (list_member_oid(aschemaPubids, puboid))
+ aexceptpubids = GetRelationPublications(ancestor, true);
+ if (list_member_oid(aschemaPubids, puboid) ||
+ (puballtables &&
!list_member_oid(aexceptpubids, puboid)))
{
topmost_relid = ancestor;
It seems we forgot to call list_free for "aexceptpubids".
Best Regards,
Takamichi Osumi
RE: Skipping schema changes in publication
On Thursday, April 21, 2022 12:15 PM vignesh C wrote:
> Updated patch by changing the syntax to use EXCEPT instead of SKIP.
Hi
This is my review comments on the v2 patch.
(1) gram.y
I think we can make a unified function that merges
preprocess_alltables_pubobj_list with check_except_in_pubobj_list.
With regard to preprocess_alltables_pubobj_list,
we don't use the 2nd argument "location" in this function.
(2) create_publication.sgml
+
+ Create a publication that publishes all changes in all the tables except for
+ the changes of users and
+ departments table;
This sentence should end ":" not ";".
(3) publication.out & publication.sql
+-- fail - can't set except table to schema publication
+ALTER PUBLICATION testpub_forschema SET EXCEPT TABLE testpub_tbl1;
There is one unnecessary space in the comment.
Kindly change from "schema publication" to "schema publication".
(4) pg_dump.c & describe.c
In your first email of this thread, you explained this feature
is for PG16. Don't we need additional branch for PG16 ?
@@ -6322,6 +6328,21 @@ describePublications(const char *pattern)
}
}
+ if (pset.sversion >= 15)
+ {
@@ -4162,7 +4164,7 @@ getPublicationTables(Archive *fout, TableInfo tblinfo[],
int numTables)
/* Collect all publication membership info. */
if (fout->remoteVersion >= 15)
appendPQExpBufferStr(query,
-"SELECT tableoid, oid,
prpubid, prrelid, "
+"SELECT tableoid, oid,
prpubid, prrelid, prexcept,"
(5) psql-ref.sgml
+If + is appended to the command name, the tables,
+except tables and schemas associated with each publication are shown as
+well.
I'm not sure if "except tables" is a good description.
I suggest "excluded tables". This applies to the entire patch,
in case if this is reasonable suggestion.
Best Regards,
Takamichi Osumi
RE: Failed transaction statistics to measure the logical replication progress
Hi On Friday, March 25, 2022 2:36 PM Masahiko Sawada wrote: > On Thu, Mar 24, 2022 at 12:30 PM Amit Kapila > wrote: > > > > On Thu, Mar 3, 2022 at 8:58 AM osumi.takami...@fujitsu.com > > wrote: > > > > > > > This patch introduces two new subscription statistics columns > > (apply_commit_count and apply_rollback_count) to the > > pg_stat_subscription_stats view for counting cumulative transactions > > commits/rollbacks for a particular subscription. Now, users can > > already see the total number of xacts committed/rolled back in a > > particular database via pg_stat_database, so this can be considered > > duplicate information. > > Right. ... > > I am not sure if it is worth adding this additional information or how > > useful it will be for users. Does anyone else want to weigh in on > > this? > > > > If nobody else sees value in this then I feel it is better to mark > > this patch as Returned with feedback or Rejected. We can come back to > > it later if we see more demand for this. > > Marking as Returned with feedback makes sense to me. OK. Thank you so much for sharing your opinions, Sawada-san and Amit-san. I changed the status of this entry to "Returned with feedback" accordingly. Best Regards, Takamichi Osumi
RE: Skipping logical replication transactions on subscriber side
On Thursday, March 17, 2022 7:56 PM Masahiko Sawada wrote: On Thu, Mar 17, 2022 at 5:52 PM Amit Kapila > wrote: > > > > On Thu, Mar 17, 2022 at 12:39 PM osumi.takami...@fujitsu.com > > wrote: > > > > > > On Thursday, March 17, 2022 3:04 PM Amit Kapila > wrote: > > > > On Wed, Mar 16, 2022 at 1:53 PM Masahiko Sawada > > > > wrote: > > > > > > > > > > I've attached an updated version patch. > > > > > > > > > > > > > The patch LGTM. I have made minor changes in comments and docs in > > > > the attached patch. Kindly let me know what you think of the attached? > > > Hi, thank you for the patch. Few minor comments. > > > > > > > > > (3) apply_handle_commit_internal > > > > > ... > > > > > > I feel if we move those two functions at the end of the > > > apply_handle_commit and apply_handle_stream_commit, then we will > > > have more aligned codes and improve readability. > > > > > I think we cannot just move them to the end of apply_handle_commit() and > apply_handle_stream_commit(). Because if we do that, we end up missing > updating replication_session_origin_lsn/timestamp when clearing the > subskiplsn if we're skipping a non-stream transaction. > > Basically, the apply worker differently handles 2pc transactions and non-2pc > transactions; we always prepare even empty transactions whereas we don't > commit empty non-2pc transactions. So I think we don’t have to handle both in > the same way. Okay. Thank you so much for your explanation. Then the code looks good to me. Best Regards, Takamichi Osumi
RE: Skipping logical replication transactions on subscriber side
On Thursday, March 17, 2022 3:04 PM Amit Kapila wrote: > On Wed, Mar 16, 2022 at 1:53 PM Masahiko Sawada > wrote: > > > > I've attached an updated version patch. > > > > The patch LGTM. I have made minor changes in comments and docs in the > attached patch. Kindly let me know what you think of the attached? Hi, thank you for the patch. Few minor comments. (1) comment of maybe_start_skipping_changes + /* +* Quick return if it's not requested to skip this transaction. This +* function is called for every remote transaction and we assume that +* skipping the transaction is not used often. +*/ I feel this comment should explain more about our intention and what it confirms. In a case when user requests skip, but it doesn't match the condition, we don't start skipping changes, strictly speaking. From: Quick return if it's not requested to skip this transaction. To: Quick return if we can't ensure possible skiplsn is set and it equals to the finish LSN of this transaction. (2) 029_on_error.pl + my $contents = slurp_file($node_subscriber->logfile, $offset); + $contents =~ + qr/processing remote data for replication origin \"pg_\d+\" during "INSERT" for replication target relation "public.tbl" in transaction \d+ finishe$ + or die "could not get error-LSN"; I think we shouldn't use a lot of new words. How about a change below ? From: could not get error-LSN To: failed to find expected error message that contains finish LSN for SKIP option (3) apply_handle_commit_internal Lastly, may I have the reasons to call both stop_skipping_changes and clear_subscription_skip_lsn in this function, instead of having them at the end of apply_handle_commit and apply_handle_stream_commit ? IMHO, this structure looks to create the extra condition branches in apply_handle_commit_internal. Also, because of this code, when we call stop_skipping_changes in the apply_handle_commit_internal, after checking is_skipping_changes() returns true, we check another is_skipping_changes() at the top of stop_skipping_changes. OTOH, for other cases like apply_handle_prepare, apply_handle_stream_prepare, we call those two functions (or either one) depending on the needs, after existing commits and during the closing processing. (In the case of rollback_prepare, it's also called after existing commit) I feel if we move those two functions at the end of the apply_handle_commit and apply_handle_stream_commit, then we will have more aligned codes and improve readability. Best Regards, Takamichi Osumi
RE: Skipping logical replication transactions on subscriber side
On Wednesday, March 16, 2022 3:37 PM I wrote:
> On Wednesday, March 16, 2022 11:33 AM Amit Kapila
> wrote:
> > On Tue, Mar 15, 2022 at 7:30 PM osumi.takami...@fujitsu.com
> > wrote:
> > >
> > > On Tuesday, March 15, 2022 3:13 PM Masahiko Sawada
> > wrote:
> > > > I've attached an updated version patch.
> > >
> > > A couple of minor comments on v14.
> > >
> > > (1) apply_handle_commit_internal
> > >
> > >
> > > + if (is_skipping_changes())
> > > + {
> > > + stop_skipping_changes();
> > > +
> > > + /*
> > > +* Start a new transaction to clear the subskipxid,
> > > + if not
> > started
> > > +* yet. The transaction is committed below.
> > > +*/
> > > + if (!IsTransactionState())
> > > + StartTransactionCommand();
> > > + }
> > > +
> > >
> > > I suppose we can move this condition check and
> > > stop_skipping_changes() call to the inside of the block we enter
> > > when IsTransactionState() returns
> > true.
> > >
> > > As the comment of apply_handle_commit_internal() mentions, it's the
> > > helper function for apply_handle_commit() and
> > > apply_handle_stream_commit().
> > >
> > > Then, I couldn't think that both callers don't open a transaction
> > > before the call of apply_handle_commit_internal().
> > > For applying spooled messages, we call begin_replication_step as well.
> > >
> > > I can miss something, but timing when we receive COMMIT message
> > > without opening a transaction, would be the case of empty
> > > transactions where the subscription (and its subscription worker) is not
> interested.
> > >
> >
> > I think when we skip non-streamed transactions we don't start a transaction.
> > So, if we do what you are suggesting, we will miss to clear the
> > skip_lsn after skipping the transaction.
> OK, this is what I missed.
>
> On the other hand, what I was worried about is that empty transaction can
> start
> skipping changes, if the subskiplsn is equal to the finish LSN for the empty
> transaction. The reason is we call maybe_start_skipping_changes even for
> empty ones and set skip_xact_finish_lsn by the finish LSN in that case.
>
> I checked I could make this happen with debugger and some logs for LSN.
> What I did is just having two pairs of pub/sub and conduct a change for one of
> them, after I set a breakpoint in the logicalrep_write_begin on the walsender
> that will issue an empty transaction.
> Then, I check the finish LSN of it and
> conduct an alter subscription skip lsn command with this LSN value.
> As a result, empty transaction calls stop_skipping_changes in the
> apply_handle_commit_internal and then enter the block for IsTransactionState
> == true, which would not happen before applying the patch.
>
> Also, this behavior looks contradicted with some comments in worker.c "The
> subskiplsn is cleared after successfully skipping the transaction or applying
> non-empty transaction." so, I was just confused and wrote the above comment.
Sorry, my understanding was not correct.
Even when we clear the subskiplsn by empty transaction,
we can say that it applies to the success of skipping the transaction.
Then this behavior and allowing empty transaction to match the indicated
LSN by alter subscription is fine.
I'm sorry for making noises.
Best Regards,
Takamichi Osumi
RE: Skipping logical replication transactions on subscriber side
On Wednesday, March 16, 2022 11:33 AM Amit Kapila
wrote:
> On Tue, Mar 15, 2022 at 7:30 PM osumi.takami...@fujitsu.com
> wrote:
> >
> > On Tuesday, March 15, 2022 3:13 PM Masahiko Sawada
> wrote:
> > > I've attached an updated version patch.
> >
> > A couple of minor comments on v14.
> >
> > (1) apply_handle_commit_internal
> >
> >
> > + if (is_skipping_changes())
> > + {
> > + stop_skipping_changes();
> > +
> > + /*
> > +* Start a new transaction to clear the subskipxid, if not
> started
> > +* yet. The transaction is committed below.
> > +*/
> > + if (!IsTransactionState())
> > + StartTransactionCommand();
> > + }
> > +
> >
> > I suppose we can move this condition check and stop_skipping_changes()
> > call to the inside of the block we enter when IsTransactionState() returns
> true.
> >
> > As the comment of apply_handle_commit_internal() mentions, it's the
> > helper function for apply_handle_commit() and
> > apply_handle_stream_commit().
> >
> > Then, I couldn't think that both callers don't open a transaction
> > before the call of apply_handle_commit_internal().
> > For applying spooled messages, we call begin_replication_step as well.
> >
> > I can miss something, but timing when we receive COMMIT message
> > without opening a transaction, would be the case of empty transactions
> > where the subscription (and its subscription worker) is not interested.
> >
>
> I think when we skip non-streamed transactions we don't start a transaction.
> So, if we do what you are suggesting, we will miss to clear the skip_lsn after
> skipping the transaction.
OK, this is what I missed.
On the other hand, what I was worried about is that
empty transaction can start skipping changes,
if the subskiplsn is equal to the finish LSN for
the empty transaction. The reason is we call
maybe_start_skipping_changes even for empty ones
and set skip_xact_finish_lsn by the finish LSN in that case.
I checked I could make this happen with debugger and some logs for LSN.
What I did is just having two pairs of pub/sub
and conduct a change for one of them,
after I set a breakpoint in the logicalrep_write_begin
on the walsender that will issue an empty transaction.
Then, I check the finish LSN of it and
conduct an alter subscription skip lsn command with this LSN value.
As a result, empty transaction calls stop_skipping_changes
in the apply_handle_commit_internal and then
enter the block for IsTransactionState == true,
which would not happen before applying the patch.
Also, this behavior looks contradicted with some comments in worker.c
"The subskiplsn is cleared after successfully skipping the transaction
or applying non-empty transaction." so, I was just confused and
wrote the above comment.
I think this would not happen in practice, then
it might be OK without a special measure for this,
but I wasn't sure.
Best Regards,
Takamichi Osumi
RE: Skipping logical replication transactions on subscriber side
On Tuesday, March 15, 2022 3:13 PM Masahiko Sawada
wrote:
> I've attached an updated version patch.
A couple of minor comments on v14.
(1) apply_handle_commit_internal
+ if (is_skipping_changes())
+ {
+ stop_skipping_changes();
+
+ /*
+* Start a new transaction to clear the subskipxid, if not
started
+* yet. The transaction is committed below.
+*/
+ if (!IsTransactionState())
+ StartTransactionCommand();
+ }
+
I suppose we can move this condition check and stop_skipping_changes() call
to the inside of the block we enter when IsTransactionState() returns true.
As the comment of apply_handle_commit_internal() mentions,
it's the helper function for apply_handle_commit() and
apply_handle_stream_commit().
Then, I couldn't think that both callers don't open
a transaction before the call of apply_handle_commit_internal().
For applying spooled messages, we call begin_replication_step as well.
I can miss something, but timing when we receive COMMIT message
without opening a transaction, would be the case of empty transactions
where the subscription (and its subscription worker) is not interested.
If this is true, currently the patch's code includes
such cases within the range of is_skipping_changes() check.
(2) clear_subscription_skip_lsn's comments.
The comments for this function shouldn't touch
update of origin states, now that we don't update those.
+/*
+ * Clear subskiplsn of pg_subscription catalog with origin state updated.
+ *
This applies to other comments.
+ /*
+* Update the subskiplsn of the tuple to InvalidXLogRecPtr. If user has
+* already changed subskiplsn before clearing it we don't update the
+* catalog and don't advance the replication origin state.
...
+*We can reduce the possibility by
+* logging a replication origin WAL record to advance the origin LSN
+* instead but there is no way to advance the origin timestamp and it
+* doesn't seem to be worth doing anything about it since it's a very
rare
+* case.
+*/
Best Regards,
Takamichi Osumi
RE: Optionally automatically disable logical replication subscriptions on error
On Tuesday, March 15, 2022 8:04 AM Nathan Bossart wrote: > My compiler is worried that syncslotname may be used uninitialized in > start_table_sync(). The attached patch seems to silence this warning. Thank you for your reporting ! Your fix looks good to me. Best Regards, Takamichi Osumi
RE: Skipping logical replication transactions on subscriber side
On Friday, March 11, 2022 5:20 PM Masahiko Sawada wrote: > I've attached an updated version patch. This patch can be applied on top of > the > latest disable_on_error patch[1]. Hi, few extra comments on v13. (1) src/backend/replication/logical/worker.c With regard to clear_subscription_skip_lsn, There are cases that we conduct origin state update twice. For instance, the case we reset subskiplsn by executing an irrelevant non-empty transaction. The first update is conducted at apply_handle_commit_internal and the second one is at clear_subscription_skip_lsn. In the second change, we update replorigin_session_origin_lsn by smaller value(commit_lsn), compared to the first update(end_lsn). Were those intentional and OK ? (2) src/backend/replication/logical/worker.c + * Both origin_lsn and origin_timestamp are the remote transaction's end_lsn + * and commit timestamp, respectively. + */ +static void +stop_skipping_changes(XLogRecPtr origin_lsn, TimestampTz origin_ts) Typo. Should change 'origin_timestamp' to 'origin_ts', because the name of the argument is the latter. Also, here we handle not only commit but also prepare. You need to fix the comment "commit timestamp" as well. (3) src/backend/replication/logical/worker.c +/* + * Clear subskiplsn of pg_subscription catalog with origin state update. + * + * if with_warning is true, we raise a warning when clearing the subskipxid. It's better to insert this second sentence as the last sentence of the other comments. It should start with capital letter as well. Best Regards, Takamichi Osumi
RE: Optionally automatically disable logical replication subscriptions on error
On Monday, March 14, 2022 7:49 PM Amit Kapila wrote: > On Thu, Mar 10, 2022 at 12:04 PM Amit Kapila > wrote: > > > > On Wed, Mar 9, 2022 at 7:57 PM osumi.takami...@fujitsu.com > > wrote: > > > > > > Hi, attached v32 removed my additional code for > maybe_reread_subscription. > > > > > > > Thanks, the patch looks good to me. I have made minor edits in the > > attached. I am planning to commit this early next week (Monday) unless > > there are any other major comments. > > > > Pushed. Thank you so much ! Best Regards, Takamichi Osumi
RE: Skipping logical replication transactions on subscriber side
On Friday, March 11, 2022 5:20 PM Masahiko Sawada wrote:
> I've attached an updated version patch. This patch can be applied on top of
> the
> latest disable_on_error patch[1].
Hi, thank you for the patch. I'll share my review comments on v13.
(a) src/backend/commands/subscriptioncmds.c
@@ -84,6 +86,8 @@ typedef struct SubOpts
boolstreaming;
booltwophase;
booldisableonerr;
+ XLogRecPtr lsn;/* InvalidXLogRecPtr for
resetting purpose,
+* otherwise a
valid LSN */
I think this explanation is slightly odd and can be improved.
Strictly speaking, I feel a *valid* LSN is for retting transaction purpose
from the functional perspective. Also, the wording "resetting purpose"
is unclear by itself. I'll suggest below change.
From:
InvalidXLogRecPtr for resetting purpose, otherwise a valid LSN
To:
A valid LSN when we skip transaction, otherwise InvalidXLogRecPtr
(b) The code position of additional append in describeSubscriptions
+
+ /* Skip LSN is only supported in v15 and higher */
+ if (pset.sversion >= 15)
+ appendPQExpBuffer(,
+ ", subskiplsn AS
\"%s\"\n",
+ gettext_noop("Skip
LSN"));
I suggest to combine this code after subdisableonerr.
(c) parse_subscription_options
+ /* Parse the argument as LSN */
+ lsn =
DatumGetTransactionId(DirectFunctionCall1(pg_lsn_in,
Here, shouldn't we call DatumGetLSN, instead of DatumGetTransactionId ?
(d) parse_subscription_options
+ if (strcmp(lsn_str, "none") == 0)
+ {
+ /* Setting lsn = NONE is treated as resetting
LSN */
+ lsn = InvalidXLogRecPtr;
+ }
+
We should remove this pair of curly brackets that is for one sentence.
(e) src/backend/replication/logical/worker.c
+ * to skip applying the changes when starting to apply changes. The
subskiplsn is
+ * cleared after successfully skipping the transaction or applying non-empty
+ * transaction, where the later avoids the mistakenly specified subskiplsn from
+ * being left.
typo "the later" -> "the latter"
At the same time, I feel the last part of this sentence can be an independent
sentence.
From:
, where the later avoids the mistakenly specified subskiplsn from being left
To:
. The latter prevents the mistakenly specified subskiplsn from being left
* Note that my comments below are applied if we choose we don't merge
disable_on_error test with skip lsn tests.
(f) src/test/subscription/t/030_skip_xact.pl
+use Test::More tests => 4;
It's better to utilize the new style for the TAP test.
Then, probably we should introduce done_testing()
at the end of the test.
(g) src/test/subscription/t/030_skip_xact.pl
I think there's no need to create two types of subscriptions.
Just one subscription with two_phase = on and streaming = on
would be sufficient for the tests(normal commit, commit prepared,
stream commit cases). I think this point of view will reduce
the number of the table and the publication, which will
make the whole test simpler.
Best Regards,
Takamichi Osumi
RE: Skipping logical replication transactions on subscriber side
On Wednesday, March 2, 2022 12:01 AM Masahiko Sawada wrote: > I've attached an updated patch along with two patches for cfbot tests since > the > main patch (0003) depends on the other two patches. Both > 0001 and 0002 patches are the same ones I attached on another thread[2]. Hi, few comments on v12-0003-Add-ALTER-SUBSCRIPTION-.-SKIP-to-skip-the-transa.patch. (1) doc/src/sgml/ref/alter_subscription.sgml +SKIP ( skip_option = valuepg_subscription.subskiplsn) + is cleared. See for + the details of logical replication conflicts. + ... +lsn (pg_lsn) + + + Specifies the commit LSN of the remote transaction whose changes are to be skipped + by the logical replication worker. Skipping + individual subtransactions is not supported. Setting NONE + resets the LSN. I think we'll extend the SKIP option choices in the future besides the 'lsn' option. Then, one sentence "After logical replication successfully skips the transaction or commits non-empty transaction, the LSN .. is cleared" should be moved to the explanation for 'lsn' section, if we think this behavior to reset LSN is unique for 'lsn' option ? (2) doc/src/sgml/catalogs.sgml + + + subskiplsn pg_lsn + + + Commit LSN of the transaction whose changes are to be skipped, if a valid + LSN; otherwise 0/0. + + + We need to cover the PREPARE that keeps causing errors on the subscriber. This would apply to the entire patch (e.g. the rename of skip_xact_commit_lsn) (3) apply_handle_commit_internal comments /* * Helper function for apply_handle_commit and apply_handle_stream_commit. + * Return true if the transaction was committed, otherwise return false. */ If we want to make the new added line alinged with other functions in worker.c, we should insert one blank line before it ? (4) apply_worker_post_transaction I'm not sure if the current refactoring is good or not. For example, the current HEAD calls pgstat_report_stat(false) for a commit case if we are in a transaction in apply_handle_commit_internal. On the other hand, your refactoring calls pgstat_report_stat unconditionally for apply_handle_commit path. I'm not sure if there are many cases to call apply_handle_commit without opening a transaction, but is that acceptable ? Also, the name is a bit broad. How about making a function only for stopping and resetting LSN at this stage ? (5) comments for clear_subscription_skip_lsn How about changing the comment like below ? From: Clear subskiplsn of pg_subscription catalog To: Clear subskiplsn of pg_subscription catalog with origin state update Best Regards, Takamichi Osumi
RE: Optionally automatically disable logical replication subscriptions on error
On Wednesday, March 9, 2022 8:22 PM Amit Kapila wrote: > On Wed, Mar 9, 2022 at 2:21 PM Masahiko Sawada > wrote: > > > > On Wed, Mar 9, 2022 at 4:33 PM osumi.takami...@fujitsu.com > > wrote: > > > > > > On Tuesday, March 8, 2022 10:23 PM Amit Kapila > wrote: > > > > On Tue, Mar 8, 2022 at 1:37 PM osumi.takami...@fujitsu.com > > > > wrote: > > > > > > > > > > > > > > > 2. Is there a reason the patch doesn't allow workers to restart > > > > via > > > > maybe_reread_subscription() when this new option is changed, if > > > > so, then let's add a comment for the same? We currently seem to be > > > > restarting the worker on any change via Alter Subscription. If we > > > > decide to change it for this option as well then I think we need > > > > to accordingly update the current comment: "Exit if any parameter > > > > that affects the remote connection was changed." to something like > > > > "Exit if any parameter that affects the remote connection or a > subscription option was changed..." > > > I thought it's ok without the change at the beginning, but I was wrong. > > > To make this new option aligned with others, I should add one check > > > for this feature. Fixed. > > > > Why do we need to restart the apply worker when disable_on_error is > > changed? It doesn't affect the remote connection at all. I think it > > can be changed without restarting like synchronous_commit option. > > > > oh right, I thought that how will we update its value in MySubscription after > a > change but as we re-read the pg_subscription table for the current > subscription and update MySubscription, I feel we don't need to restart it. I > haven't tested it but it should work without a restart. Hi, attached v32 removed my additional code for maybe_reread_subscription. Also, I judged that we don't need to add a comment for this feature in this patch. It's because we can interpret this discussion from existing comments and codes. (1) "Reread subscription info if needed. Most changes will be exit." There are some cases we don't exit. (2) Like "Exit if any parameter that affects the remote connection was changed.", readers can understand no exit case matches the disable_on_error option change. Kindly review the v32. Best Regards, Takamichi Osumi v32-0001-Optionally-disable-subscriptions-on-error.patch Description: v32-0001-Optionally-disable-subscriptions-on-error.patch
RE: Optionally automatically disable logical replication subscriptions on error
On Tuesday, March 8, 2022 10:23 PM Amit Kapila wrote:
> On Tue, Mar 8, 2022 at 1:37 PM osumi.takami...@fujitsu.com
> wrote:
> >
> > Kindly have a look at v30.
> >
>
> Review comments:
Thank you for checking !
> ===
> 1.
> + ereport(LOG,
> + errmsg("logical replication subscription \"%s\" has been be disabled
> due to an error",
>
> Typo.
> /been be/been
Fixed.
> 2. Is there a reason the patch doesn't allow workers to restart via
> maybe_reread_subscription() when this new option is changed, if so, then let's
> add a comment for the same? We currently seem to be restarting the worker on
> any change via Alter Subscription. If we decide to change it for this option
> as
> well then I think we need to accordingly update the current comment: "Exit if
> any parameter that affects the remote connection was changed." to something
> like "Exit if any parameter that affects the remote connection or a
> subscription
> option was changed..."
I thought it's ok without the change at the beginning, but I was wrong.
To make this new option aligned with others, I should add one check
for this feature. Fixed.
> 3.
> if (fout->remoteVersion >= 15)
> - appendPQExpBufferStr(query, " s.subtwophasestate\n");
> + appendPQExpBufferStr(query, " s.subtwophasestate,\n");
> else
> appendPQExpBuffer(query,
> - " '%c' AS subtwophasestate\n",
> + " '%c' AS subtwophasestate,\n",
> LOGICALREP_TWOPHASE_STATE_DISABLED);
>
> + if (fout->remoteVersion >= 15)
> + appendPQExpBuffer(query, " s.subdisableonerr\n"); else
> + appendPQExpBuffer(query,
> + " false AS subdisableonerr\n");
>
> It is better to combine these parameters. I see there is a similar coding
> pattern
> for 14 but I think that is not required.
Fixed and combined them together.
> 4.
> +$node_subscriber->safe_psql('postgres', qq(ALTER SUBSCRIPTION sub
> +ENABLE));
> +
> +# Wait for the data to replicate.
> +$node_subscriber->poll_query_until(
> + 'postgres', qq(
> +SELECT COUNT(1) = 1 FROM pg_catalog.pg_subscription_rel sr WHERE
> +sr.srsubstate IN ('s', 'r') AND sr.srrelid = 'tbl'::regclass));
>
> See other scripts like t/015_stream.pl and wait for data replication in the
> same
> way. I think there are two things to change: (a) In the above query, we use
> NOT
> IN at other places (b) use $node_publisher->wait_for_catchup before this
> query.
Fixed.
The new patch is shared in [1].
[1] -
https://www.postgresql.org/message-id/TYCPR01MB8373824855A6C4D2178027A0ED0A9%40TYCPR01MB8373.jpnprd01.prod.outlook.com
Best Regards,
Takamichi Osumi
RE: Optionally automatically disable logical replication subscriptions on error
On Wednesday, March 9, 2022 9:58 AM Masahiko Sawada wrote: > On Tue, Mar 8, 2022 at 5:07 PM osumi.takami...@fujitsu.com > wrote: > > > > Kindly have a look at v30. > > Thank you for updating the patch. Here are some comments: Hi, thank you for your review ! > + /* > +* Allocate the origin name in long-lived context for error context > +* message. > +*/ > + ReplicationOriginNameForTablesync(MySubscription->oid, > + MyLogicalRepWorker->relid, > + originname, > + sizeof(originname)); > + apply_error_callback_arg.origin_name = > MemoryContextStrdup(ApplyContext, > + > + originname); > > I think it's better to set apply_error_callback_arg.origin_name in the caller > rather than in start_table_sync(). Apply workers set > apply_error_callback_arg.origin_name there and it's not necessarily necessary > to do that in this function. OK. I made this origin_name logic back to the level of ApplyWorkerMain. The new patch v31 is shared in [1]. [1] - https://www.postgresql.org/message-id/TYCPR01MB8373824855A6C4D2178027A0ED0A9%40TYCPR01MB8373.jpnprd01.prod.outlook.com Best Regardfs, Takamichi Osumi
RE: Optionally automatically disable logical replication subscriptions on error
On Wednesday, March 9, 2022 1:29 PM Amit Kapila wrote:
> On Tue, Mar 8, 2022 at 6:53 PM Amit Kapila wrote:
> >
> > On Tue, Mar 8, 2022 at 1:37 PM osumi.takami...@fujitsu.com
> > wrote:
> > >
> > > Kindly have a look at v30.
> > >
> >
> > Review comments:
> > ===
Thank you for reviewing !
> Few comments on test script:
> ===
> 1.
> +# This tests the uniqueness violation will cause the subscription # to
> +fail during initial synchronization and make it disabled.
>
> /This tests the/This tests that the
Fixed.
> 2.
> +$node_publisher->safe_psql('postgres',
> + qq(CREATE PUBLICATION pub FOR TABLE tbl));
> +$node_subscriber->safe_psql( 'postgres', qq( CREATE SUBSCRIPTION
> sub
> +CONNECTION '$publisher_connstr'
> +PUBLICATION pub WITH (disable_on_error = true)));
>
> Please check other test scripts like t/015_stream.pl or t/028_row_filter.pl
> and
> keep the indentation of these commands similar. It looks odd and inconsistent
> with other tests. Also, we can use double-quotes instead of qq so as to be
> consistent with other scripts. Please check other similar places and make
> them consistent with other test script files.
Fixed the inconsistent indentations within each commands.
Also, replace the qq with double-quotes (except for the is()'s
2nd argument, which is the aligned way to write the tests).
> 3.
> +# Initial synchronization failure causes the subscription # to be
> +disabled.
>
> Here and in other places in test scripts, the comment lines seem too short to
> me. Normally, we can keep it at the 80 char limit but this appears too short.
Fixed.
> 4.
> +# Delete the data from the subscriber and recreate the unique index.
> +$node_subscriber->safe_psql(
> + 'postgres', q(
> +DELETE FROM tbl;
> +CREATE UNIQUE INDEX tbl_unique ON tbl (i)));
>
> In other tests, we are executing single statements via safe_psql. I don't see
> a
> problem with this but also don't see a reason to deviate from the normal
> pattern.
Fixed.
At the same time, I fixed one comment
where I should write "subscriber", not "sub",
since in the entire test file, I express the subscriber
by using the former.
The new patch v31 is shared in [1].
[1] -
https://www.postgresql.org/message-id/TYCPR01MB8373824855A6C4D2178027A0ED0A9%40TYCPR01MB8373.jpnprd01.prod.outlook.com
Best Regards,
Takamichi Osumi
RE: Optionally automatically disable logical replication subscriptions on error
On Wednesday, March 9, 2022 3:02 PM Amit Kapila wrote: > On Wed, Mar 9, 2022 at 11:22 AM Masahiko Sawada > wrote: > > > > On Wed, Mar 9, 2022 at 12:37 PM Amit Kapila > wrote: > > > > > > On Wed, Mar 9, 2022 at 6:29 AM Masahiko Sawada > wrote: > > > > > > > > --- > > > > It might have already been discussed but the worker disables the > > > > subscription on an error but doesn't work for a fatal. Is that > > > > expected or should we handle that too? > > > > > > > > > > I am not too sure about handling FATALs with this feature because > > > this is mainly to aid in resolving conflicts due to various > > > constraints. It might be okay to retry in case of FATAL which is > > > possibly due to some system resource error. OTOH, if we see that it > > > will be good to disable for a FATAL error as well then I think we > > > can use PG_ENSURE_ERROR_CLEANUP construct. What do you think? > > > > I think that since FATAL raised by logical replication workers (e.g., > > terminated by DDL or out of memory etc?) is normally not a repeatable > > error, it's reasonable to retry in this case. > > > > Yeah, I think we can add a comment in the code for this so that future readers > know that this has been done deliberately. OK. I've added some comments in the codes. The v31 addressed other comments on hackers so far. (a) brush up the TAP test alignment (b) fix the place of apply_error_callback_arg.origin_name for table sync worker (c) modify maybe_reread_subscription to exit, when disable_on_error changes (d) improve getSubscriptions to combine some branches for v15 Kindly check the attached v31. Best Regards, Takamichi Osumi v31-0001-Optionally-disable-subscriptions-on-error.patch Description: v31-0001-Optionally-disable-subscriptions-on-error.patch
RE: Optionally automatically disable logical replication subscriptions on error
On Tuesday, March 8, 2022 1:07 PM Peter Smith wrote:
> Please find below some review comments for v29.
Thank you for your comments !
> ==
>
> 1. src/backend/replication/logical/worker.c - worker_post_error_processing
>
> +/*
> + * Abort and cleanup the current transaction, then do post-error processing.
> + * This function must be called in a PG_CATCH() block.
> + */
> +static void
> +worker_post_error_processing(void)
>
> The function comment and function name are too vague/generic. I guess this is
> a hang-over from Sawada-san's proposed patch, but now since this is only
> called when disabling the subscription so both the comment and the function
> name should say that's what it is doing...
>
> e.g. rename to DisableSubscriptionOnError() or something similar.
Fixed the comments and the function name in v30 shared in [1].
> ~~~
>
> 2. src/backend/replication/logical/worker.c - worker_post_error_processing
>
> + /* Notify the subscription has been disabled */ ereport(LOG,
> + errmsg("logical replication subscription \"%s\" has been be disabled
> due to an error",
> +MySubscription->name));
>
> proc_exit(0);
> }
>
> I know this is common code, but IMO it would be better to do the proc_exit(0);
> from the caller in the PG_CATCH. Then I think the code will be much easier to
> read the throw/exit logic, rather than now where it is just calling some
> function
> that never returns...
>
> Alternatively, if you want the code how it is, then the function name should
> give
> some hint that it is never going to return - e.g.
> DisableSubscriptionOnErrorAndExit)
I renamed it to DisableSubscriptionAndExit in the end
according to the discussion.
> ~~~
>
> 3. src/backend/replication/logical/worker.c - start_table_sync
>
> + {
> + /*
> + * Abort the current transaction so that we send the stats message
> + * in an idle state.
> + */
> + AbortOutOfAnyTransaction();
> +
> + /* Report the worker failed during table synchronization */
> + pgstat_report_subscription_error(MySubscription->oid, false);
> +
> + PG_RE_THROW();
> + }
>
> (This is a repeat of a previous comment from [1] comment #2)
>
> I felt the separation of those 2 statements and comments makes the code less
> clean than it could/should be. IMO they should be grouped together.
>
> SUGGESTED
>
> /*
> * Report the worker failed during table synchronization. Abort the
> * current transaction so that the stats message is sent in an idle
> * state.
> */
> AbortOutOfAnyTransaction();
> pgstat_report_subscription_error(MySubscription->oid, !am_tablesync_work
> er());
Fixed.
> ~~~
>
> 4. src/backend/replication/logical/worker.c - start_apply
>
> + {
> + /*
> + * Abort the current transaction so that we send the stats message
> + * in an idle state.
> + */
> + AbortOutOfAnyTransaction();
> +
> + /* Report the worker failed while applying changes */
> + pgstat_report_subscription_error(MySubscription->oid,
> + !am_tablesync_worker());
> +
> + PG_RE_THROW();
> + }
>
> (same as #3 but comment says "while applying changes")
>
> SUGGESTED
>
> /*
> * Report the worker failed while applying changing. Abort the current
> * transaction so that the stats message is sent in an idle state.
> */
> AbortOutOfAnyTransaction();
> pgstat_report_subscription_error(MySubscription->oid, !am_tablesync_work
> er());
Fixed. I choose the woring "while applying changes" which you mentioned first
and sounds more natural.
[1] -
https://www.postgresql.org/message-id/TYCPR01MB8373B74627C6BAF2F146D779ED099%40TYCPR01MB8373.jpnprd01.prod.outlook.com
Best Regards,
Takamichi Osumi
RE: Optionally automatically disable logical replication subscriptions on error
On Tuesday, March 8, 2022 2:52 PM Amit Kapila wrote:
> On Tue, Mar 8, 2022 at 9:37 AM Peter Smith wrote:
> >
> > Please find below some review comments for v29.
> >
> > ==
> >
> > 1. src/backend/replication/logical/worker.c -
> > worker_post_error_processing
> >
> > +/*
> > + * Abort and cleanup the current transaction, then do post-error
> > processing.
> > + * This function must be called in a PG_CATCH() block.
> > + */
> > +static void
> > +worker_post_error_processing(void)
> >
> > The function comment and function name are too vague/generic. I guess
> > this is a hang-over from Sawada-san's proposed patch, but now since
> > this is only called when disabling the subscription so both the
> > comment and the function name should say that's what it is doing...
> >
> > e.g. rename to DisableSubscriptionOnError() or something similar.
> >
> > ~~~
> >
> > 2. src/backend/replication/logical/worker.c -
> > worker_post_error_processing
> >
> > + /* Notify the subscription has been disabled */ ereport(LOG,
> > + errmsg("logical replication subscription \"%s\" has been be disabled
> > due to an error",
> > +MySubscription->name));
> >
> > proc_exit(0);
> > }
> >
> > I know this is common code, but IMO it would be better to do the
> > proc_exit(0); from the caller in the PG_CATCH. Then I think the code
> > will be much easier to read the throw/exit logic, rather than now
> > where it is just calling some function that never returns...
> >
> > Alternatively, if you want the code how it is, then the function name
> > should give some hint that it is never going to return - e.g.
> > DisableSubscriptionOnErrorAndExit)
> >
>
> I think we are already in error so maybe it is better to name it as
> DisableSubscriptionAndExit.
OK. Renamed.
> Few other comments:
> =
> 1.
> DisableSubscription()
> {
> ..
> +
> + LockSharedObject(SubscriptionRelationId, subid, 0,
> + AccessExclusiveLock);
>
> Why do we need AccessExclusiveLock here? The Alter/Drop Subscription
> takes AccessExclusiveLock, so AccessShareLock should be sufficient unless
> we have a reason to use AccessExclusiveLock lock. The other similar usages in
> this file (pg_subscription.c) also take AccessShareLock.
Fixed.
> 2. Shall we mention this feature in conflict handling docs [1]:
> Now:
> To skip the transaction, the subscription needs to be disabled temporarily by
> ALTER SUBSCRIPTION ... DISABLE first.
>
> After:
> To skip the transaction, the subscription needs to be disabled temporarily by
> ALTER SUBSCRIPTION ... DISABLE first or alternatively, the subscription can
> be used with the disable_on_error option.
>
> Feel free to use something on the above lines, if you agree.
Agreed. Fixed.
At the same time, the attached v30 has incorporated
some rebase results of recent commit(d3e8368)
so that start_table_sync allocates the origin names
in long-lived context. Accoring to this, I modified
some comments on this function.
I made some comments for sending stats in
start_table_sync and start_apply united and concise,
which were pointed out by Peter Smith in [1].
[1] -
https://www.postgresql.org/message-id/CAHut%2BPs3b8HjsVyo-Aygtnxbw1PiVOC9nvrN6dKxYtS4C8s%2Bgw%40mail.gmail.com
Kindly have a look at v30.
Best Regards,
Takamichi Osumi
v30-0001-Optionally-disable-subscriptions-on-error.patch
Description: v30-0001-Optionally-disable-subscriptions-on-error.patch
RE: Optionally automatically disable logical replication subscriptions on error
On Monday, March 7, 2022 5:45 PM Amit Kaila wrote:
> On Mon, Mar 7, 2022 at 4:55 AM Peter Smith
> wrote:
> >
> > On Fri, Mar 4, 2022 at 5:55 PM Masahiko Sawada
> wrote:
> > >
> > > ---
> > > +/*
> > > + * First, ensure that we log the error message so
> > > that it won't be
> > > + * lost if some other internal error occurs in the
> > > following code.
> > > + * Then, abort the current transaction and send the
> > > stats message of
> > > + * the table synchronization failure in an idle state.
> > > + */
> > > +HOLD_INTERRUPTS();
> > > +EmitErrorReport();
> > > +FlushErrorState();
> > > +AbortOutOfAnyTransaction();
> > > +RESUME_INTERRUPTS();
> > > +
> > > + pgstat_report_subscription_error(MySubscription->oid, false);
> > > +
> > > +if (MySubscription->disableonerr)
> > > +{
> > > +DisableSubscriptionOnError();
> > > +proc_exit(0);
> > > +}
> > > +
> > > +PG_RE_THROW();
> > >
> > > If the disableonerr is false, the same error is reported twice.
> > > Also, the code in PG_CATCH() in both start_apply() and
> > > start_table_sync() are almost the same. Can we create a common
> > > function to do post-error processing?
> > >
> > > The worker should exit with return code 1.
> > >
> > > I've attached a fixup patch for changes to worker.c for your
> > > reference. Feel free to adopt the changes.
> >
> > The way that common function is implemented required removal of the
> > existing PG_RE_THROW logic, which in turn was only possible using
> > special knowledge that this just happens to be the last try/catch
> > block for the apply worker.
> >
>
> I think we should re_throw the error in case we have not handled it by
> disabling
> the subscription (in which case we can exit with success code (0)).
Agreed. Fixed the patch so that it use re_throw.
Another point I changed from v28 is the order
to call AbortOutOfAnyTransaction and FlushErrorState,
which now is more aligned with other places.
Kindly check the attached v29.
Best Regards,
Takamichi Osumi
v29-0001-Optionally-disable-subscriptions-on-error.patch
Description: v29-0001-Optionally-disable-subscriptions-on-error.patch
RE: Optionally automatically disable logical replication subscriptions on error
On Monday, March 7, 2022 12:01 PM Shi, Yu/侍 雨 wrote: > On Wed, Mar 2, 2022 5:39 PM osumi.takami...@fujitsu.com > wrote: > > > > Attached an updated patch v26. > > > > Thanks for your patch. A comment on the document. Hi, thank you for checking my patch ! > @@ -7771,6 +7771,16 @@ SCRAM-SHA-256$iteration > count: > > > > + subdisableonerr bool > + > + > + If true, the subscription will be disabled if one of its workers > + detects an error > + > + > + > + > + > subconninfo text > > > > The document for "subdisableonerr" option is placed after "The following > parameters control what happens during subscription creation: ". I think it > should be placed after "The following parameters control the subscription's > replication behavior after it has been created: ", right? Addressed your comment for create_subscription.sgml (not for catalogs.sgml). Attached an updated patch v28. Best Regards, Takamichi Osumi v28-0001-Optionally-disable-subscriptions-on-error.patch Description: v28-0001-Optionally-disable-subscriptions-on-error.patch
RE: Optionally automatically disable logical replication subscriptions on error
On Friday, March 4, 2022 3:55 PM Masahiko Sawada wrote:
> Thank you for updating the patch.
>
> Here are some comments on v26 patch:
Thank you for your review !
> +/*
> + * Disable the current subscription.
> + */
> +static void
> +DisableSubscriptionOnError(void)
>
> This function now just updates the pg_subscription catalog so can we move it
> to pg_subscritpion.c while having this function accept the subscription OID to
> disable? If you agree, the function comment will also need to be updated.
Agreed. Fixed.
> ---
> +/*
> + * First, ensure that we log the error message so
> that it won't be
> + * lost if some other internal error occurs in the
> following code.
> + * Then, abort the current transaction and send the
> stats message of
> + * the table synchronization failure in an idle state.
> + */
> +HOLD_INTERRUPTS();
> +EmitErrorReport();
> +FlushErrorState();
> +AbortOutOfAnyTransaction();
> +RESUME_INTERRUPTS();
> +pgstat_report_subscription_error(MySubscription->oid,
> + false);
> +
> +if (MySubscription->disableonerr)
> +{
> +DisableSubscriptionOnError();
> +proc_exit(0);
> +}
> +
> +PG_RE_THROW();
>
> If the disableonerr is false, the same error is reported twice. Also, the
> code in
> PG_CATCH() in both start_apply() and start_table_sync() are almost the same.
> Can we create a common function to do post-error processing?
Yes. Also, calling PG_RE_THROW wasn't appropriate,
because in the previous v26, for the second error you mentioned,
the patch didn't call errstart when disable_on_error = false.
This was introduced by recent patch refactoring around this code and the rebase
of this patch, but has been fixed by your suggestion.
> The worker should exit with return code 1.
> I've attached a fixup patch for changes to worker.c for your reference. Feel
> free
> to adopt the changes.
Yes. I adopted almost all of your suggestion.
One thing I fixed was a comment that mentioned table sync
in worker_post_error_processing(), because start_apply()
also uses the function.
>
> ---
> +
> +# Confirm that we have finished the table sync.
> +is( $node_subscriber->safe_psql(
> +'postgres', qq(SELECT MAX(i), COUNT(*) FROM tbl)),
> +"1|3",
> +"subscription sub replicated data");
> +
>
> Can we store the result to a local variable to check? I think it's more
> consistent
> with other tap tests.
Agreed. Fixed.
Attached the v27. Kindly review the patch.
Best Regards,
Takamichi Osumi
v27-0001-Optionally-disable-subscriptions-on-error.patch
Description: v27-0001-Optionally-disable-subscriptions-on-error.patch
RE: Add the replication origin name and commit-LSN to logical replication worker errcontext
On Friday, March 4, 2022 2:23 PM Masahiko Sawada wrote: > I've attached updated patches. Hi, thank you for updating the patch. One comment on v4. In v4-0002, we introduce 'commit_lsn' in the ApplyErrorCallbackArg. This member is set for prepare, rollback prepared and stream_abort as well. The new log message format is useful when we have a prepare transaction that keeps failing on the subscriber and want to know the target transaction for the pg_replication_origin_advance(), right ? If this is true, I wasn't sure if the name 'commit_lsn' is the most accurate name for this variable. Should we adjust the name a bit ? Even when we decide to continue to use 'commit_lsn', it might be better to add some comments near the definition, I feel. Best Regards, Takamichi Osumi
RE: Add the replication origin name and commit-LSN to logical replication worker errcontext
On Friday, March 4, 2022 10:09 AM Masahiko Sawada wrote:
> On Thu, Mar 3, 2022 at 10:02 PM Masahiko Sawada
> wrote:
> >
> >
> > I'm updating the patches and will submit them.
>
> Attached updated version patches.
Thank you for sharing the patch v3.
Few minor comments.
(1) v03-0001, apply_error_callback function
- /* append transaction information */
- if (TransactionIdIsNormal(errarg->remote_xid))
+ if (errarg->rel == NULL)
{
- appendStringInfo(, _(" in transaction %u"),
errarg->remote_xid);
Should write !errarg->rel ?
(2) v03-0002, doc/src/sgml/logical-replication.sgml
+ transaction that conflicts with the existing data. When a conflict produces
+ an error, it is shown in the subscriber's server logs as follows:
+
+ERROR: duplicate key value violates unique constraint "test_pkey"
+DETAIL: Key (c)=(1) already exists.
+CONTEXT: processing remote data during "INSERT" for replication target
relation "public.test" in transaction 725 committed at LSN 0/14BFA88
+
We should update the CONTEXT message by using the v3-0001.
(3) v03-0002, doc/src/sgml/logical-replication.sgml
+ The LSN of the transaction that contains the change violating the
constraint and
+ the replication origin name can be found from those outputs (LSN 0/14C0378
and
+ replication origin pg_16395 in the above case). To skip
the
+ transaction, the subscription needs to be disabled temporarily by
+ ALTER SUBSCRIPTION ... DISABLE first. Then, the
transaction
+ can be skipped by calling the
The LSN(0/14C0378) is not same as the one in the above error context.
It's recommended to check LSNs directly written in the documentation.
(4) one confirmation
We don't have a TAP test of pg_replication_origin_advance()
for v3, that utilizes this new log in a logical replication setup.
This is because existing tests for this function (in test_decoding) is only for
permission check
and argument validation, and we're just changing error message itself.
Is this correct ?
Best Regards,
Takamichi Osumi
RE: Failed transaction statistics to measure the logical replication progress
On Wednesday, March 2, 2022 5:29 PM Shi, Yu/侍 雨 wrote: > A comments on the v26 patch. Thank you for checking the patch ! > > The following document about pg_stat_subscription_stats view only says that > "showing statistics about errors", should we add something about transactions > here? > > > > pg_stat_subscription_stats >pg_stat_subscription_stats > One row per subscription, showing statistics about errors. > See > pg_stat_subscription_stats for > details. > > > > > I noticed that the v24 patch has some changes about the description of this > view. Maybe we can modify to "showing statistics about errors and > transactions". You are right. Fixed. New patch v27 that incorporated your comments is shared in [1]. Kindly have a look at it. [1] - https://www.postgresql.org/message-id/TYCPR01MB837345FB72E3CE9C827AF42CED049%40TYCPR01MB8373.jpnprd01.prod.outlook.com Best Regards, Takamichi Osumi
RE: Failed transaction statistics to measure the logical replication progress
On Wednesday, March 2, 2022 2:18 PM Masahiko Sawada
wrote:
> On Wed, Mar 2, 2022 at 10:21 AM osumi.takami...@fujitsu.com
> wrote:
> > Also, I quickly checked other similar views(pg_stat_slru,
> > pg_stat_wal_receiver) commit logs, especially when they introduce columns.
> > But, I couldn't find column name validations.
> > So, I feel this is aligned.
> >
>
> I've looked at v26 patch and here are some random comments:
Hi, thank you for reviewing !
> +/* determine the subscription entry */
> +Oidm_subid;
> +
> +PgStat_Counter apply_commit_count;
> +PgStat_Counter apply_rollback_count;
>
> I think it's better to add the prefix "m_" to apply_commit/rollback_count for
> consistency.
Fixed.
> ---
> +/*
> + * Increment the counter of commit for subscription statistics.
> + */
> +static void
> +subscription_stats_incr_commit(void)
> +{
> +Assert(OidIsValid(subStats.subid));
> +
> +subStats.apply_commit_count++;
> +}
> +
>
> I think we don't need the Assert() here since it should not be a problem even
> if
> subStats.subid is InvalidOid at least in this function.
>
> If we remove it, we can remove both subscription_stats_incr_commit() and
> +subscription_stats_incr_rollback() as well.
Removed the Assert() from both functions.
> ---
> +void
> +pgstat_report_subscription_xact(bool force) {
> +static TimestampTz last_report = 0;
> +PgStat_MsgSubscriptionXact msg;
> +
> +/* Bailout early if nothing to do */
> +if (!OidIsValid(subStats.subid) ||
> +(subStats.apply_commit_count == 0 &&
> subStats.apply_rollback_count == 0))
> +return;
> +
>
> +LogicalRepSubscriptionStats subStats =
> +{
> +.subid = InvalidOid,
> +.apply_commit_count = 0,
> +.apply_rollback_count = 0,
> +};
>
> Do we need subStats.subid? I think we can pass MySubscription->oid (or
> MyLogicalRepWorker->subid) to pgstat_report_subscription_xact() along
> with the pointer of the statistics (subStats). That way, we don't need to
> expose
> subStats.
Removed the subStats.subid. Also, now I pass the oid to the
pgstat_report_subscription_xact with the pointer of the statistics.
> Also, I think it's better to add "Xact" or something to the struct name. For
> example, SubscriptionXactStats.
Renamed.
> ---
> +
> +typedef struct LogicalRepSubscriptionStats {
> +Oidsubid;
> +
> +int64 apply_commit_count;
> +int64 apply_rollback_count;
> +} LogicalRepSubscriptionStats;
>
> We need a description for this struct.
>
> Probably it is better to declare it in logicalworker.h instead so that
> pgstat.c
> includes it instead of worker_internal.h? worker_internal.h is the header file
> shared by logical replication workers such as apply worker, tablesync worker,
> and launcher. So it might not be advisable to include it in pgstat.c.
Changed the definition place to logicalworker.h
and added some explanations for it.
Attached the updated v27.
Best Regards,
Takamichi Osumi
v27-0001-Extend-pg_stat_subscription_stats-to-include-gen.patch
Description: v27-0001-Extend-pg_stat_subscription_stats-to-include-gen.patch
RE: Optionally automatically disable logical replication subscriptions on error
On Wednesday, March 2, 2022 12:47 PM Masahiko Sawada
wrote:
> After more thoughts, should we do both AbortOutOfAnyTransaction() and error
> message handling while holding interrupts? That is,
>
> HOLD_INTERRUPTS();
> EmitErrorReport();
> FlushErrorState();
> AbortOutOfAny Transaction();
> RESUME_INTERRUPTS();
> pgstat_report_subscription_error(MySubscription->oid, !am_tablesync_work
> er());
>
> I think it's better that we do clean up first and then do other works such as
> sending the message to the stats collector and updating the catalog.
I agree. Fixed. Along with this change, I corrected the header comment of
DisableSubscriptionOnError, too.
> Here are some comments on v24 patch:
>
> +/* Look up our subscription in the catalogs */
> +tup = SearchSysCacheCopy2(SUBSCRIPTIONNAME, MyDatabaseId,
> +
> CStringGetDatum(MySubscription->name));
>
> s/catalogs/catalog/
>
> Why don't we use SUBSCRIPTIONOID with MySubscription->oid?
Changed.
> ---
> +if (!HeapTupleIsValid(tup))
> +ereport(ERROR,
> +errcode(ERRCODE_UNDEFINED_OBJECT),
> +errmsg("subscription \"%s\" does not
> exist",
> + MySubscription->name));
>
> I think we should use elog() here rather than ereport() since it's a
> should-not-happen error.
Fixed
> ---
> +/* Notify the subscription will be no longer valid */
>
> I'd suggest rephrasing it to like "Notify the subscription will be disabled".
> (the
> subscription is still valid actually, but just disabled).
Fixed. Also, I've made this sentence past one, because of the code place
change below.
> ---
> +/* Notify the subscription will be no longer valid */
> +ereport(LOG,
> +errmsg("logical replication subscription
> \"%s\" will be disabled due to an error",
> + MySubscription->name));
> +
>
> I think we can report the log at the end of this function rather than during
> the
> transaction.
Fixed. In this case, I needed to adjust the comment to indicate the processing
to disable the sub has *completed* as well.
> ---
> +my $cmd = qq(
> +CREATE TABLE tbl (i INT);
> +ALTER TABLE tbl REPLICA IDENTITY FULL;
> +CREATE INDEX tbl_idx ON tbl(i));
>
> I think we don't need to set REPLICA IDENTITY FULL to this table since there
> is
> notupdate/delete.
>
> Do we need tbl_idx?
Removed both the replica identity and tbl_idx;
> ---
> +$cmd = qq(
> +SELECT COUNT(1) = 1 FROM pg_catalog.pg_subscription_rel sr WHERE
> +sr.srsubstate IN ('s', 'r'));
> +$node_subscriber->poll_query_until('postgres', $cmd);
>
> It seems better to add a condition of srrelid just in case.
Makes sense. Fixed.
> ---
> +$cmd = qq(
> +SELECT count(1) = 1 FROM pg_catalog.pg_subscription s WHERE
> s.subname =
> +'sub' AND s.subenabled IS FALSE);
> +$node_subscriber->poll_query_until('postgres', $cmd)
> + or die "Timed out while waiting for subscriber to be disabled";
>
> I think that it's more natural to directly check the subscription's
> subenabled.
> For example:
>
> SELECT subenabled = false FROM pg_subscription WHERE subname = 'sub';
Fixed. I modified another code similar to this for tablesync error as well.
> ---
> +$cmd = q(ALTER SUBSCRIPTION sub ENABLE);
> +$node_subscriber->safe_psql('postgres', $cmd); $cmd = q(SELECT
> COUNT(1)
> += 3 FROM tbl WHERE i = 3);
> +$node_subscriber->poll_query_until('postgres', $cmd)
> + or die "Timed out while waiting for applying";
>
> I think it's better to wait for the subscriber to catch up and check the query
> result instead of using poll_query_until() so that we can check the query
> result
> in case where the test fails.
I modified the code to wait for the subscriber and deleted poll_query_until.
Also, when I consider the test failure for this test as you mentioned,
expecting and checking the number of return value that equals 3
would be better. So, I expressed this point in my test as well,
according to your advice.
> ---
> +$cmd = qq(DROP INDEX tbl_unique);
> +$node_subscriber->safe_psql('postgres', $cmd);
>
> In the newly added tap tests, all queries are consistently assigned to $cmd
> and
> executed even when the query is used only once. It seems a different style
> from
> the one in other tap tests. Is there any reason why we do this style for all
> queries
> in the tap tests?
Fixed. I removed the 'cmd' variable itself.
Attached an updated patch v26.
Best Regards,
Takamichi Osumi
v26-0001-Optionally-disable-subscriptions-on-error.patch
Description: v26-0001-Optionally-disable-subscriptions-on-error.patch
RE: Optionally automatically disable logical replication subscriptions on error
On Wednesday, March 2, 2022 9:34 AM Peter Smith wrote:
> Please see below my review comments for v24.
Thank you for checking my patch !
> ==
>
> 1. src/backend/replication/logical/worker.c - start_table_sync
>
> + /* Report the worker failed during table synchronization */
> + pgstat_report_subscription_error(MySubscription->oid,
> + !am_tablesync_worker());
>
> (This review comment is just FYI in case you did not do this deliberately)
>
> FYI, you didn't really need to call am_tablesync_worker() here because it is
> already asserted for the sync phase that it MUST be a tablesync worker.
>
> OTOH, IMO it documents the purpose of the parm so if it was deliberate then
> that is OK too.
Fixed.
> ~~~
>
> 2. src/backend/replication/logical/worker.c - start_table_sync
>
> + PG_CATCH();
> + {
> + /*
> + * Abort the current transaction so that we send the stats message in
> + * an idle state.
> + */
> + AbortOutOfAnyTransaction();
> +
> + /* Report the worker failed during table synchronization */
> + pgstat_report_subscription_error(MySubscription->oid,
> + !am_tablesync_worker());
> +
>
> [Maybe you will say that this review comment is unrelated to disable_on_err,
> but since this is a totally new/refactored function then I think maybe there
> is no
> problem to make this change at the same time. Anyway there is no function
> change, it is just rearranging some comments.]
>
> I felt the separation of those 2 statements and comments makes that code less
> clean than it could/should be. IMO they should be grouped together.
>
> SUGGESTED
> /*
> * Report the worker failed during table synchronization. Abort the
> * current transaction so that the stats message is sent in an idle
> * state.
> */
> AbortOutOfAnyTransaction();
> pgstat_report_subscription_error(MySubscription->oid, !am_tablesync_work
> er());
I think this is OK. Thank you for suggestion. Fixed.
> ~~~
>
> 3. src/backend/replication/logical/worker.c - start_apply
>
> + /*
> + * Abort the current transaction so that we send the stats message in
> + * an idle state.
> + */
> + AbortOutOfAnyTransaction();
> +
> + /* Report the worker failed during the application of the change */
> + pgstat_report_subscription_error(MySubscription->oid,
> + !am_tablesync_worker());
>
> Same comment as #2 above, but this code fragment is in start_apply function.
Fixed.
> ~~~
>
> 4. src/test/subscription/t/029_disable_on_error.pl - comment
>
> +# Drop the unique index on the sub and re-enabled the subscription.
> +# Then, confirm that we have finished the apply.
>
> SUGGESTED (tweak the comment wording)
> # Drop the unique index on the sub and re-enable the subscription.
> # Then, confirm that the previously failing insert was applied OK.
Fixed.
Best Regards,
Takamichi Osumi
v25-0001-Optionally-disable-subscriptions-on-error.patch
Description: v25-0001-Optionally-disable-subscriptions-on-error.patch
RE: logical replication restrictions
On Wednesday, March 2, 2022 8:54 AM Euler Taveira wrote: > On Tue, Mar 1, 2022, at 3:27 AM, osumi.takami...@fujitsu.com > <mailto:osumi.takami...@fujitsu.com> wrote: > > > $ git am v1-0001-Time-delayed-logical-replication-subscriber.patch > > > I generally use -3 to fall back on 3-way merge. Doesn't it work for you? It did. Excuse me for making noises. Best Regards, Takamichi Osumi
RE: Failed transaction statistics to measure the logical replication progress
On Tuesday, March 1, 2022 4:12 PM Peter Smith wrote: > Please see below my review comments for v25. > > == > > 1. Commit message > > Introduce cumulative columns of transactions of logical replication subscriber > to the pg_stat_subscription_stats view. > > "cumulative columns of transactions" sounds a bit strange to me. > > SUGGESTED > Introduce 2 new subscription statistics columns (apply_commit_count, and > apply_rollback_count) to the pg_stat_subscription_stats view for counting > cumulative transaction commits/rollbacks. Fixed. > ~~~ > > 2. doc/src/sgml/monitoring.sgml - bug > > The new SGML s have been added in the wrong place! > > I don't think this renders like you expect it does. Please regenerate the > help to > see for yourself. Fixed. > ~~~ > > 3. doc/src/sgml/monitoring.sgml - wording > > + > + Number of transactions rollbacked in this subscription. Both > + ROLLBACK of transaction streamed as > in-progress > + transaction and ROLLBACK PREPARED > increment this > + counter. > + > > BEFORE > Number of transactions rollbacked in this subscription. > > SUGGESTED > Number of transaction rollbacks in this subscription. Fixed. > ~~~ > > 4. doc/src/sgml/monitoring.sgml - wording > > + > + Number of transactions rollbacked in this subscription. Both > + ROLLBACK of transaction streamed as > in-progress > + transaction and ROLLBACK PREPARED > increment this > + counter. > + > > Trying to distinguish between the ROLLBACK of a transaction and of a > streamed in-progress transaction seems to have made this description too > complicated. I don't think the user even cares/knows about this > (in-progress) distinction. So, I think this should just be written more simply > (like the COMMIT part was) > > BEFORE > Both ROLLBACK of transaction streamed as > in-progress transaction and ROLLBACK > PREPARED increment this counter. > > SUGGESTED > Both ROLLBACK and ROLLBACK > PREPARED increment this counter. Fixed. > ~~~ > > 5. Question - column names. > > Just curious why the columns are called "apply_commit_count" and > "apply_rollback_count"? Specifically, what extra meaning do those names have > versus just calling them "commit_count" and "rollback_count"? I think there's possibility that we'll have counters for tablesync commit for example. So, the name prefix avoids the overlap between the possible names. > ~~~ > > 6. src/backend/postmaster/pgstat.c - pgstat_report_subscription_xact > > @@ -3421,6 +3425,60 @@ pgstat_send_slru(void) } > > /* -- > + * pgstat_report_subscription_xact() - > + * > + * Send a subscription transaction stats to the collector. > + * The statistics are cleared upon sending. > + * > + * 'force' is true only when the subscription worker process exits. > + * -- > + */ > +void > +pgstat_report_subscription_xact(bool force) > > 6a. > I think this comment should be worded more like the other > pgstat_report_subscption_XXX comments > > BEFORE > Send a subscription transaction stats to the collector. > > SUGGESTED > Tell the collector about subscriptions transaction stats. Fixed. > 6b. > + * 'force' is true only when the subscription worker process exits. > > I thought this comment should just describe what the 'force' param actually > does in this function; not the scenario about who calls it... Fixed. > ~~~ > > 7. src/backend/postmaster/pgstat.c - pgstat_report_subscription_xact > > I think the entire function maybe should be relocated to be nearby the other > pgstat_report_subscription_XXX functions in the source. I placed the pgstat_report_subscription_xact below pgstat_report_subscription_drop. Meanwhile, pgstat_recv_subscription_xact, another new function in pgstat.c, is already placed below pgstat_recv_subscription_error, so I kept it as it is. > ~~~ > > 8. src/backend/postmaster/pgstat.c - pgstat_report_subscription_xact > > + /* > + * This function can be called even if nothing at all has happened. In > + * this case, there's no need to go forward. > + */ > > Too much information. Clearly, it is possible for this function to be called > for this > case otherwise this code would not exist in the first place :) IMO the comment > can be much simpler but still say all it needs to. > > BEFORE > This function can be called even if nothing at all has happened. In this case, > there's no need to go forward. > SUGGESTED > Bailout early if nothing to do. Fixed. > ~~~ > > 9. src/backend/postmaster/pgstat.c - pgstat_report_subscription_xact > > + if (subStats.subid == InvalidOid || > + (subStats.apply_commit_count == 0 && subStats.apply_rollback_count == > + 0)) return; > > Maybe using !OisIsValid(subStats.subid) is better? Fixed. > ~~~ > > 10. src/backend/postmaster/pgstat.c - pgstat_report_subscription_xact > > + /* > + * Don't send a message unless it's been at least PGSTAT_STAT_INTERVAL > + * msec since we last sent one to avoid
RE: logical replication restrictions
On Tuesday, March 1, 2022 9:19 AM Euler Taveira wrote: > Long time, no patch. Here it is. I will provide documentation in the next > > version. I would appreciate some feedback. Hi, thank you for posting the patch ! $ git am v1-0001-Time-delayed-logical-replication-subscriber.patch Applying: Time-delayed logical replication subscriber error: patch failed: src/backend/catalog/system_views.sql:1261 error: src/backend/catalog/system_views.sql: patch does not apply FYI, by one recent commit(7a85073), the HEAD redesigned pg_stat_subscription_workers. Thus, the blow change can't be applied. Could you please rebase v1 ? diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql index 3cb69b1f87..1cc0d86f2e 100644 --- a/src/backend/catalog/system_views.sql +++ b/src/backend/catalog/system_views.sql @@ -1261,7 +1261,8 @@ REVOKE ALL ON pg_replication_origin_status FROM public; -- All columns of pg_subscription except subconninfo are publicly readable. REVOKE ALL ON pg_subscription FROM public; GRANT SELECT (oid, subdbid, subname, subowner, subenabled, subbinary, - substream, subtwophasestate, subslotname, subsynccommit, subpublications) + substream, subtwophasestate, subslotname, subsynccommit, + subapplydelay, subpublications) ON pg_subscription TO public; CREATE VIEW pg_stat_subscription_workers AS Best Regards, Takamichi Osumi
RE: Optionally automatically disable logical replication subscriptions on error
On Tuesday, March 1, 2022 9:49 AM Peter Smith wrote:
> Please see below my review comments for v22.
>
> ==
>
> 1. Commit message
>
> "table sync worker" -> "tablesync worker"
Fixed.
> ~~~
>
> 2. doc/src/sgml/catalogs.sgml
>
> +
> + If true, the subscription will be disabled when subscription
> + workers detect any errors
> +
>
> It felt a bit strange to say "subscription" 2x in the sentence, but I am not
> sure
> how to improve it. Maybe like below?
>
> BEFORE
> If true, the subscription will be disabled when subscription workers detect
> any
> errors
>
> SUGGESTED
> If true, the subscription will be disabled if one of its workers detects an
> error
Fixed.
> ~~~
>
> 3. src/backend/replication/logical/worker.c - DisableSubscriptionOnError
>
> @@ -2802,6 +2803,69 @@ LogicalRepApplyLoop(XLogRecPtr
> last_received) }
>
> /*
> + * Disable the current subscription, after error recovery processing.
> + */
> +static void
> +DisableSubscriptionOnError(void)
>
> I thought the "after error recovery processing" part was a bit generic and
> did not
> really say what it was doing.
>
> BEFORE
> Disable the current subscription, after error recovery processing.
> SUGGESTED
> Disable the current subscription, after logging the error that caused this
> function to be called.
Fixed.
> ~~~
>
> 4. src/backend/replication/logical/worker.c - start_apply
>
> + if (MySubscription->disableonerr)
> + {
> + DisableSubscriptionOnError();
> + return;
> + }
> +
> + MemoryContextSwitchTo(ecxt);
> + PG_RE_THROW();
> + }
> + PG_END_TRY();
>
> The current code looks correct, but I felt it is a bit tricky to easily see
> the
> execution path after the return.
>
> Since it will effectively just exit anyhow I think it will be simpler just to
> do that
> explicitly right here instead of the 'return'. This will also make the code
> consistent with the same 'disableonerr' logic of the start_start_sync.
>
> SUGGESTION
> if (MySubscription->disableonerr)
> {
> DisableSubscriptionOnError();
> proc_exit(0);
> }
Fixed.
> ~~~
>
> 5. src/bin/pg_dump/pg_dump.c
>
> @@ -4463,6 +4473,9 @@ dumpSubscription(Archive *fout, const
> SubscriptionInfo *subinfo)
> if (strcmp(subinfo->subtwophasestate, two_phase_disabled) != 0)
> appendPQExpBufferStr(query, ", two_phase = on");
>
> + if (strcmp(subinfo->subdisableonerr, "f") != 0)
> + appendPQExpBufferStr(query, ", disable_on_error = true");
> +
>
> Although the code is correct, I think it would be more natural to set this
> option
> as true when the user wants it true. e.g. check for "t"
> same as 'subbinary' is doing. This way, even if there was some
> unknown/corrupted value the code would do nothing, which is the behaviour
> you want...
>
> SUGGESTION
> if (strcmp(subinfo->subdisableonerr, "t") == 0)
Fixed.
> ~~~
>
> 6. src/include/catalog/pg_subscription.h
>
> @@ -67,6 +67,9 @@ CATALOG(pg_subscription,6100,SubscriptionRelationId)
> BKI_SHARED_RELATION BKI_ROW
>
> char subtwophasestate; /* Stream two-phase transactions */
>
> + bool subdisableonerr; /* True if occurrence of apply errors
> + * should disable the subscription */
>
> The comment seems not quite right because it's not just about apply errors.
> E.g.
> I think any error in tablesync will cause disablement too.
>
> BEFORE
> True if occurrence of apply errors should disable the subscription SUGGESTED
> True if a worker error should cause the subscription to be disabled
Fixed.
> ~~~
>
> 7. src/test/regress/sql/subscription.sql - whitespace
>
> +-- now it works
> +CREATE SUBSCRIPTION regress_testsub CONNECTION
> 'dbname=regress_doesnotexist' PUBLICATION testpub WITH (connect = false,
> disable_on_error = false);
> +
> +\dRs+
> +
> +ALTER SUBSCRIPTION regress_testsub SET (disable_on_error = true);
> +
> +\dRs+
> +ALTER SUBSCRIPTION regress_testsub SET (slot_name = NONE); DROP
> +SUBSCRIPTION regress_testsub;
> +
>
> I think should be a blank line after that last \dRs+ just like the other one,
> because it belongs logically with the code above it, not with the ALTER
> slot_name.
Fixed.
> ~~~
>
> 8. src/test/subscription/t/028_disable_on_error.pl - filename
>
> The 028 number needs to be bumped because there is already a TAP test
> called 028 now
This is already done in v22, so I've skipped this.
> ~~~
>
> 9. src/test/subscription/t/028_disable_on_error.pl - missing test
>
> There was no test case for the last combination where the user correct the
> apply worker problem: E.g. After a previous error/disable of the subscriber,
> remove the index, publish the inserts again, and check they get applied
> properly.
Fixed.
Attached the updated version v24.
Best Regards,
Takamichi Osumi
v24-0001-Optionally-disable-subscriptions-on-error.patch
Description: v24-0001-Optionally-disable-subscriptions-on-error.patch
RE: Optionally automatically disable logical replication subscriptions on error
On Friday, February 25, 2022 9:45 PM osumi.takami...@fujitsu.com wrote: > Kindly have a look at attached the v22. > It has incorporated other improvements of TAP test, refinement of the > DisableSubscriptionOnError function and so on. The recent commit(7a85073) has changed the subscription workers error handling. So, I rebased my disable_on_error patch first for anyone who are interested in the review. I'll incorporate incoming comments for v22 in my next version. Best Regards, Takamichi Osumi v23-0001-Optionally-disable-subscriptions-on-error.patch Description: v23-0001-Optionally-disable-subscriptions-on-error.patch
RE: Failed transaction statistics to measure the logical replication progress
On Friday, February 25, 2022 7:58 AM osumi.takami...@fujitsu.com wrote: > Kindly have a look at v24. Hi. The recent commit(7a85073) has redesigned the view pg_stat_subscription_workers and now we have pg_stat_subscription_stats. Therefore, I rebased my patch so that my statistics patch can be applied on top of the HEAD. In the process of this rebase, I had to drop one column that stored error count for unique errors(which was incremented after confirming the error is not the same as previous one), because the commit tentatively removes the same error check mechanism. Therefore, this patch has apply_commit_count, apply_rollback_count only. I slightly changed minor changes as well so that those can become more aligned. Kindly please have a look at the patch. Best Regards, Takamichi Osumi v25-0001-Extend-pg_stat_subscription_stats-to-include-gen.patch Description: v25-0001-Extend-pg_stat_subscription_stats-to-include-gen.patch
RE: Design of pg_stat_subscription_workers vs pgstats
On Monday, February 28, 2022 12:57 PM Amit Kapila > On Mon, Feb 28, 2022 at 8:49 AM osumi.takami...@fujitsu.com > wrote: > > > > On Monday, February 28, 2022 11:34 AM Amit Kapila > wrote: > > > On Sat, Feb 26, 2022 at 1:35 PM osumi.takami...@fujitsu.com > > > wrote: > > > > > > > > On Saturday, February 26, 2022 11:51 AM Amit Kapila > > > wrote: > > > > > I have reviewed the latest version and made a few changes along > > > > > with fixing some of the pending comments by Peter Smith. The > > > > > changes are as > > > > > follows: (a) Removed m_databaseid in PgStat_MsgSubscriptionError > > > > > as that is not required now; (b) changed the struct name > > > > > PgStat_MsgSubscriptionPurge to PgStat_MsgSubscriptionDrop to > > > > > make it similar to DropDb; (c) changed the view name to > > > > > pg_stat_subscription_stats, we can reconsider it in future if > > > > > there is a consensus on some other name, accordingly changed the > > > > > reset function name to pg_stat_reset_subscription_stats; (d) > > > > > moved some of the newly added subscription stats functions > > > > > adjacent to slots to main the consistency in code; (e) changed > > > > > comments at few places; > > > > > (f) added LATERAL back to system_views query as we refer > > > pg_subscription's oid in the function call, previously that was not clear. > > > > > > > > > > Do let me know what you think of the attached? > > > > Hi, thank you for updating the patch ! > > > > I have a couple of comments on v4. > > > > > > > > (1) > > > > > > > > I'm not sure if I'm correct, but I'd say the sync_error_count can > > > > come next to the subname as the order of columns. > > > > I felt there's case that the column order is somewhat related to > > > > the time/processing order (I imagined pg_stat_replication's LSN > > > > related columns). > > > > If this was right, table sync related column could be the first > > > > column as a counter within this patch. > > > > > > > > > > I am not sure if there is such a correlation but even if it is there > > > it doesn't seem to fit here completely as sync errors can happen > > > after apply errors in multiple ways like via Alter Subscription ... > > > Refresh ... > > > > > > So, I don't see the need to change the order here. What do you or others > think? > > In the alter subscription case, any errors after the table sync would > > increment apply_error_count. > > > > Sure, but the point I was trying to explain was that there is no certainty in > the > order of these errors. I got it. Thank you so much for your explanation. I don't have other new comments on this patch. It looks good to me as well. Best Regards, Takamichi Osumi
RE: Design of pg_stat_subscription_workers vs pgstats
On Monday, February 28, 2022 11:34 AM Amit Kapila wrote: > On Sat, Feb 26, 2022 at 1:35 PM osumi.takami...@fujitsu.com > wrote: > > > > On Saturday, February 26, 2022 11:51 AM Amit Kapila > wrote: > > > I have reviewed the latest version and made a few changes along with > > > fixing some of the pending comments by Peter Smith. The changes are > > > as > > > follows: (a) Removed m_databaseid in PgStat_MsgSubscriptionError as > > > that is not required now; (b) changed the struct name > > > PgStat_MsgSubscriptionPurge to PgStat_MsgSubscriptionDrop to make it > > > similar to DropDb; (c) changed the view name to > > > pg_stat_subscription_stats, we can reconsider it in future if there > > > is a consensus on some other name, accordingly changed the reset > > > function name to pg_stat_reset_subscription_stats; (d) moved some of > > > the newly added subscription stats functions adjacent to slots to > > > main the consistency in code; (e) changed comments at few places; > > > (f) added LATERAL back to system_views query as we refer > pg_subscription's oid in the function call, previously that was not clear. > > > > > > Do let me know what you think of the attached? > > Hi, thank you for updating the patch ! > > I have a couple of comments on v4. > > > > (1) > > > > I'm not sure if I'm correct, but I'd say the sync_error_count can come > > next to the subname as the order of columns. > > I felt there's case that the column order is somewhat related to the > > time/processing order (I imagined pg_stat_replication's LSN related > > columns). > > If this was right, table sync related column could be the first column > > as a counter within this patch. > > > > I am not sure if there is such a correlation but even if it is there it > doesn't seem > to fit here completely as sync errors can happen after apply errors in > multiple > ways like via Alter Subscription ... Refresh ... > > So, I don't see the need to change the order here. What do you or others > think? In the alter subscription case, any errors after the table sync would increment apply_error_count. I mentioned this, because this point of view would impact on the doc read by users and internal source codes for developers. I had a concern that when we extend and increase a lot of statistics (not only for this view, but also other statistics in general), writing doc for statistics needs some alignment for better readability. *But*, as you mentioned, in case we don't have such a correlation, I'm okay with the current patch. Thank you for replying. Best Regards, Takamichi Osumi
RE: Design of pg_stat_subscription_workers vs pgstats
On Saturday, February 26, 2022 11:51 AM Amit Kapila wrote: > I have reviewed the latest version and made a few changes along with fixing > some of the pending comments by Peter Smith. The changes are as > follows: (a) Removed m_databaseid in PgStat_MsgSubscriptionError as that is > not required now; (b) changed the struct name PgStat_MsgSubscriptionPurge > to PgStat_MsgSubscriptionDrop to make it similar to DropDb; (c) changed the > view name to pg_stat_subscription_stats, we can reconsider it in future if > there > is a consensus on some other name, accordingly changed the reset function > name to pg_stat_reset_subscription_stats; (d) moved some of the newly > added subscription stats functions adjacent to slots to main the consistency > in > code; (e) changed comments at few places; (f) added LATERAL back to > system_views query as we refer pg_subscription's oid in the function call, > previously that was not clear. > > Do let me know what you think of the attached? Hi, thank you for updating the patch ! I have a couple of comments on v4. (1) I'm not sure if I'm correct, but I'd say the sync_error_count can come next to the subname as the order of columns. I felt there's case that the column order is somewhat related to the time/processing order (I imagined pg_stat_replication's LSN related columns). If this was right, table sync related column could be the first column as a counter within this patch. (2) doc/src/sgml/monitoring.sgml +Resets statistics for a single subscription shown in the +pg_stat_subscription_stats view to zero. If +the argument is NULL, reset statistics for all +subscriptions. I felt we could improve the first sentence. From: Resets statistics for a single subscription shown in the.. To(idea1): Resets statistics for a single subscription defined by the argument to zero. Or, To(idea2): Resets statistics to zero for a single subscription or for all subscriptions. Best Regards, Takamichi Osumi
RE: Optionally automatically disable logical replication subscriptions on error
On Tuesday, February 22, 2022 3:03 PM Peter Smith wrote:
> Here are a couple more review comments for v21.
>
> ~~~
>
> 1. worker.c - comment
>
> + subform = (Form_pg_subscription) GETSTRUCT(tup);
> +
> + /*
> + * We would not be here unless this subscription's disableonerr field
> + was
> + * true, but check whether that field has changed in the interim.
> + */
> + if (!subform->subdisableonerr)
> + {
> + heap_freetuple(tup);
> + table_close(rel, RowExclusiveLock);
> + CommitTransactionCommand();
> + return false;
> + }
>
> I felt that comment belongs above the subform assignment because that is the
> only reason we are getting the subform again.
This part has been removed along with the modification
that we just disable the subscription in the main processing
when we get an error.
> ~~
>
> 2. worker.c - subform->oid
>
> + /* Notify the subscription will be no longer valid */ ereport(LOG,
> + errmsg("logical replication subscription \"%s\" will be disabled due
> to an error",
> +MySubscription->name));
> +
> + LockSharedObject(SubscriptionRelationId, subform->oid, 0,
> AccessExclusiveLock);
>
> Can't we just use MySubscription->oid here? We really only needed that
> subform to get new option values.
Fixed.
> ~~
>
> 3. worker.c - whitespace
>
> Your pg_indent has also changed some whitespace for parts of worker.c that
> are completely unrelated to this patch. You might want to revert those
> changes.
Fixed.
Kindly have a look at v22 that took in all your comments.
It's shared in [1].
[1] -
https://www.postgresql.org/message-id/TYCPR01MB8373D9B26F988307B0D3FE20ED3E9%40TYCPR01MB8373.jpnprd01.prod.outlook.com
Best Regards,
Takamichi Osumi
RE: Optionally automatically disable logical replication subscriptions on error
On Wednesday, February 23, 2022 6:52 PM Tang, Haiying/唐 海英 wrote: > I have a comment on v21 patch. > > I wonder if we really need subscription s2 in 028_disable_on_error.pl. I > think for > subscription s2, we only tested some normal cases(which could be tested with > s1), and didn't test any error case, which means it wouldn't be automatically > disabled. > Is there any reason for creating subscription s2? Removed the subscription s2. This has reduced the code amount of TAP tests. Kindly have a look at the v22 shared in [1]. [1] - https://www.postgresql.org/message-id/TYCPR01MB8373D9B26F988307B0D3FE20ED3E9%40TYCPR01MB8373.jpnprd01.prod.outlook.com Best Regards, Takamichi Osumi
RE: Optionally automatically disable logical replication subscriptions on error
On Thursday, February 24, 2022 4:50 PM Masahiko Sawada
wrote:
> On Tue, Feb 22, 2022 at 3:03 PM Peter Smith
> wrote:
> >
> > ~~~
> >
> > 1. worker.c - comment
> >
> > + subform = (Form_pg_subscription) GETSTRUCT(tup);
> > +
> > + /*
> > + * We would not be here unless this subscription's disableonerr field
> > + was
> > + * true, but check whether that field has changed in the interim.
> > + */
> > + if (!subform->subdisableonerr)
> > + {
> > + heap_freetuple(tup);
> > + table_close(rel, RowExclusiveLock);
> > + CommitTransactionCommand();
> > + return false;
> > + }
> >
> > I felt that comment belongs above the subform assignment because that
> > is the only reason we are getting the subform again.
>
> IIUC if we return false here, the same error will be emitted twice.
> And I'm not sure this check is really necessary. It would work only when the
> subdisableonerr is set to false concurrently, but doesn't work for the
> opposite
> caces. I think we can check
> MySubscription->disableonerr and then just update the tuple.
Addressed. I followed your advice and deleted the check.
Kindly have a look at v22 shared in [1].
[1] -
https://www.postgresql.org/message-id/TYCPR01MB8373D9B26F988307B0D3FE20ED3E9%40TYCPR01MB8373.jpnprd01.prod.outlook.com
Best Regards,
Takamichi Osumi
RE: Optionally automatically disable logical replication subscriptions on error
On Thursday, February 24, 2022 8:09 PM Amit Kapila > On Thu, Feb 24, 2022 at 1:20 PM Masahiko Sawada > wrote: > > + /* > > +* Log the error that caused DisableSubscriptionOnError to be > called. We > > +* do this immediately so that it won't be lost if some other > > internal > > +* error occurs in the following code. > > +*/ > > + EmitErrorReport(); > > + AbortOutOfAnyTransaction(); > > + FlushErrorState(); > > > > Do we need to hold interrupts during cleanup here? > > > > I think so. We do prevent interrupts via > HOLD_INTERRUPTS/RESUME_INTERRUPTS during cleanup. Fixed. Kindly have a look at v22 shared in [1]. [1] - https://www.postgresql.org/message-id/TYCPR01MB8373D9B26F988307B0D3FE20ED3E9%40TYCPR01MB8373.jpnprd01.prod.outlook.com Best Regards, Takamichi Osumi
RE: Optionally automatically disable logical replication subscriptions on error
On Friday, February 25, 2022 12:57 PM Amit Kapila wrote: > On Thu, Feb 24, 2022 at 6:30 PM Masahiko Sawada > wrote: > > > > On Thu, Feb 24, 2022 at 8:08 PM Amit Kapila > wrote: > > > > > > On Thu, Feb 24, 2022 at 1:20 PM Masahiko Sawada > wrote: > > > > > > > > Here are some comments: > > > > > > > > Why do we need SyncTableStartWrapper() and ApplyLoopWrapper()? > > > > > > > > > > I have given this comment to move the related code to separate > > > functions to slightly simplify ApplyWorkerMain() code but if you > > > don't like we can move it back. I am not sure I like the new > > > function names in the patch though. > > > > Okay, I'm fine with moving this code but perhaps we can find a better > > function name as "Wrapper" seems slightly odd to me. > > > > Agreed. > > > For example, > > start_table_sync_start() and start_apply_changes() or something (it > > seems we use the snake case for static functions in worker.c). > > > > I am fine with something on these lines, how about start_table_sync() and > start_apply() respectively? Adopted. (If we come up with better names, we can change those then) Kindly have a look at attached the v22. It has incorporated other improvements of TAP test, refinement of the DisableSubscriptionOnError function and so on. Best Regards, Takamichi Osumi v22-0001-Optionally-disable-subscriptions-on-error.patch Description: v22-0001-Optionally-disable-subscriptions-on-error.patch
RE: Failed transaction statistics to measure the logical replication progress
On Wednesday, February 23, 2022 3:30 PM Amit Kapila > On Tue, Feb 22, 2022 at 6:45 AM tanghy.f...@fujitsu.com > wrote: > > > > I found a problem when using it. When a replication workers exits, the > > transaction stats should be sent to stats collector if they were not > > sent before because it didn't reach PGSTAT_STAT_INTERVAL. But I saw > > that the stats weren't updated as expected. > > > > I looked into it and found that the replication worker would send the > > transaction stats (if any) before it exits. But it got invalid subid > > in pgstat_send_subworker_xact_stats(), which led to the following result: > > > > postgres=# select pg_stat_get_subscription_worker(0, null); > > pg_stat_get_subscription_worker > > - > > (0,,2,0,00,"",) > > (1 row) > > > > I think that's because subid has already been cleaned when trying to > > send the stats. I printed the value of before_shmem_exit_list, the > > functions in this list would be called in shmem_exit() when the worker > > exits. > > logicalrep_worker_onexit() would clean up the worker info (including > > subid), and > > pgstat_shutdown_hook() would send stats if any. > > logicalrep_worker_onexit() was called before calling > pgstat_shutdown_hook(). > > > > Yeah, I think that is a problem and maybe we can think of solving it by > sending > the stats via logicalrep_worker_onexit before subid is cleared but not sure > that > is a good idea. I feel we need to go back to the idea of v21 for sending stats > instead of using pgstat_report_stat. > I think the one thing which we could improve is to avoid trying to send it > each > time before receiving each message by walrcv_receive and rather try to send it > before we try to wait (WaitLatchOrSocket). > Trying after each message doesn't seem to be required and could lead to some > overhead as well. What do you think? I agree. Fixed. Kindly have a look at v24 shared in [1]. [1] - https://www.postgresql.org/message-id/TYCPR01MB8373A3E1BE237BAF38185BF2ED3D9%40TYCPR01MB8373.jpnprd01.prod.outlook.com Best Regards, Takamichi Osumi
RE: Failed transaction statistics to measure the logical replication progress
On Thursday, February 24, 2022 11:07 AM Masahiko Sawada
wrote:
> I have some comments on v23 patch:
>
> @@ -66,6 +66,12 @@ typedef struct LogicalRepWorker
> TimestampTz last_recv_time;
> XLogRecPtr reply_lsn;
> TimestampTz reply_time;
> +
> + /*
> +* Transaction statistics of subscription worker
> +*/
> + int64 commit_count;
> + int64 abort_count;
> } LogicalRepWorker;
>
> I think that adding these statistics to the struct whose data is allocated on
> the
> shared memory is not a good idea since they don't need to be shared. We might
> want to add more statistics for subscriptions such as insert_count and
> update_count in the future. I think it's better to track these statistics in
> local
> memory either in worker.c or pgstat.c.
Fixed.
> +/* --
> + * pgstat_report_subworker_xact_end() -
> + *
> + * Update the statistics of subscription worker and have
> + * pgstat_report_stat send a message to stats collector
> + * after count increment.
> + * --
> + */
> +void
> +pgstat_report_subworker_xact_end(bool is_commit) {
> +if (is_commit)
> +MyLogicalRepWorker->commit_count++;
> +else
> +MyLogicalRepWorker->abort_count++;
> +}
>
> It's slightly odd and it seems unnecessary to me that we modify fields of
> MyLogicalRepWorker in pgstat.c. Although this function has “report”
> in its name but it just increments the counter. I think we can do that in
> worker.c.
Fixed.
Also, I made the timing adjustment logic
back and now have the independent one as Amit-san suggested in [1].
Kindly have a look at v24.
[1] -
https://www.postgresql.org/message-id/CAA4eK1LWYc15%3DASj1tMTEFsXtxu%3D02aGoMwq9YanUVr9-QMhdQ%40mail.gmail.com
Best Regards,
Takamichi Osumi
v24-0001-Extend-pg_stat_subscription_workers-to-include-g.patch
Description: v24-0001-Extend-pg_stat_subscription_workers-to-include-g.patch
RE: Optionally automatically disable logical replication subscriptions on error
On Wednesday, February 23, 2022 6:52 PM Tang, Haiying/唐 海英 wrote: > I have a comment on v21 patch. > > I wonder if we really need subscription s2 in 028_disable_on_error.pl. I > think for > subscription s2, we only tested some normal cases(which could be tested with > s1), and didn't test any error case, which means it wouldn't be automatically > disabled. > Is there any reason for creating subscription s2? Hi, thank you for your review ! It's for checking there's no impact/influence when disabling one subscription on the other subscription if any. *But*, when I have a look at the past tests to add options (e.g. streaming, two_phase), we don't have this kind of test that I have for disable_on_error patch. Therefore, I'd like to fix the test as you suggested in my next version. Best Regards, Takamichi Osumi
RE: Design of pg_stat_subscription_workers vs pgstats
On Tuesday, February 22, 2022 11:47 PM Masahiko Sawada wrote: > On Tue, Feb 22, 2022 at 9:22 PM osumi.takami...@fujitsu.com > wrote: > > (4) doc/src/sgml/monitoring.sgml > > > > > > role="column_definition"> > > - last_error_time timestamp with > time zone > > + sync_error_count uint8 > > > > > > - Last time at which this error occurred > > + Number of times the error occurred during the initial data > > + copy > > > > > > I supposed it might be better to use "initial data sync" > > or "initial data synchronization", rather than "initial data copy". > > "Initial data synchronization" sounds like the whole table synchronization > process including COPY and applying changes to catch up. But > sync_error_count is incremented only during COPY so I used "initial data > copy". > What do you think? Okay. Please keep it as is. Best Regards, Takamichi Osumi
RE: Design of pg_stat_subscription_workers vs pgstats
On Tuesday, February 22, 2022 2:45 PM Masahiko Sawada
wrote:
> I've attached a patch that changes pg_stat_subscription_workers view.
> It removes non-cumulative values such as error details such as error-XID and
> the error message from the view, and consequently the view now has only
> cumulative statistics counters: apply_error_count and sync_error_count. Since
> the new view name is under discussion I temporarily chose
> pg_stat_subscription_activity.
Hi, thank you for sharing the patch.
Few minor comments for v1.
(1) commit message's typo
This commits changes the view so that it stores only statistics
counters: apply_error_count and sync_error_count.
"This commits" -> "This commit"
(2) minor improvement suggestion for the commit message
I suggest that we touch the commit id 8d74fc9
that introduces the pg_stat_subscription_workers
in the commit message, for better traceability. Below is an example.
From:
As the result of the discussion, we've concluded that the stats
collector is not an appropriate place to store the error information of
subscription workers.
To:
As the result of the discussion about the view introduced by 8d74fc9,...
(3) doc/src/sgml/logical-replication.sgml
Kindly refer to commit id 85c61ba for the detail.
You forgot "the" in the below sentence.
@@ -346,8 +346,6 @@
A conflict will produce an error and will stop the replication; it must be
resolved manually by the user. Details about the conflict can be found in
-
- pg_stat_subscription_workers and the
subscriber's server log.
From:
subscriber's server log.
to:
the subscriber's server log.
(4) doc/src/sgml/monitoring.sgml
- last_error_time timestamp with time
zone
+ sync_error_count uint8
- Last time at which this error occurred
+ Number of times the error occurred during the initial data copy
I supposed it might be better to use "initial data sync"
or "initial data synchronization", rather than "initial data copy".
(5) src/test/subscription/t/026_worker_stats.pl
+# Truncate test_tab1 so that table sync can continue.
+$node_subscriber->safe_psql('postgres', "TRUNCATE test_tab1;");
The second truncate is for apply, isn't it? Therefore, kindly change
From:
Truncate test_tab1 so that table sync can continue.
To:
Truncate test_tab1 so that apply can continue.
(6) src/test/subscription/t/026_worker_stats.pl
+# Insert more data to test_tab1 on the subscriber and then on the publisher,
raising an
+# error on the subscriber due to violation of the unique constraint on
test_tab1.
+$node_subscriber->safe_psql('postgres', "INSERT INTO test_tab1 VALUES (2)");
Did we need this insert ?
If you want to indicate the apply is working okay after the error of table sync
is solved,
waiting for the max value in the test_tab1 becoming 2 on the subscriber by
polling query
would work. But, I was not sure if this is essentially necessary for the
testing purpose.
Best Regards,
Takamichi Osumi
RE: Optionally automatically disable logical replication subscriptions on error
On Tuesday, February 22, 2022 7:53 AM Peter Smith wrote:
> On Mon, Feb 21, 2022 at 11:44 PM osumi.takami...@fujitsu.com
> wrote:
> >
> > On Monday, February 21, 2022 2:56 PM Peter Smith
> wrote:
> > > Thanks for addressing my previous comments. Now I have looked at v19.
> > >
> > > On Mon, Feb 21, 2022 at 11:25 AM osumi.takami...@fujitsu.com
> > > wrote:
> > > >
> > > > On Friday, February 18, 2022 3:27 PM Peter Smith
> > > wrote:
> > > > > Hi. Below are my code review comments for v18.
> > > > Thank you for your review !
> > > ...
> > > > > 5. src/backend/replication/logical/worker.c -
> > > > > DisableSubscriptionOnError
> > > > >
> > > > > + /*
> > > > > + * We would not be here unless this subscription's disableonerr
> > > > > + field was
> > > > > + * true when our worker began applying changes, but check
> > > > > + whether that
> > > > > + * field has changed in the interim.
> > > > > + */
> > > > >
> > > > > Apparently, this function might just do nothing if it detects
> > > > > some situation where the flag was changed somehow, but I'm not
> > > > > 100% sure that the callers are properly catering for when nothing
> happens.
> > > > >
> > > > > IMO it would be better if this function would return true/false
> > > > > to mean "did disable subscription happen or not?" because that
> > > > > will give the calling code the chance to check the function
> > > > > return and do the right thing - e.g. if the caller first thought
> > > > > it should be disabled but then
> > > it turned out it did NOT disable...
> > > > I don't think we need to do something more.
> > > > After this function, table sync worker and the apply worker just exit.
> > > > IMO, we don't need to do additional work for already-disabled
> > > > subscription on the caller sides.
> > > > It should be sufficient to fulfill the purpose of
> > > > DisableSubscriptionOnError or confirm it has been fulfilled.
> > >
> > > Hmmm - Yeah, it may be the workers might just exit soon after
> > > anyhow as you say so everything comes out in the wash, but still, I
> > > felt for this case when DisableSubscriptionOnError turned out to do
> > > nothing it would be better to exit via the existing logic. And that is
> > > easy to do
> if the function returns true/false.
> > >
> > > For example, changes like below seemed neater code to me. YMMV.
> > >
> > > BEFORE (SyncTableStartWrapper):
> > > if (MySubscription->disableonerr)
> > > {
> > > DisableSubscriptionOnError();
> > > proc_exit(0);
> > > }
> > > AFTER
> > > if (MySubscription->disableonerr && DisableSubscriptionOnError())
> > > proc_exit(0);
> > >
> > > BEFORE (ApplyLoopWrapper)
> > > if (MySubscription->disableonerr)
> > > {
> > > /* Disable the subscription */
> > > DisableSubscriptionOnError();
> > > return;
> > > }
> > > AFTER
> > > if (MySubscription->disableonerr && DisableSubscriptionOnError())
> > > return;
> > Okay, so this return value works for better readability.
> > Fixed.
> >
> >
> > > ~~~
> > >
> > > Here are a couple more comments:
> > >
> > > 1. src/backend/replication/logical/worker.c -
> > > DisableSubscriptionOnError, Refactor error handling
> > >
> > > (this comment assumes the above gets changed too)
> > I think those are independent.
>
> OK. I was only curious if the change #5 above might cause the error to be
> logged
> 2x, if the DisableSubscriptionOnError returns false.
> - firstly, when it logs errors within the function
> - secondly, by normal error mechanism when the caller re-throws it.
>
> But, if you are sure that won't happen then it is good news.
I didn't feel this would become a substantial issue.
When we alter subscription with disable_on_error = false
after we go into the DisableSubscriptionOnError,
we don't disable the subscription in the same function.
That means we launch new apply workers repeatedly after that
until we solve the error cause or we set the disable_on_error = true again.
So, if we confirm that the disable_on_error = false in the
DisableSubscriptionOnError,
it's highly possible that we'll get more same
RE: Failed transaction statistics to measure the logical replication progress
On Tuesday, February 22, 2022 10:15 AM Tang, Haiying/唐 海英
wrote:
> On Mon, Feb 21, 2022 11:46 AM osumi.takami...@fujitsu.com
> wrote:
> >
> > On Saturday, February 19, 2022 12:00 AM osumi.takami...@fujitsu.com
> > wrote:
> > > On Friday, February 18, 2022 3:34 PM Tang, Haiying/唐 海英
> > > wrote:
> > > > On Wed, Jan 12, 2022 8:35 PM osumi.takami...@fujitsu.com
> > > > wrote:
> > > > 4) I noticed that the abort_count doesn't include aborted
> > > > streaming transactions.
> > > > Should we take this case into consideration?
> > > Hmm, we can add this into this column, when there's no objection.
> > > I'm not sure but someone might say those should be separate columns.
> > I've addressed this point in a new v23 patch, since there was no
> > opinion on this so far.
> >
> > Kindly have a look at the attached one.
> >
>
> Thanks for updating the patch.
>
> I found a problem when using it. When a replication workers exits, the
> transaction stats should be sent to stats collector if they were not sent
> before
> because it didn't reach PGSTAT_STAT_INTERVAL. But I saw that the stats
> weren't updated as expected.
>
> I looked into it and found that the replication worker would send the
> transaction
> stats (if any) before it exits. But it got invalid subid in
> pgstat_send_subworker_xact_stats(), which led to the following result:
>
> postgres=# select pg_stat_get_subscription_worker(0, null);
> pg_stat_get_subscription_worker
> -
> (0,,2,0,00,"",)
> (1 row)
>
> I think that's because subid has already been cleaned when trying to send the
> stats. I printed the value of before_shmem_exit_list, the functions in this
> list
> would be called in shmem_exit() when the worker exits.
> logicalrep_worker_onexit() would clean up the worker info (including subid),
> and
> pgstat_shutdown_hook() would send stats if any. logicalrep_worker_onexit()
> was called before calling pgstat_shutdown_hook().
>
> (gdb) p before_shmem_exit_list
> $1 = {{function = 0xa88f1e , arg = 0}, {function =
> 0xb619e7 , arg = 0}, {function = 0xb07b5c
> , arg = 0}, {
> function = 0xabdd93 , arg = 0}, {function =
> 0xe30c89 , arg = 0}, {function = 0x0, arg = 0} times>}
>
> Maybe we should make some modification to fix it.
Thank you for letting me know this issue.
I'll investigate this and will report the result.
Best Regards,
Takamichi Osumi
RE: Optionally automatically disable logical replication subscriptions on error
On Monday, February 21, 2022 2:56 PM Peter Smith wrote:
> Thanks for addressing my previous comments. Now I have looked at v19.
>
> On Mon, Feb 21, 2022 at 11:25 AM osumi.takami...@fujitsu.com
> wrote:
> >
> > On Friday, February 18, 2022 3:27 PM Peter Smith
> wrote:
> > > Hi. Below are my code review comments for v18.
> > Thank you for your review !
> ...
> > > 5. src/backend/replication/logical/worker.c -
> > > DisableSubscriptionOnError
> > >
> > > + /*
> > > + * We would not be here unless this subscription's disableonerr
> > > + field was
> > > + * true when our worker began applying changes, but check whether
> > > + that
> > > + * field has changed in the interim.
> > > + */
> > >
> > > Apparently, this function might just do nothing if it detects some
> > > situation where the flag was changed somehow, but I'm not 100% sure
> > > that the callers are properly catering for when nothing happens.
> > >
> > > IMO it would be better if this function would return true/false to
> > > mean "did disable subscription happen or not?" because that will
> > > give the calling code the chance to check the function return and do
> > > the right thing - e.g. if the caller first thought it should be disabled
> > > but then
> it turned out it did NOT disable...
> > I don't think we need to do something more.
> > After this function, table sync worker and the apply worker just exit.
> > IMO, we don't need to do additional work for already-disabled
> > subscription on the caller sides.
> > It should be sufficient to fulfill the purpose of
> > DisableSubscriptionOnError or confirm it has been fulfilled.
>
> Hmmm - Yeah, it may be the workers might just exit soon after anyhow as you
> say so everything comes out in the wash, but still, I felt for this case when
> DisableSubscriptionOnError turned out to do nothing it would be better to exit
> via the existing logic. And that is easy to do if the function returns
> true/false.
>
> For example, changes like below seemed neater code to me. YMMV.
>
> BEFORE (SyncTableStartWrapper):
> if (MySubscription->disableonerr)
> {
> DisableSubscriptionOnError();
> proc_exit(0);
> }
> AFTER
> if (MySubscription->disableonerr && DisableSubscriptionOnError())
> proc_exit(0);
>
> BEFORE (ApplyLoopWrapper)
> if (MySubscription->disableonerr)
> {
> /* Disable the subscription */
> DisableSubscriptionOnError();
> return;
> }
> AFTER
> if (MySubscription->disableonerr && DisableSubscriptionOnError()) return;
Okay, so this return value works for better readability.
Fixed.
> ~~~
>
> Here are a couple more comments:
>
> 1. src/backend/replication/logical/worker.c - DisableSubscriptionOnError,
> Refactor error handling
>
> (this comment assumes the above gets changed too)
I think those are independent.
> +static void
> +DisableSubscriptionOnError(void)
> +{
> + Relation rel;
> + bool nulls[Natts_pg_subscription];
> + bool replaces[Natts_pg_subscription];
> + Datum values[Natts_pg_subscription];
> + HeapTuple tup;
> + Form_pg_subscription subform;
> +
> + /* Emit the error */
> + EmitErrorReport();
> + /* Abort any active transaction */
> + AbortOutOfAnyTransaction();
> + /* Reset the ErrorContext */
> + FlushErrorState();
> +
> + /* Disable the subscription in a fresh transaction */
> + StartTransactionCommand();
>
> If this DisableSubscriptionOnError function decides later that actually the
> 'disableonerr' flag is false (i.e. it's NOT going to disable the subscription
> after
> all) then IMO it make more sense that the error logging for that case should
> just
> do whatever it is doing now by the normal error processing mechanism.
>
> In other words, I thought perhaps the code to EmitErrorReport/FlushError state
> etc be moved to be BELOW the if
> (!subform->subdisableonerr) bail-out code?
>
> Please see what you think in my attached POC [1]. It seems neater to me, and
> tests are all OK. Maybe I am mistaken...
I had a concern that this order change of codes would have a negative
impact when we have another new error during the call of
DisableSubscriptionOnError.
With the debugger, I raised an error in this function before emitting the
original error.
As a result, the original error that makes the apply worker go into the path of
DisableSubscriptionOnError (in my test, duplication error) has vanished.
In this sense, v19 looks safer, and the current order to handle error recovery
first
looks better to me.
FYI, after the 2nd debugger error,
the next new a
RE: Failed transaction statistics to measure the logical replication progress
On Monday, February 21, 2022 6:06 PM Monday, February 21, 2022 6:06 PM wrote:
> On Mon, Feb 21, 2022 11:46 AM Osumi, Takamichi/大墨 昂道
> wrote:
> >I've addressed this point in a new v23 patch, since there was no opinion on
> this so far.
> >Kindly have a look at the attached one.
> Thanks for updating the patch. Here is a comment:
>
> In function apply_handle_stream_abort:
> @@ -1217,6 +1219,7 @@ apply_handle_stream_abort(StringInfo s)
> {
> set_apply_error_context_xact(xid, 0);
> stream_cleanup_files(MyLogicalRepWorker->subid, xid);
> + pgstat_report_subworker_xact_end(false);
> }
> else
> {
>
> I think there is a problem here, pgstat_report_stat is not invoked here.
> While the other three places where function
> pgstat_report_subworker_xact_end is invoked, the function pgstat_report_stat
> is invoked.
> Do we need to invoke pgstat_report_stat in apply_handle_stream_abort?
Hi,
I had tested this case before I posted the latest patch v23.
It works when I call pg_stat_report_stat by other transaction.
But, if we want to add pgstat_report_stat here,
I need to investigate the impact of the addition.
I'll check it and let you know.
Best Regards,
Takamichi Osumi
RE: Failed transaction statistics to measure the logical replication progress
On Saturday, February 19, 2022 12:00 AM osumi.takami...@fujitsu.com wrote: > On Friday, February 18, 2022 3:34 PM Tang, Haiying/唐 海英 > wrote: > > On Wed, Jan 12, 2022 8:35 PM osumi.takami...@fujitsu.com > > wrote: > > 4) I noticed that the abort_count doesn't include aborted streaming > > transactions. > > Should we take this case into consideration? > Hmm, we can add this into this column, when there's no objection. > I'm not sure but someone might say those should be separate columns. I've addressed this point in a new v23 patch, since there was no opinion on this so far. Kindly have a look at the attached one. Best Regards, Takamichi Osumi v23-0001-Extend-pg_stat_subscription_workers-to-include-g.patch Description: v23-0001-Extend-pg_stat_subscription_workers-to-include-g.patch
RE: Optionally automatically disable logical replication subscriptions on error
On Friday, February 18, 2022 3:27 PM Peter Smith wrote:
> Hi. Below are my code review comments for v18.
Thank you for your review !
> ==
>
> 1. Commit Message - wording
>
> BEFORE
> To partially remedy the situation, adding a new subscription_parameter named
> 'disable_on_error'.
>
> AFTER
> To partially remedy the situation, this patch adds a new
> subscription_parameter named 'disable_on_error'.
Fixed.
> ~~~
>
> 2. Commit message - wording
>
> BEFORE
> Require to bump catalog version.
>
> AFTER
> A catalog version bump is required.
Fixed.
> ~~~
>
> 3. doc/src/sgml/ref/alter_subscription.sgml - whitespace
>
> @@ -201,8 +201,8 @@ ALTER SUBSCRIPTION class="parameter">name RENAME TO <
>information. The parameters that can be altered
>are slot_name,
>synchronous_commit,
> - binary, and
> - streaming.
> + binary,streaming, and
> + disable_on_error.
>
>
> There is a missing space before streaming.
Fixed.
> ~~~
>
> 4. src/backend/replication/logical/worker.c - WorkerErrorRecovery
>
> @@ -2802,6 +2803,89 @@ LogicalRepApplyLoop(XLogRecPtr
> last_received) }
>
> /*
> + * Worker error recovery processing, in preparation for disabling the
> + * subscription.
> + */
> +static void
> +WorkerErrorRecovery(void)
>
> I was wondering about the need for this to be a separate function? It is only
> called immediately before calling 'DisableSubscriptionOnError'
> so would it maybe be better just to put this code inside
> DisableSubscriptionOnError with the appropriate comments?
I preferred to have one specific for error handling,
because from caller sides, when we catch error, it's apparent
that error recovery is done. But, the function name "DisableSubscriptionOnError"
by itself should have the nuance that we do something on error.
So, we can think that it's okay to have error recovery processing
in this function.
So, I removed the function and fixed some related comments.
> ~~~
>
> 5. src/backend/replication/logical/worker.c - DisableSubscriptionOnError
>
> + /*
> + * We would not be here unless this subscription's disableonerr field
> + was
> + * true when our worker began applying changes, but check whether that
> + * field has changed in the interim.
> + */
>
> Apparently, this function might just do nothing if it detects some situation
> where the flag was changed somehow, but I'm not 100% sure that the callers
> are properly catering for when nothing happens.
>
> IMO it would be better if this function would return true/false to mean "did
> disable subscription happen or not?" because that will give the calling code
> the
> chance to check the function return and do the right thing - e.g. if the
> caller first
> thought it should be disabled but then it turned out it did NOT disable...
I don't think we need to do something more.
After this function, table sync worker and the apply worker
just exit. IMO, we don't need to do additional work for
already-disabled subscription on the caller sides.
It should be sufficient to fulfill the purpose of
DisableSubscriptionOnError or confirm it has been fulfilled.
> ~~~
>
> 6. src/backend/replication/logical/worker.c - LogicalRepHandleTableSync
> name
>
> +/*
> + * Execute the initial sync with error handling. Disable the
> +subscription,
> + * if it's required.
> + */
> +static void
> +LogicalRepHandleTableSync(XLogRecPtr *origin_startpos,
> + char **myslotname, MemoryContext cctx)
>
> I felt that it is a bit overkill to put a "LogicalRep" prefix here because it
> is a static
> function.
>
> IMO this function should be renamed as 'SyncTableStartWrapper' because that
> describes better what it is doing.
Makes sense. Fixed.
> ~~~
>
> 7. src/backend/replication/logical/worker.c - LogicalRepHandleTableSync
> Assert
>
> Even though we can know this to be true because of where it is called from, I
> think the readability of the function will be improved if you add an
> assertion at
> the top:
>
> Assert(am_tablesync_worker());
Fixed.
> And then, because the function is clearly for Tablesync worker only there is
> no
> need to keep mentioning that in the subsequent comments...
>
> e.g.1
> /* This is table synchronization worker, call initial sync. */
> AFTER:
> /* Call initial sync. */
Fixed.
> e.g.2
> /*
> * Report the table sync error. There is no corresponding message type
> * for table synchronization.
> */
> AFTER
> /*
> * Report the error. There is no corresponding message type for table
> * synchronization.
> */
Agreed. Fixed
> ~~~
>
> 8. src/backend/replication/logical/worker.c - LogicalRepHandleTableSync
> unnecessarily complex
>
> +static void
> +LogicalRepHandleTableSync(XLogRecPtr *origin_startpos,
> + char **myslotname, MemoryContext cctx) {
> + char*syncslotname;
> + bool error_recovery_done = false;
>
> IMO this logic is way more complex than it needed to be. IIUC that
> 'error_recovery_done' and various conditions can be removed, and the whole
RE: Failed transaction statistics to measure the logical replication progress
On Friday, February 18, 2022 3:34 PM Tang, Haiying/唐 海英
wrote:
> On Wed, Jan 12, 2022 8:35 PM osumi.takami...@fujitsu.com
> wrote:
> >
> > The attached v21 has a couple of other minor updates like a
> > modification of error message text.
> >
> >
>
> Thanks for updating the patch. Here are some comments.
Thank you for your reivew !
> 1) I saw the following description about pg_stat_subscription_workers view in
> existing doc:
>
>The pg_stat_subscription_workers view will
> contain
>one row per subscription worker on which errors have occurred, ...
>
> It only says "which errors have occurred", maybe we should also mention
> transactions here, right?
I wrote about this statistics in the next line but as you pointed out,
separating the description into two sentences wasn't good idea.
Fixed.
> 2)
> /* --
> + * pgstat_send_subworker_xact_stats() -
> + *
> + * Send a subworker's transaction stats to the collector.
> + * The statistics are cleared upon return.
>
> Should "The statistics are cleared upon return" changed to "The statistics are
> cleared upon sending"? Because if it doesn't reach PGSTAT_STAT_INTERVAL
> and the transaction stats are not sent, the function will return without
> clearing
> out statistics.
Now, the purpose of this function has become purely
to send a message and whenever it's called, the function
clears the saved stats. So, I skipped this comments now.
> 3)
> + Assert(command == LOGICAL_REP_MSG_COMMIT ||
> +command == LOGICAL_REP_MSG_STREAM_COMMIT ||
> +command == LOGICAL_REP_MSG_COMMIT_PREPARED
> ||
> +command ==
> LOGICAL_REP_MSG_ROLLBACK_PREPARED);
> +
> + switch (command)
> + {
> + case LOGICAL_REP_MSG_COMMIT:
> + case LOGICAL_REP_MSG_STREAM_COMMIT:
> + case LOGICAL_REP_MSG_COMMIT_PREPARED:
> + MyLogicalRepWorker->commit_count++;
> + break;
> + case LOGICAL_REP_MSG_ROLLBACK_PREPARED:
> + MyLogicalRepWorker->abort_count++;
> + break;
> + default:
> + ereport(ERROR,
> + errmsg("invalid logical message type
> for transaction statistics of subscription"));
> + break;
> + }
>
> I'm not sure that do we need the assert, because it will report an error
> later if
> command is an invalid value.
The Assert has been removed, along with the switch branches now.
Since there was an adivce that we should call this from
apply_handle_commit_internal
and from that function, if we don't want to change this function's argument,
all we need to do is to pass a boolean value that indicates the stats is
commit_count or abort_count. Kindly have a look at the updated version.
> 4) I noticed that the abort_count doesn't include aborted streaming
> transactions.
> Should we take this case into consideration?
Hmm, we can add this into this column, when there's no objection.
I'm not sure but someone might say those should be separate columns.
The new patch v22 is shared in [2].
[2] -
https://www.postgresql.org/message-id/TYCPR01MB83737C689F8F310C87C19C1EED379%40TYCPR01MB8373.jpnprd01.prod.outlook.com
Best Regards,
Takamichi Osumi
RE: Failed transaction statistics to measure the logical replication progress
On Friday, February 18, 2022 8:11 PM Amit Kapila wrote: > On Fri, Feb 18, 2022 at 2:04 PM osumi.takami...@fujitsu.com > wrote: > > > > On Thursday, February 17, 2022 6:45 PM Amit Kapila > wrote: > > > On Thu, Feb 17, 2022 at 3:13 PM Amit Kapila > > > > > > wrote: > > > > Can't we use pgstat_report_stat() here? Basically, you can update > > > > xact completetion counters during apply, and then from > > > > pgstat_report_stat(), you can invoke a logical replication worker > > > > stats-related function. > > > > > > > > > > If we can do this then we can save the logic this patch is trying to > > > introduce for PGSTAT_STAT_INTERVAL. > > Hi, I've encounter a couple of questions during my modification, following > your advice. > > > > In the pgstat_report_stat, we refer to the return value of > > GetCurrentTransactionStopTimestamp to compare the time different from > the last time. > > (In my previous patch, I used GetCurrentTimestamp) > > > > This time is updated in apply_handle_commit_internal's > CommitTransactionCommand for the apply worker. > > Then, if I update the subscription worker > > stats(commit_count/abort_count) immediately after this > > CommitTransactionCommand and immediately call pgstat_report_stat in the > apply_handle_commit_internal, the time difference becomes too small (falls > short of PGSTAT_STAT_INTERVAL). > > Also, the time of GetCurrentTransactionStopTimestamp is not updated > > even when I keep calling pgstat_report_stat repeatedly. > > Then, IIUC the next possible timing that message of commit_count or > > abort_count is sent to the stats collector would become the time when > > we execute another transaction by the apply worker and update the time > > for GetCurrentTransactionStopTimestamp > > and rerun pgstat_report_stat again. > > > > I think but same is true in the case of the transaction in the backend where > we > increment commit counter via AtEOXact_PgStat after updating the transaction > time. After that, we call pgstat_report_stat() at later point. How is this > case > different? > > > So, if we keep GetCurrentTransactionStopTimestamp without change, an > > update of stats depends on another new subsequent transaction if we > simply merge those. > > (this leads to users cannot see the latest stats information ?) > > > > I think this should be okay as these don't need to be accurate. > > > At least, I got a test failure because of this function for streaming > > commit case because it uses poll_query_until to wait for stats update. > > > > I feel it is not a good idea to wait for the accurate update of these > counters. Ah, then I had wrote tests based on totally wrong direction and made noises for it. Sorry for that. I don't see tests for existing xact_commit/rollback count, so I'll follow the same way. Attached a new patch that addresses three major improvements I've got so far as comments. 1. skip increment of stats counter by empty transaction, on the subscriber side (except for commit prepared) 2. utilize the existing pgstat_report_stat, instead of having a similar logic newly. 3. remove the wrong tests. Best Regards, Takamichi Osumi v22-0001-Extend-pg_stat_subscription_workers-to-include-g.patch Description: v22-0001-Extend-pg_stat_subscription_workers-to-include-g.patch
RE: logical replication empty transactions
On Friday, February 18, 2022 6:18 PM Amit Kapila wrote: > On Tue, Feb 8, 2022 at 5:27 AM osumi.takami...@fujitsu.com > wrote: > > > > On Friday, August 13, 2021 8:01 PM Ajin Cherian wrote: > > > On Mon, Aug 2, 2021 at 7:20 PM Amit Kapila > > > wrote: > > Changing the timing to send the keepalive to the decoding commit > > timing didn't look impossible to me, although my suggestion can be > > ad-hoc. > > > > After the initialization of sentPtr(by confirmed_flush lsn), sentPtr > > is updated from logical_decoding_ctx->reader->EndRecPtr in > XLogSendLogical. > > In the XLogSendLogical, we update it after we execute > LogicalDecodingProcessRecord. > > This order leads to the current implementation to wait the next > > iteration to send a keepalive in WalSndWaitForWal. > > > > But, I felt we can utilize end_lsn passed to ReorderBufferCommit for > > updating sentPtr. The end_lsn is the lsn same as the > > ctx->reader->EndRecPtr, which means advancing the timing to update the > sentPtr for the commit case. > > Then if the transaction is empty in synchronous mode, send the > > keepalive in WalSndUpdateProgress directly, instead of having the > > force_keepalive_syncrep flag and having it true. > > > > You have a point in that we don't need to delay sending this message till next > WalSndWaitForWal() but I don't see why we need to change anything about > update of sentPtr. Yeah, you're right. Now I think we don't need the update of sentPtr to send a keepalive. I thought we can send a keepalive message after its update in XLogSendLogical or any appropriate place for it after the existing update. Best Regards, Takamichi Osumi
RE: Failed transaction statistics to measure the logical replication progress
On Thursday, February 17, 2022 6:45 PM Amit Kapila
wrote:
> On Thu, Feb 17, 2022 at 3:13 PM Amit Kapila
> wrote:
> >
> > On Tue, Jan 4, 2022 at 5:22 PM osumi.takami...@fujitsu.com
> > wrote:
> > >
> > > On Friday, December 31, 2021 10:12 AM Tang, Haiying/唐 海英
> wrote:
> > > > 4)
> > > > +void
> > > > +pgstat_send_subworker_xact_stats(LogicalRepWorker *repWorker,
> > > > +bool
> > > > +force) {
> > > > + static TimestampTz last_report = 0;
> > > > + PgStat_MsgSubWorkerXactEnd msg;
> > > > +
> > > > + if (!force)
> > > > + {
> > > > ...
> > > > + if (!TimestampDifferenceExceeds(last_report, now,
> > > > PGSTAT_STAT_INTERVAL))
> > > > + return;
> > > > + last_report = now;
> > > > + }
> > > > +
> > > > ...
> > > > + if (repWorker->commit_count == 0 && repWorker->abort_count
> > > > + ==
> > > > 0)
> > > > + return;
> > > > ...
> > > >
> > > > I think it's better to check commit_count and abort_count first,
> > > > then check if reach PGSTAT_STAT_INTERVAL.
> > > > Otherwise if commit_count and abort_count are 0, it is possible
> > > > that the value of last_report has been updated but it didn't send
> > > > stats in fact. In this case, last_report is not the real time that send
> > > > last
> message.
> > > Yeah, agreed. This fix is right in terms of the variable name aspect.
> > >
> >
> > Can't we use pgstat_report_stat() here? Basically, you can update xact
> > completetion counters during apply, and then from
> > pgstat_report_stat(), you can invoke a logical replication worker
> > stats-related function.
> >
>
> If we can do this then we can save the logic this patch is trying to
> introduce for
> PGSTAT_STAT_INTERVAL.
Hi, I've encounter a couple of questions during my modification, following your
advice.
In the pgstat_report_stat, we refer to the return value of
GetCurrentTransactionStopTimestamp to compare the time different from the last
time.
(In my previous patch, I used GetCurrentTimestamp)
This time is updated in apply_handle_commit_internal's CommitTransactionCommand
for the apply worker.
Then, if I update the subscription worker stats(commit_count/abort_count)
immediately after
this CommitTransactionCommand and immediately call pgstat_report_stat in the
apply_handle_commit_internal,
the time difference becomes too small (falls short of PGSTAT_STAT_INTERVAL).
Also, the time of GetCurrentTransactionStopTimestamp is not updated
even when I keep calling pgstat_report_stat repeatedly.
Then, IIUC the next possible timing that message of commit_count or abort_count
is sent to the stats collector would become the time when we execute another
transaction
by the apply worker and update the time for GetCurrentTransactionStopTimestamp
and rerun pgstat_report_stat again.
So, if we keep GetCurrentTransactionStopTimestamp without change,
an update of stats depends on another new subsequent transaction if we simply
merge those.
(this leads to users cannot see the latest stats information ?)
At least, I got a test failure because of this function for streaming commit
case
because it uses poll_query_until to wait for stats update.
On the other hand, replacing GetCurrentTransactionStopTimestamp with
GetCurrentTimestamp in case of apply worker looks have another negative impact.
If we do so, it becomes possible that we go into the code to scan
TabStatusArray with
PgStat_TableStatus's trans with non-null values, because of the timing change.
I might be able to avoid this kind of assert failure if I write the message send
function of this patch before other existing functions to send various type of
messages
and return if the process is apply worker in pgstat_report_stat.
But, I can't be convinced that this way of modification is OK.
What did you think about those issues ?
Best Regards,
Takamichi Osumi
RE: Optionally automatically disable logical replication subscriptions on error
On Tuesday, February 15, 2022 2:19 PM I wrote
> On Monday, February 14, 2022 8:58 PM Amit Kapila
> > 2. Can we move the code related to tablesync worker and its error
> > handing (the code insider if (am_tablesync_worker())) to a separate
> > function say
> > LogicalRepHandleTableSync() or something like that.
> >
> > 3. Similarly, we can move apply-loop related code ("Run the main
> > loop.") to a separate function say LogicalRepHandleApplyMessages().
> >
> > If we do (2) and (3), I think the code in ApplyWorkerMain will look
> > better. What do you think?
> I agree with (2) and (3), since those contribute to better readability.
>
> Attached a new patch v17 that addresses those refactorings.
Hi, I noticed that one new tap test was added in the src/test/subscription/
and needed to increment the number of my test of this patch.
Also, I conducted minor fixes of comments and function name.
Kindly have a look at the attached v18.
Best Regards,
Takamichi Osumi
v18-0001-Optionally-disable-subscriptions-on-error.patch
Description: v18-0001-Optionally-disable-subscriptions-on-error.patch
RE: logical replication empty transactions
Hi
I'll quote one other remaining discussion of this thread again
to invoke more attentions from the community.
On Friday, August 13, 2021 8:01 PM Ajin Cherian wrote:
> On Mon, Aug 2, 2021 at 7:20 PM Amit Kapila
> wrote:
> > Few other miscellaneous comments:
> > 1.
> > static void
> > pgoutput_commit_prepared_txn(LogicalDecodingContext *ctx,
> > ReorderBufferTXN *txn,
> > - XLogRecPtr commit_lsn)
> > + XLogRecPtr commit_lsn, XLogRecPtr prepare_end_lsn, TimestampTz
> > + prepare_time)
> > {
> > + PGOutputTxnData*txndata = (PGOutputTxnData *)
> txn->output_plugin_private;
> > +
> > OutputPluginUpdateProgress(ctx);
> >
> > + /*
> > + * If the BEGIN PREPARE was not yet sent, then it means there were no
> > + * relevant changes encountered, so we can skip the COMMIT PREPARED
> > + * message too.
> > + */
> > + if (txndata)
> > + {
> > + bool skip = !txndata->sent_begin_txn; pfree(txndata);
> > + txn->output_plugin_private = NULL;
> >
> > How is this supposed to work after the restart when prepared is sent
> > before the restart and we are just sending commit_prepared after
> > restart? Won't this lead to sending commit_prepared even when the
> > corresponding prepare is not sent? Can we think of a better way to
> > deal with this?
> >
>
> I have tried to resolve this by adding logic in worker,c to silently ignore
> spurious
> commit_prepareds. But this change required checking if the prepare exists on
> the subscriber before attempting the commit_prepared but the current API that
> checks this requires prepare time and transaction end_lsn. But for this I had
> to
> change the protocol of commit_prepared, and I understand that this would
> break backward compatibility between subscriber and publisher (you have
> raised this issue as well).
> I am not sure how else to handle this, let me know if you have any other
> ideas.
I feel if we don't want to change the protocol of commit_prepared,
we need to make the publisher solely judge whether the prepare was empty or not,
after the restart.
One idea I thought at the beginning was to utilize and apply
the existing mechanism to spill ReorderBufferSerializeTXN object to local disk,
by postponing the prepare txn object cleanup and when the walsender exits
and commit prepared didn't come, spilling the transaction's data,
then restoring it after the restart in the DecodePrepare.
However, this idea wasn't crash-safe fundamentally. It means,
if the publisher crashes before spilling the empty prepare transaction,
we fail to detect the prepare was empty and come down to send the
commit_prepared
in the situation where the subscriber didn't get the prepare data again.
So, I thought to utilize the spill mechanism didn't work for this purpose.
Another idea would be, to create an empty file under the the
pg_replslot/slotname
with a prefix different from "xid" in the DecodePrepare before the shutdown
if the prepare was empty, and bypass the cleanup of the serialized txns
and check the existence after the restart. But, this is pretty ad-hoc and I
wasn't sure
if to address the corner case of the restart has the strong enough justification
to create this new file format.
Therefore, in my humble opinion, the idea of protocol change slightly wins,
since the impact of the protocol change would not be big. We introduced
the protocol version 3 in the devel version and the number of users should be
little.
Best Regards,
Takamichi Osumi
RE: Optionally automatically disable logical replication subscriptions on error
On Monday, February 14, 2022 8:58 PM Amit Kapila
wrote:
> On Thu, Jan 6, 2022 at 11:23 AM osumi.takami...@fujitsu.com
> wrote:
> >
> > Kindly have a look at the attached v16.
> >
>
> Few comments:
Hi, thank you for checking the patch !
> =
> 1.
> @@ -3594,13 +3698,29 @@ ApplyWorkerMain(Datum main_arg)
> apply_error_callback_arg.command,
> apply_error_callback_arg.remote_xid,
> errdata->message);
> - MemoryContextSwitchTo(ecxt);
> +
> + if (!MySubscription->disableonerr)
> + {
> + /*
> + * Some work in error recovery work is done. Switch to the old
> + * memory context and rethrow.
> + */
> + MemoryContextSwitchTo(ecxt);
> + PG_RE_THROW();
> + }
> }
> + else if (!MySubscription->disableonerr) PG_RE_THROW();
>
> - PG_RE_THROW();
>
> Can't we combine these two different checks for
> 'MySubscription->disableonerr' if you do it as a separate if check after
> sending
> the stats message?
No, we can't. The second check of MySubscription->disableonerr is for the case
apply_error_callback_arg.command equals 0. We disable the subscription
on any errors. In other words, we need to rethrow the error in the case,
if the flag disableonerr is not set to true.
So, moving it to after sending
the stats message can't be done. At the same time, if we move
the disableonerr flag check outside of the apply_error_callback_arg.command
condition
branch, we need to write another call of pgstat_report_subworker_error, with the
same arguments that we have now. This wouldn't be preferrable as well.
>
> 2. Can we move the code related to tablesync worker and its error handing (the
> code insider if (am_tablesync_worker())) to a separate function say
> LogicalRepHandleTableSync() or something like that.
>
> 3. Similarly, we can move apply-loop related code ("Run the main
> loop.") to a separate function say LogicalRepHandleApplyMessages().
>
> If we do (2) and (3), I think the code in ApplyWorkerMain will look better.
> What
> do you think?
I agree with (2) and (3), since those contribute to better readability.
Attached a new patch v17 that addresses those refactorings.
Best Regards,
Takamichi Osumi
v17-0001-Optionally-disable-subscriptions-on-error.patch
Description: v17-0001-Optionally-disable-subscriptions-on-error.patch
RE: logical replication empty transactions
Hi, Thank you for your updating the patch. I'll quote one of the past discussions in order to make this thread go forward or more active. On Friday, August 13, 2021 8:01 PM Ajin Cherian wrote: > On Mon, Aug 2, 2021 at 7:20 PM Amit Kapila > wrote: > > > > On Fri, Jul 23, 2021 at 3:39 PM Ajin Cherian wrote: > > > > > > > Let's first split the patch for prepared and non-prepared cases as > > that will help to focus on each of them separately. BTW, why haven't > > you considered implementing point 1b as explained by Andres in his > > email [1]? I think we can send a keepalive message in case of > > synchronous replication when we skip an empty transaction, otherwise, > > it might delay in responding to transactions synchronous_commit mode. > > I think in the tests done in the thread, it might not have been shown > > because we are already sending keepalives too frequently. But what if > > someone disables wal_sender_timeout or kept it to a very large value? > > See WalSndKeepaliveIfNecessary. The other thing you might want to look > > at is if the reason for frequent keepalives is the same as described > > in the email [2]. > > > > I have tried to address the comment here by modifying the > ctx->update_progress callback function (WalSndUpdateProgress) provided > for plugins. I have added an option > by which the callback can specify if it wants to send keep_alives. And when > the callback is called with that option set, walsender updates a flag > force_keep_alive_syncrep. > The Walsender in the WalSndWaitForWal for loop, checks this flag and if > synchronous replication is enabled, then sends a keep alive. > Currently this logic > is added as an else to the current logic that is already there in > WalSndWaitForWal, which is probably considered unnecessary and a source of > the keep alive flood that you talked about. So, I can change that according to > how that fix shapes up there. I have also added an extern function in > syncrep.c > that makes it possible for walsender to query if synchronous replication is > turned on. Changing the timing to send the keepalive to the decoding commit timing didn't look impossible to me, although my suggestion can be ad-hoc. After the initialization of sentPtr(by confirmed_flush lsn), sentPtr is updated from logical_decoding_ctx->reader->EndRecPtr in XLogSendLogical. In the XLogSendLogical, we update it after we execute LogicalDecodingProcessRecord. This order leads to the current implementation to wait the next iteration to send a keepalive in WalSndWaitForWal. But, I felt we can utilize end_lsn passed to ReorderBufferCommit for updating sentPtr. The end_lsn is the lsn same as the ctx->reader->EndRecPtr, which means advancing the timing to update the sentPtr for the commit case. Then if the transaction is empty in synchronous mode, send the keepalive in WalSndUpdateProgress directly, instead of having the force_keepalive_syncrep flag and having it true. Best Regards, Takamichi Osumi
RE: logical replication empty transactions
On Thursday, January 27, 2022 9:57 PM Ajin Cherian wrote:
Hi, thanks for your patch update.
> On Wed, Jan 26, 2022 at 8:33 PM osumi.takami...@fujitsu.com
> wrote:
> >
> > On Tuesday, January 11, 2022 6:43 PM Ajin Cherian
> wrote:
> > (3) Is this patch's reponsibility to intialize the data in
> pgoutput_begin_prepare_txn ?
> >
> > @@ -433,6 +487,8 @@ static void
> > pgoutput_begin_prepare_txn(LogicalDecodingContext *ctx,
> > ReorderBufferTXN *txn) {
> > boolsend_replication_origin = txn->origin_id !=
> InvalidRepOriginId;
> > + PGOutputTxnData*txndata =
> MemoryContextAllocZero(ctx->context,
> > +
> > + sizeof(PGOutputTxnData));
> >
> > OutputPluginPrepareWrite(ctx, !send_replication_origin);
> > logicalrep_write_begin_prepare(ctx->out, txn);
> >
> >
> > Even if we need this initialization for either non streaming case or
> > non two_phase case, there can be another issue.
> > We don't free the allocated memory for this data, right ?
> > There's only one place to use free in the entire patch, which is in
> > the pgoutput_commit_txn(). So, corresponding free of memory looked
> > necessary in the two phase commit functions.
> >
>
> Actually it is required for begin_prepare to set the data type, so that the
> checks
> in the pgoutput_change can make sure that the begin prepare is sent. I've also
> added a free in commit_prepared code.
Okay, but if we choose the design that this patch takes
care of the initialization in pgoutput_begin_prepare_txn(),
we need another free in pgoutput_rollback_prepared_txn().
Could you please add some codes similar to pgoutput_commit_prepared_txn() to
the same ?
If we simply execute rollback prepared for non streaming transaction,
we don't free it.
Some other new minor comments.
(a) can be "synchronous replication", instead of "Synchronous Replication"
When we have a look at the syncrep.c, we use the former usually in
a normal comment.
/*
+ * Check if Synchronous Replication is enabled
+ */
(b) move below pgoutput_truncate two codes to the case where if nrelids > 0.
@@ -770,6 +850,7 @@ pgoutput_truncate(LogicalDecodingContext *ctx,
ReorderBufferTXN *txn,
int nrelations, Relation relations[],
ReorderBufferChange *change)
{
PGOutputData *data = (PGOutputData *) ctx->output_plugin_private;
+ PGOutputTxnData *txndata = (PGOutputTxnData *)
txn->output_plugin_private;
MemoryContext old;
RelationSyncEntry *relentry;
int i;
@@ -777,6 +858,9 @@ pgoutput_truncate(LogicalDecodingContext *ctx,
ReorderBufferTXN *txn,
Oid*relids;
TransactionId xid = InvalidTransactionId;
+ /* If not streaming, should have setup txndata as part of BEGIN/BEGIN
PREPARE */
+ Assert(in_streaming || txndata);
+
(c) fix indent with spaces (for the one sentence of SyncRepEnabled)
@@ -539,6 +538,15 @@ SyncRepReleaseWaiters(void)
}
/*
+ * Check if Synchronous Replication is enabled
+ */
+bool
+SyncRepEnabled(void)
+{
+return SyncRepRequested() && ((volatile WalSndCtlData *)
WalSndCtl)->sync_standbys_defined;
+}
+
+/*
This can be detected by git am.
Best Regards,
Takamichi Osumi
RE: logical replication empty transactions
On Tuesday, January 11, 2022 6:43 PM From: Ajin Cherian
wrote:
> Minor update to rebase the patch so that it applies clean on HEAD.
Hi, let me share some additional comments on v16.
(1) comment of pgoutput_change
@@ -630,11 +688,15 @@ pgoutput_change(LogicalDecodingContext *ctx,
ReorderBufferTXN *txn,
Relation relation, ReorderBufferChange *change)
{
PGOutputData *data = (PGOutputData *) ctx->output_plugin_private;
+ PGOutputTxnData *txndata = (PGOutputTxnData *)
txn->output_plugin_private;
MemoryContext old;
RelationSyncEntry *relentry;
TransactionId xid = InvalidTransactionId;
Relationancestor = NULL;
+ /* If not streaming, should have setup txndata as part of BEGIN/BEGIN
PREPARE */
+ Assert(in_streaming || txndata);
+
In my humble opinion, the comment should not touch BEGIN PREPARE,
because this patch's scope doesn't include two phase commit.
(We could add this in another patch to extend the scope after the commit ?)
This applies to pgoutput_truncate's comment.
(2) "keep alive" should be "keepalive" in WalSndUpdateProgress
/*
+* When skipping empty transactions in synchronous replication, we need
+* to send a keep alive to keep the MyWalSnd locations updated.
+*/
+ force_keepalive_syncrep = send_keepalive && SyncRepEnabled();
+
Also, this applies to the comment for force_keepalive_syncrep.
(3) Should finish the second sentence with period in the comment of
pgoutput_message.
@@ -845,6 +923,19 @@ pgoutput_message(LogicalDecodingContext *ctx,
ReorderBufferTXN *txn,
if (in_streaming)
xid = txn->xid;
+ /*
+* Output BEGIN if we haven't yet.
+* Avoid for streaming and non-transactional messages
(4) "begin" can be changed to "BEGIN" in the comment of PGOutputTxnData
definition.
In the entire patch, when we express BEGIN message,
we use capital letters "BEGIN" except for one place.
We can apply the same to this place as well.
+typedef struct PGOutputTxnData
+{
+ bool sent_begin_txn;/* flag indicating whether begin has been sent
*/
+} PGOutputTxnData;
+
(5) inconsistent way to write Assert statements with blank lines
In the below case, it'd be better to insert one blank line
after the Assert();
+static void
+pgoutput_begin(LogicalDecodingContext *ctx, ReorderBufferTXN *txn)
+{
boolsend_replication_origin = txn->origin_id !=
InvalidRepOriginId;
+ PGOutputTxnData *txndata = (PGOutputTxnData *)
txn->output_plugin_private;
+ Assert(txndata);
OutputPluginPrepareWrite(ctx, !send_replication_origin);
(6) new codes in the pgoutput_commit_txn looks messy slightly
@@ -419,7 +455,25 @@ static void
pgoutput_commit_txn(LogicalDecodingContext *ctx, ReorderBufferTXN *txn,
XLogRecPtr commit_lsn)
{
- OutputPluginUpdateProgress(ctx);
+ PGOutputTxnData *txndata = (PGOutputTxnData *)
txn->output_plugin_private;
+ boolskip;
+
+ Assert(txndata);
+
+ /*
+* If a BEGIN message was not yet sent, then it means there were no
relevant
+* changes encountered, so we can skip the COMMIT message too.
+*/
+ skip = !txndata->sent_begin_txn;
+ pfree(txndata);
+ txn->output_plugin_private = NULL;
+ OutputPluginUpdateProgress(ctx, skip);
Could we conduct a refactoring for this new part ?
IMO, writing codes to free the data structure at the top
of function seems weird.
One idea is to export some part there
and write a new function, something like below.
static bool
txn_sent_begin(ReorderBufferTXN *txn)
{
PGOutputTxnData *txndata = (PGOutputTxnData *) txn->output_plugin_private;
boolneeds_skip;
Assert(txndata);
needs_skip = !txndata->sent_begin_txn;
pfree(txndata);
txn->output_plugin_private = NULL;
return needs_skip;
}
FYI, I had a look at the
v12-0002-Skip-empty-prepared-transactions-for-logical-rep.patch
for reference of pgoutput_rollback_prepared_txn and
pgoutput_commit_prepared_txn.
Looks this kind of function might work for future extensions as well.
What did you think ?
Best Regards,
Takamichi Osumi
RE: logical replication empty transactions
On Tuesday, January 11, 2022 6:43 PM Ajin Cherian wrote:
> Minor update to rebase the patch so that it applies clean on HEAD.
Hi, thanks for you rebase.
Several comments.
(1) the commit message
"
transactions, keepalive messages are sent to keep the LSN locations updated
on the standby.
This patch does not skip empty transactions that are "streaming" or "two-phase".
"
I suggest that one blank line might be needed before the last paragraph.
(2) Could you please remove one pair of curly brackets for one sentence below ?
@@ -1546,10 +1557,13 @@ WalSndWaitForWal(XLogRecPtr loc)
* otherwise idle, this keepalive will trigger a reply.
Processing the
* reply will update these MyWalSnd locations.
*/
- if (MyWalSnd->flush < sentPtr &&
+ if (force_keepalive_syncrep ||
+ (MyWalSnd->flush < sentPtr &&
MyWalSnd->write < sentPtr &&
- !waiting_for_ping_response)
+ !waiting_for_ping_response))
+ {
WalSndKeepalive(false);
+ }
(3) Is this patch's reponsibility to intialize the data in
pgoutput_begin_prepare_txn ?
@@ -433,6 +487,8 @@ static void
pgoutput_begin_prepare_txn(LogicalDecodingContext *ctx, ReorderBufferTXN *txn)
{
boolsend_replication_origin = txn->origin_id !=
InvalidRepOriginId;
+ PGOutputTxnData*txndata = MemoryContextAllocZero(ctx->context,
+
sizeof(PGOutputTxnData));
OutputPluginPrepareWrite(ctx, !send_replication_origin);
logicalrep_write_begin_prepare(ctx->out, txn);
Even if we need this initialization for either non streaming case
or non two_phase case, there can be another issue.
We don't free the allocated memory for this data, right ?
There's only one place to use free in the entire patch,
which is in the pgoutput_commit_txn(). So,
corresponding free of memory looked necessary
in the two phase commit functions.
(4) SyncRepEnabled's better alignment.
IIUC, SyncRepEnabled is called not only by the walsender but also by other
backends
via CommitTransaction -> RecordTransactionCommit -> SyncRepWaitForLSN.
Then, the place to add the prototype function for SyncRepEnabled seems not
appropriate,
strictly speaking or requires a comment like /* called by wal sender or other
backends */.
@@ -90,6 +90,7 @@ extern void SyncRepCleanupAtProcExit(void);
/* called by wal sender */
extern void SyncRepInitConfig(void);
extern void SyncRepReleaseWaiters(void);
+extern bool SyncRepEnabled(void);
Even if we intend it is only used by the walsender, the current code place
of SyncRepEnabled in the syncrep.c might not be perfect.
In this file, seemingly we have a section for functions for wal sender processes
and the place where you wrote it is not here.
at src/backend/replication/syncrep.c, find a comment below.
/*
* ===
* Synchronous Replication functions for wal sender processes
* ===
*/
(5) minor alignment for expressing a couple of messages.
@@ -777,6 +846,9 @@ pgoutput_truncate(LogicalDecodingContext *ctx,
ReorderBufferTXN *txn,
Oid*relids;
TransactionId xid = InvalidTransactionId;
+ /* If not streaming, should have setup txndata as part of BEGIN/BEGIN
PREPARE */
+ Assert(in_streaming || txndata);
In the commit message, the way you write is below.
...
skip BEGIN / COMMIT messages for transactions that are empty. The patch
...
In this case, we have spaces back and forth for "BEGIN / COMMIT".
Then, I suggest to unify all of those to show better alignment.
Best Regards,
Takamichi Osumi
RE: Skipping logical replication transactions on subscriber side
On Friday, January 21, 2022 2:30 PM Amit Kapila wrote: > On Fri, Jan 21, 2022 at 10:32 AM osumi.takami...@fujitsu.com > wrote: > > > > On Friday, January 21, 2022 12:08 PM Masahiko Sawada > wrote: > > > I've attached an updated patch that incorporated these commends as > > > well as other comments I got so far. > > Thank you for your update ! > > > > Few minor comments. > > > > (1) trivial question > > > > For the users, > > was it perfectly clear that in the cascading logical replication > > setup, we can't selectively skip an arbitrary transaction of one upper > > nodes, without skipping its all executions on subsequent nodes, when > > we refer to the current doc description of v9 ? > > > > IIUC, this is because we don't write changes WAL either and can't > > propagate the contents to subsequent nodes. > > > > I tested this case and it didn't, as I expected. > > This can apply to other measures for conflicts, though. > > > > Right, there is nothing new as the user will same effect when she uses > existing > function pg_replication_origin_advance(). So, not sure if we want to add > something specific to this. Okay, thank you for clarifying this ! That's good to know. > > (3) minor question > > > > In the past, there was a discussion that it might be better if we > > reset the XID according to a change of subconninfo, which might be an > > opportunity to connect another publisher of a different XID space. > > Currently, we can regard it as user's responsibility. > > Was this correct ? > > > > I think if the user points to another publisher, doesn't it similarly needs to > change slot_name as well? If so, I think this can be treated in a similar way. I see. Then, in the AlterSubscription(), switching a slot_name doesn't affect other columns, which means this time, we don't need some special measure for this either as well, IIUC. Thanks ! Best Regards, Takamichi Osumi
RE: Skipping logical replication transactions on subscriber side
On Friday, January 21, 2022 12:08 PM Masahiko Sawada wrote: > I've attached an updated patch that incorporated these commends as well as > other comments I got so far. Thank you for your update ! Few minor comments. (1) trivial question For the users, was it perfectly clear that in the cascading logical replication setup, we can't selectively skip an arbitrary transaction of one upper nodes, without skipping its all executions on subsequent nodes, when we refer to the current doc description of v9 ? IIUC, this is because we don't write changes WAL either and can't propagate the contents to subsequent nodes. I tested this case and it didn't, as I expected. This can apply to other measures for conflicts, though. (2) suggestion There's no harm in writing a notification for a committer "Bump catalog version" in the commit log, as the patch changes the catalog. (3) minor question In the past, there was a discussion that it might be better if we reset the XID according to a change of subconninfo, which might be an opportunity to connect another publisher of a different XID space. Currently, we can regard it as user's responsibility. Was this correct ? Best Regards, Takamichi Osumi
RE: Skipping logical replication transactions on subscriber side
On Tuesday, January 18, 2022 3:05 PM Masahiko Sawada wrote: > I've attached a rebased patch. Thank you for your rebase ! Several review comments on v8. (1) doc/src/sgml/logical-replication.sgml + + + To resolve conflicts, you need to consider changing the data on the subscriber so + that it doesn't conflict with incoming changes, or dropping the conflicting constraint + or unique index, or writing a trigger on the subscriber to suppress or redirect + conflicting incoming changes, or as a last resort, by skipping the whole transaction. + Skipping the whole transaction includes skipping changes that may not violate + any constraint. This can easily make the subscriber inconsistent, especially if + a user specifies the wrong transaction ID or the position of origin. + The first sentence is too long and lack of readability slightly. One idea to sort out listing items is to utilize "itemizedlist". For instance, I imagined something like below. To resolve conflicts, you need to consider following actions: Change the data on the subscriber so that it doesn't conflict with incoming changes ... As a last resort, skip the whole transaction What did you think ? By the way, in case only when you want to keep the current sentence style, I have one more question. Do we need "by" in the part "by skipping the whole transaction" ? If we focus on only this action, I think the sentence becomes "you need to consider skipping the whole transaction". If this is true, we don't need "by" in the part. (2) Also, in the same paragraph, we write + ... This can easily make the subscriber inconsistent, especially if + a user specifies the wrong transaction ID or the position of origin. The subject of this sentence should be "Those" or "Some of those" ? because we want to mention either "new skip xid feature" or "pg_replication_origin_advance". (3) doc/src/sgml/ref/alter_subscription.sgml Below change contains unnecessary spaces. + the whole transaction. Using ALTER SUBSCRIPTION ... SKIP Need to change From: ALTER SUBSCRIPTION ... SKIP To: ALTER SUBSCRIPTION ... SKIP (4) comment in clear_subscription_skip_xid +* the flush position the transaction will be sent again and the user +* needs to be set subskipxid again. We can reduce the possibility by Shoud change From: the user needs to be set... To: the user needs to set... (5) clear_subscription_skip_xid + if (!HeapTupleIsValid(tup)) + elog(ERROR, "subscription \"%s\" does not exist", MySubscription->name); Can we change it to ereport with ERRCODE_UNDEFINED_OBJECT ? This suggestion has another aspect that in within one patch, we don't mix both ereport and elog at the same time. (6) apply_handle_stream_abort @@ -1209,6 +1300,13 @@ apply_handle_stream_abort(StringInfo s) logicalrep_read_stream_abort(s, , ); + /* +* We don't expect the user to set the XID of the transaction that is +* rolled back but if the skip XID is set, clear it. +*/ + if (MySubscription->skipxid == xid || MySubscription->skipxid == subxid) + clear_subscription_skip_xid(MySubscription->skipxid, InvalidXLogRecPtr, 0); + In my humble opinion, this still cares about subtransaction xid still. If we want to be consistent with top level transactions only, I felt checking MySubscription->skipxid == xid should be sufficient. Below is an *insame* (in a sense not correct usage) scenario to hit the "MySubscription->skipxid == subxid". Sorry if it is not perfect. --- Set logical_decoding_work_mem = 64. Create tables named 'tab' with a column id (integer); Create pub and sub with streaming = true. No initial data is required on both nodes because we just want to issue stream_abort after executing skip xid feature. to the publisher begin; select pg_current_xact_id(); -- for reference insert into tab values (1); savepoint s1; insert into tab values (2); savepoint s2; insert into tab values (generate_series(1001, 2000)); select ctid, xmin, xmax, id from tab where id in (1, 2, 1001); to the subscriber select subname, subskipxid from pg_subscription; -- shows 0 alter subscription mysub skip (xid = xxx); -- xxx is that of xmin for 1001 on the publisher select subname, subskipxid from pg_subscription; -- check it shows xxx just in case rollback to s1; commit; select * from tab; -- shows only data '1'. select subname, subskipxid from pg_subscription; -- shows 0. subskipxid was reset by the skip xid feature select count(1) = 1 from tab; -- shows true FYI: the commands result of those last two commands. postgres=# select subname, subskipxid from pg_subscription; subname | subskipxid -+ mysub | 0 (1 row) postgres=# select count(1) = 1 from tab; ?column? -- t (1 row) Thus, it still cares about
RE: Skipping logical replication transactions on subscriber side
On Tuesday, January 18, 2022 1:39 PM Masahiko Sawada wrote: > I've attached an updated patch. All comments I got so far were incorporated > into this patch unless I'm missing something. Hi, thank you for your new patch v7. For your information, I've encountered a failure to apply patch v7 on top of the latest commit (d3f4532) $ git am v7-0001-Add-ALTER-SUBSCRIPTION-.-SKIP-to-skip-the-transac.patch Applying: Add ALTER SUBSCRIPTION ... SKIP to skip the transaction on subscriber nodes error: patch failed: src/backend/parser/gram.y:9954 error: src/backend/parser/gram.y: patch does not apply Could you please rebase it when it's necessary ? Best Regards, Takamichi Osumi
