RE: [PATCH] Reuse Workers and Replication Slots during Logical Replication
On Thu, Jun 1, 2023 6:54 PM Melih Mutlu wrote:
>
> Hi,
>
> I rebased the patch and addressed the following reviews.
>
Thanks for updating the patch. Here are some comments on 0001 patch.
1.
- ereport(LOG,
- (errmsg("logical replication table synchronization
worker for subscription \"%s\", table \"%s\" has finished",
- MySubscription->name,
-
get_rel_name(MyLogicalRepWorker->relid;
Could we move this to somewhere else instead of removing it?
2.
+ if (!OidIsValid(originid))
+ originid = replorigin_create(originname);
+ replorigin_session_setup(originid, 0);
+ replorigin_session_origin = originid;
+ *origin_startpos = replorigin_session_get_progress(false);
+ CommitTransactionCommand();
+
+ /* Is the use of a password mandatory? */
+ must_use_password = MySubscription->passwordrequired &&
+ !superuser_arg(MySubscription->owner);
+ LogRepWorkerWalRcvConn = walrcv_connect(MySubscription->conninfo, true,
+
must_use_password,
+
MySubscription->name, );
It seems that there is a problem when refactoring.
See commit e7e7da2f8d5.
3.
+ /* Set this to false for safety, in case we're already reusing the
worker */
+ MyLogicalRepWorker->ready_to_reuse = false;
+
I am not sure do we need to lock when setting it.
4.
+ /*
+* Allocate the origin name in long-lived context for error context
+* message.
+*/
+ StartTransactionCommand();
+ ReplicationOriginNameForLogicalRep(MySubscription->oid,
+
MyLogicalRepWorker->relid,
+
originname,
+
originname_size);
+ CommitTransactionCommand();
Do we need the call to StartTransactionCommand() and CommitTransactionCommand()
here? Besides, the comment here is the same as the comment atop
set_apply_error_context_origin(), do we need it?
5.
I saw a segmentation fault when debugging.
It happened when calling sync_worker_exit() called (see the code below in
LogicalRepSyncTableStart()). In the case that this is not the first table the
worker synchronizes, clean_sync_worker() has been called before (in
TablesyncWorkerMain()), and LogRepWorkerWalRcvConn has been set to NULL. Then, a
segmentation fault happened because LogRepWorkerWalRcvConn is a null pointer.
switch (relstate)
{
case SUBREL_STATE_SYNCDONE:
case SUBREL_STATE_READY:
case SUBREL_STATE_UNKNOWN:
sync_worker_exit(); /* doesn't return */
}
Here is the backtrace.
#0 0x7fc8a8ce4c95 in libpqrcv_disconnect (conn=0x0) at
libpqwalreceiver.c:757
#1 0x0092b8c0 in clean_sync_worker () at tablesync.c:150
#2 0x0092b8ed in sync_worker_exit () at tablesync.c:164
#3 0x0092d8f6 in LogicalRepSyncTableStart
(origin_startpos=0x7ffd50f30f08) at tablesync.c:1293
#4 0x00934f76 in start_table_sync (origin_startpos=0x7ffd50f30f08,
myslotname=0x7ffd50f30e80) at worker.c:4457
#5 0x0093513b in run_tablesync_worker (options=0x7ffd50f30ec0,
slotname=0x0, originname=0x7ffd50f30f10 "pg_16394_16395",
originname_size=64, origin_startpos=0x7ffd50f30f08) at worker.c:4532
#6 0x00935a3a in TablesyncWorkerMain (main_arg=1) at worker.c:4853
#7 0x008e97f6 in StartBackgroundWorker () at bgworker.c:864
#8 0x008f350b in do_start_bgworker (rw=0x12fc1a0) at postmaster.c:5762
#9 0x008f38b7 in maybe_start_bgworkers () at postmaster.c:5986
#10 0x008f2975 in process_pm_pmsignal () at postmaster.c:5149
#11 0x008ee98a in ServerLoop () at postmaster.c:1770
#12 0x008ee3bb in PostmasterMain (argc=3, argv=0x12c4af0) at
postmaster.c:1463
#13 0x007b6d3a in main (argc=3, argv=0x12c4af0) at main.c:198
The steps to reproduce:
Worker1, in TablesyncWorkerMain(), the relstate of new table to sync (obtained
by GetSubscriptionRelations) is SUBREL_STATE_INIT, and in the foreach loop,
before the following Check (it needs a breakpoint before locking),
LWLockAcquire(LogicalRepWorkerLock, LW_EXCLUSIVE);
if (rstate->state != SUBREL_STATE_SYNCDONE &&
!logicalrep_worker_find(MySubscription->oid,
rstate->relid, false))
{
/* Update worker state for the next table */
MyLogicalRepWorker->relid = rstate->relid;
MyLogicalRepWorker->relstate
RE: Support logical replication of DDLs
On Wed, May 31, 2023 5:41 PM shveta malik wrote: > > On Mon, May 29, 2023 at 11:45 AM Yu Shi (Fujitsu) > wrote: > > > > 0008 patch > > - > > 4. > > case AT_AddColumn: > > /* XXX need to set the "recurse" bit > > somewhere? */ > > Assert(IsA(subcmd->def, ColumnDef)); > > - tree = deparse_ColumnDef(rel, dpcontext, > > false, > > - > > (ColumnDef *) subcmd->def, true, > ); > > > > mark_function_volatile(context, expr); > > > > After this change, `expr` is not assigned a value when > > mark_function_volatile is > called. > > > > Corrected. > It looks the call to mark_function_volatile() is removed in this case. I think we still need it, otherwise we won't know if the command contains a volatile function. (see check_command_publishable().) > > 5. > > create table p1(f1 int); > > create table p1_c1() inherits(p1); > > alter table p1 add constraint inh_check_constraint1 check (f1 > 0); > > alter table p1_c1 add constraint inh_check_constraint1 check (f1 > 0); > > > > The re-formed command of the last command is "ALTER TABLE public.p1_c1", > which > > seems to be wrong. > > > > Fixed, second alter-table should actually be no-op in terms of > deparsing. But when it is run without running the first alter-table > command, it should generate the reformed command. > If the second alter-table is no-op in terms of deparsing, the dumped result of origin command and that of re-formed command will be different. This seems to be because `conislocal` column of pg_constraint has different values. (After the second alter-table, its value is changed from false to true.) Regards, Shi Yu
RE: Support logical replication of DDLs
On Wed, May 31, 2023 5:41 PM shveta malik wrote:
>
> PFA the set of patches consisting above changes. All the changes are
> made in 0008 patch.
>
> Apart from above changes, many partition attach/detach related tests
> are uncommented in alter_table.sql in patch 0008.
>
Thanks for updating the patch, here are some comments.
1.
I think some code can be removed because it is not for supporting table
commands.
1.1
0001 patch, in deparse_RenameStmt().
OBJECT_ATTRIBUTE is type's attribute.
+ tmp_obj = new_objtree("CASCADE");
+ if (node->renameType != OBJECT_ATTRIBUTE ||
+ node->behavior != DROP_CASCADE)
+ append_not_present(tmp_obj);
+ append_object_object(ret, "cascade", tmp_obj);
1.2
0001 patch, in deparse_AlterRelation().
+ case AT_AddColumnToView:
+ /* CREATE OR REPLACE VIEW -- nothing to do here
*/
+ break;
1.3
0001 patch.
("alter table ... owner to ... " is deparsed in deparse_AlterRelation(), instead
of this funciton.)
+static ObjTree *
+deparse_AlterOwnerStmt(ObjectAddress address, Node *parsetree)
+{
+ AlterOwnerStmt *node = (AlterOwnerStmt *) parsetree;
+
+ return new_objtree_VA("ALTER %{objtype}s %{identity}s OWNER TO
%{newowner}I", 3,
+ "objtype", ObjTypeString,
+
stringify_objtype(node->objectType),
+ "identity", ObjTypeString,
+ getObjectIdentity(,
false),
+ "newowner", ObjTypeString,
+
get_rolespec_name(node->newowner));
+}
1.4
0001 patch, in deparse_AlterSeqStmt().
I think only "owned_by" option is needed, other options can't be generated by
"alter table" command.
+ foreach(cell, ((AlterSeqStmt *) parsetree)->options)
+ {
+ DefElem*elem = (DefElem *) lfirst(cell);
+ ObjElem*newelm;
+
+ if (strcmp(elem->defname, "cache") == 0)
+ newelm = deparse_Seq_Cache(seqform, false);
+ else if (strcmp(elem->defname, "cycle") == 0)
+ newelm = deparse_Seq_Cycle(seqform, false);
+ else if (strcmp(elem->defname, "increment") == 0)
+ newelm = deparse_Seq_IncrementBy(seqform, false);
+ else if (strcmp(elem->defname, "minvalue") == 0)
+ newelm = deparse_Seq_Minvalue(seqform, false);
+ else if (strcmp(elem->defname, "maxvalue") == 0)
+ newelm = deparse_Seq_Maxvalue(seqform, false);
+ else if (strcmp(elem->defname, "start") == 0)
+ newelm = deparse_Seq_Startwith(seqform, false);
+ else if (strcmp(elem->defname, "restart") == 0)
+ newelm = deparse_Seq_Restart(seqvalues->last_value);
+ else if (strcmp(elem->defname, "owned_by") == 0)
+ newelm = deparse_Seq_OwnedBy(objectId, false);
+ else if (strcmp(elem->defname, "as") == 0)
+ newelm = deparse_Seq_As(seqform);
+ else
+ elog(ERROR, "invalid sequence option %s",
elem->defname);
+
+ elems = lappend(elems, newelm);
+ }
2. 0001 patch, in deparse_AlterTableStmt(),
+ case AT_CookedColumnDefault:
+ {
+ Relationattrrel;
+ HeapTuple atttup;
+ Form_pg_attribute attStruct;
...
I think this case can be removed because it is for "create table like", and in
such case the function has returned before reaching here, see below.
+ /*
+* ALTER TABLE subcommands generated for TableLikeClause is processed in
+* the top level CREATE TABLE command; return empty here.
+*/
+ if (stmt->table_like)
+ return NULL;
3. 0001 patch, in deparse_AlterRelation().
Is there any case that "ALTER TABLE" command adds an index which is not a
constraint? If not, maybe we can remove the check or replace it with an assert.
+ case AT_AddIndex:
+ {
...
+
+ if (!istmt->isconstraint)
+ break;
4. To run this test when building with meson, it seems we should add
"test_ddl_deparse_regress" to src/test/modules/meson.build.
5.
create table t1 (a int);
create table t2 (a int);
SET allow_in_place_tablespaces = true;
CREATE TABLESPACE ddl_tblspace LOCATION '';
RESET allow_in_place_tablespaces;
ALTER TABLE ALL IN
RE: BF animal dikkop reported a failure in 035_standby_logical_decoding
On Monday, May 29, 2023 5:22 PM Drouvot, Bertrand
wrote:
>
> Hi,
>
> On 5/26/23 9:27 AM, Yu Shi (Fujitsu) wrote:
> > Hi hackers,
> >
> > I saw a buildfarm failure on "dikkop"[1]. It failed in
> > 035_standby_logical_decoding.pl, because the slots row_removal_inactiveslot
> and
> > row_removal_activeslot are not invalidated after vacuum.
>
> Thanks for sharing!
>
> >
> > regress_log_035_standby_logical_decoding:
> > ```
> > [12:15:05.943](4.442s) not ok 22 - inactiveslot slot invalidation is logged
> > with
> vacuum on pg_class
> > [12:15:05.945](0.003s)
> > [12:15:05.946](0.000s) # Failed test 'inactiveslot slot invalidation is
> > logged
> with vacuum on pg_class'
> > # at t/035_standby_logical_decoding.pl line 238.
> > [12:15:05.948](0.002s) not ok 23 - activeslot slot invalidation is logged
> > with
> vacuum on pg_class
> > [12:15:05.949](0.001s)
> > [12:15:05.950](0.000s) # Failed test 'activeslot slot invalidation is
> > logged with
> vacuum on pg_class'
> > # at t/035_standby_logical_decoding.pl line 244.
> > [13:38:26.977](5001.028s) # poll_query_until timed out executing this query:
> > # select (confl_active_logicalslot = 1) from pg_stat_database_conflicts
> > where
> datname = 'testdb'
> > # expecting this output:
> > # t
> > # last actual query output:
> > # f
> > # with stderr:
> > [13:38:26.980](0.003s) not ok 24 - confl_active_logicalslot updated
> > [13:38:26.982](0.002s)
> > [13:38:26.982](0.000s) # Failed test 'confl_active_logicalslot updated'
> > # at t/035_standby_logical_decoding.pl line 251.
> > Timed out waiting confl_active_logicalslot to be updated at
> t/035_standby_logical_decoding.pl line 251.
> > ```
> >
> > 035_standby_logical_decoding.pl:
> > ```
> > # This should trigger the conflict
> > $node_primary->safe_psql(
> > 'testdb', qq[
> >CREATE TABLE conflict_test(x integer, y text);
> >DROP TABLE conflict_test;
> >VACUUM pg_class;
> >INSERT INTO flush_wal DEFAULT VALUES; -- see create table flush_wal
> > ]);
> >
> > $node_primary->wait_for_replay_catchup($node_standby);
> >
> > # Check invalidation in the logfile and in pg_stat_database_conflicts
> > check_for_invalidation('row_removal_', $logstart, 'with vacuum on
> > pg_class');
> > ```
> >
> > Is it possible that the vacuum command didn't remove tuples and then the
> > conflict was not triggered?
>
> The flush_wal table added by Andres should guarantee that the WAL is flushed,
> so
> the only reason I can think about is indeed that the vacuum did not remove
> tuples (
> but I don't get why/how that could be the case).
>
> > It seems we can't confirm this because there is not
> > enough information.
>
> Right, and looking at its status history most of the time the test is green
> (making
> it
> even more difficult to diagnose).
>
> > Maybe "vacuum verbose" can be used to provide more
> > information.
>
> I can see that dikkop "belongs" to Tomas (adding Tomas to this thread).
> Tomas, do you think it would be possible to run some
> 035_standby_logical_decoding.pl
> manually with "vacuum verbose" in the test mentioned above? (or any other
> way you can think
> about that would help diagnose this random failure?).
>
Thanks for your reply.
I saw another failure on "drongo" [1], which looks like a similar problem.
Maybe a temporary patch can be committed to dump the result of "vacuum verbose".
And we can check this when the test fails.
[1]
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=drongo=2023-05-26%2018%3A05%3A57
Regards,
Shi Yu
RE: Support logical replication of DDLs
On Mon, May 22, 2023 1:57 PM shveta malik wrote:
>
> Please find the new set of patches for object-tree Removal. The new
> changes are in patch 0008 only. The new changes address object tree
> removal for below commands.
>
> create sequence
> alter sequence
> alter object owner to
> alter object set schema
> alter object rename
>
> In this patch 0008, ddldeparse.c is now object-tree free for all the
> table related commands. Index related commands are yet to be done.
>
Thanks for updating the patch. Here are some comments.
0001 patch
-
1.
+ colname = get_attname(ownerId, depform->refobjsubid, false);
+ if (colname == NULL)
+ continue;
missing_ok is false when calling get_attname(), so is there any case that
colname is NULL?
2.
+ case AT_SetStatistics:
+ {
+ Assert(IsA(subcmd->def, Integer));
+ if (subcmd->name)
+ tmp_obj = new_objtree_VA("ALTER
COLUMN %{column}I SET STATISTICS %{statistics}n", 3,
+
"type", ObjTypeString, "set statistics",
+
"column", ObjTypeString, subcmd->name,
+
"statistics", ObjTypeInteger,
+
intVal((Integer *) subcmd->def));
+ else
+ tmp_obj = new_objtree_VA("ALTER
COLUMN %{column}n SET STATISTICS %{statistics}n", 3,
+
"type", ObjTypeString, "set statistics",
+
"column", ObjTypeInteger, subcmd->num,
+
"statistics", ObjTypeInteger,
+
intVal((Integer *) subcmd->def));
+ subcmds = lappend(subcmds,
new_object_object(tmp_obj));
+ }
+ break;
I think subcmd->name will be NULL only if relation type is index. So should it
be removed because currently only table commands are supported?
0002 patch
-
3.
+ /* Skip adding constraint for inherits
table sub command */
+ if (!constrOid)
+ continue;
Would it be better to use OidIsValid() here?
0008 patch
-
4.
case AT_AddColumn:
/* XXX need to set the "recurse" bit somewhere?
*/
Assert(IsA(subcmd->def, ColumnDef));
- tree = deparse_ColumnDef(rel, dpcontext, false,
-
(ColumnDef *) subcmd->def, true, );
mark_function_volatile(context, expr);
After this change, `expr` is not assigned a value when mark_function_volatile
is called.
Some problems I saw :
-
5.
create table p1(f1 int);
create table p1_c1() inherits(p1);
alter table p1 add constraint inh_check_constraint1 check (f1 > 0);
alter table p1_c1 add constraint inh_check_constraint1 check (f1 > 0);
The re-formed command of the last command is "ALTER TABLE public.p1_c1", which
seems to be wrong.
6.
SET allow_in_place_tablespaces = true;
CREATE TABLESPACE ddl_tblspace LOCATION '';
RESET allow_in_place_tablespaces;
CREATE TABLE tbl_index_tblspe (a int, PRIMARY KEY(a) USING INDEX TABLESPACE
ddl_tblspace) ;
The re-formed command of the last command seems incorrect:
CREATE TABLE public.tbl_index_tblspe (a pg_catalog.int4 STORAGE PLAIN ,
USING INDEX TABLESPACE ddl_tblspace)
7.
CREATE TABLE part2_with_multiple_storage_params(
id int,
name varchar
) WITH (autovacuum_enabled);
re-formed command: CREATE TABLE public.part2_with_multiple_storage_params (id
pg_catalog.int4 STORAGE PLAIN , name pg_catalog."varchar" STORAGE EXTENDED
COLLATE pg_catalog."default")WITH (vacuum_index_cleanup = 'on',
autovacuum_vacuum_scale_factor = '0.2', vacuum_truncate = 'true',
autovacuum_enabled = 'TRUE')
When the option is not specified, re-formed command used uppercase letters. The
reloptions column in pg_class of the original command is
"{autovacuum_enabled=true}", but that of the re-formed command is
"{autovacuum_enabled=TRUE}". I tried to add this case to
BF animal dikkop reported a failure in 035_standby_logical_decoding
Hi hackers,
I saw a buildfarm failure on "dikkop"[1]. It failed in
035_standby_logical_decoding.pl, because the slots row_removal_inactiveslot and
row_removal_activeslot are not invalidated after vacuum.
regress_log_035_standby_logical_decoding:
```
[12:15:05.943](4.442s) not ok 22 - inactiveslot slot invalidation is logged
with vacuum on pg_class
[12:15:05.945](0.003s)
[12:15:05.946](0.000s) # Failed test 'inactiveslot slot invalidation is
logged with vacuum on pg_class'
# at t/035_standby_logical_decoding.pl line 238.
[12:15:05.948](0.002s) not ok 23 - activeslot slot invalidation is logged with
vacuum on pg_class
[12:15:05.949](0.001s)
[12:15:05.950](0.000s) # Failed test 'activeslot slot invalidation is logged
with vacuum on pg_class'
# at t/035_standby_logical_decoding.pl line 244.
[13:38:26.977](5001.028s) # poll_query_until timed out executing this query:
# select (confl_active_logicalslot = 1) from pg_stat_database_conflicts where
datname = 'testdb'
# expecting this output:
# t
# last actual query output:
# f
# with stderr:
[13:38:26.980](0.003s) not ok 24 - confl_active_logicalslot updated
[13:38:26.982](0.002s)
[13:38:26.982](0.000s) # Failed test 'confl_active_logicalslot updated'
# at t/035_standby_logical_decoding.pl line 251.
Timed out waiting confl_active_logicalslot to be updated at
t/035_standby_logical_decoding.pl line 251.
```
035_standby_logical_decoding.pl:
```
# This should trigger the conflict
$node_primary->safe_psql(
'testdb', qq[
CREATE TABLE conflict_test(x integer, y text);
DROP TABLE conflict_test;
VACUUM pg_class;
INSERT INTO flush_wal DEFAULT VALUES; -- see create table flush_wal
]);
$node_primary->wait_for_replay_catchup($node_standby);
# Check invalidation in the logfile and in pg_stat_database_conflicts
check_for_invalidation('row_removal_', $logstart, 'with vacuum on pg_class');
```
Is it possible that the vacuum command didn't remove tuples and then the
conflict was not triggered? It seems we can't confirm this because there is not
enough information. Maybe "vacuum verbose" can be used to provide more
information.
[1]
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=dikkop=2023-05-24%2006%3A16%3A18
Regards,
Shi Yu
RE: Fix a test case in 035_standby_logical_decoding.pl
On Thu, Apr 27, 2023 9:53 PM Amit Kapila wrote:
>
> On Thu, Apr 27, 2023 at 4:07 PM Drouvot, Bertrand
> wrote:
> >
> > On 4/27/23 11:53 AM, Amit Kapila wrote:
> > > On Thu, Apr 27, 2023 at 2:16 PM Drouvot, Bertrand
> > > wrote:
> > >>
> > >> On 4/27/23 10:11 AM, Yu Shi (Fujitsu) wrote:
> > >>> Hi hackers,
> > >>>
> > >>> In 035_standby_logical_decoding.pl, I think that the check in the
> > >>> following
> test
> > >>> case should be performed on the standby node, instead of the primary
> node, as
> > >>> the slot is created on the standby node.
> > >>
> > >> Oh right, the current test is not done on the right node, thanks!
> > >>
> > >>> The test currently passes because it
> > >>> only checks the return value of psql. It might be more appropriate to
> > >>> check
> the
> > >>> error message.
> > >>
> > >> Agree, and it's consistent with what is being done in
> 006_logical_decoding.pl.
> > >>
> > >>> Please see the attached patch.
> > >>>
> > >>
> > >> +
> > >> +($result, $stdout, $stderr) = $node_standby->psql(
> > >>'otherdb',
> > >>"SELECT lsn FROM
> pg_logical_slot_peek_changes('behaves_ok_activeslot', NULL, NULL) ORDER BY
> lsn DESC LIMIT 1;"
> > >> -),
> > >> -3,
> > >> -'replaying logical slot from another database fails');
> > >> +);
> > >> +ok( $stderr =~
> > >> + m/replication slot "behaves_ok_activeslot" was not created in
> > >> this
> database/,
> > >> + "replaying logical slot from another database fails");
> > >>
> > >>
> > >> That does look good to me.
> > >>
> > >
> > > I agree that that the check should be done on standby but how does it
> > > make a difference to check the error message or return value? Won't it
> > > be the same for both primary and standby?
> > >
> >
> > Yes that would be the same. I think the original idea from Shi-san (please
> correct me If I'm wrong)
> > was "while at it" let's also make this test on the right node even better.
> >
>
> Fair enough. Let''s do it that way then.
>
> > >> Nit: I wonder if while at it (as it was already there) we could not
> > >> remove the
> " ORDER BY lsn DESC LIMIT 1" part of it.
> > >> It does not change anything regarding the test but looks more appropriate
> to me.
> > >>
> > >
> > > It may not make a difference as this is anyway an error case but it
> > > looks more logical to LIMIT by 1 as you are fetching a single LSN
> > > value and it will be consistent with other tests in this file and the
> > > test case in the file 006_logical_decoding.pl.
> > >
> >
> > yeah I think it all depends how one see this test (sort of test a failure
> > or try to
> get
> > a result and see if it fails). That was a Nit so I don't have a strong
> > opinion on
> this one.
> >
>
> I feel let's be consistent here and keep it as it is.
>
> > > BTW, in the same test, I see it uses wait_for_catchup() in one place
> > > and wait_for_replay_catchup() in another place after Insert. Isn't it
> > > better to use wait_for_replay_catchup in both places?
> > >
> >
> > They are both using the 'replay' mode so both are perfectly correct but for
> consistency (and as
> > they don't use the same "target_lsn" ('write' vs 'flush')) I think that
> > using
> wait_for_replay_catchup()
> > in both places (which is using the 'flush' target lsn) is a good idea.
> >
>
> Yeah, let's use wait_for_replay_catchup() at both places.
>
Thanks for your reply. Your suggestions make sense to me.
Please see the attached patch.
Regards,
Shi Yu
v3-0001-Fix-a-test-case-in-035_standby_logical_decoding.p.patch
Description: v3-0001-Fix-a-test-case-in-035_standby_logical_decoding.p.patch
RE: Fix a test case in 035_standby_logical_decoding.pl
On Thu, Apr 27, 2023 4:47 PM Drouvot, Bertrand
wrote:
>
> Hi,
>
> On 4/27/23 10:11 AM, Yu Shi (Fujitsu) wrote:
> > Hi hackers,
> >
> > In 035_standby_logical_decoding.pl, I think that the check in the following
> > test
> > case should be performed on the standby node, instead of the primary node,
> as
> > the slot is created on the standby node.
>
> Oh right, the current test is not done on the right node, thanks!
>
> > The test currently passes because it
> > only checks the return value of psql. It might be more appropriate to check
> > the
> > error message.
>
> Agree, and it's consistent with what is being done in 006_logical_decoding.pl.
>
> > Please see the attached patch.
> >
>
> +
> +($result, $stdout, $stderr) = $node_standby->psql(
> 'otherdb',
> "SELECT lsn FROM
> pg_logical_slot_peek_changes('behaves_ok_activeslot',
> NULL, NULL) ORDER BY lsn DESC LIMIT 1;"
> -),
> -3,
> -'replaying logical slot from another database fails');
> +);
> +ok( $stderr =~
> + m/replication slot "behaves_ok_activeslot" was not created in this
> database/,
> + "replaying logical slot from another database fails");
>
>
> That does look good to me.
>
> Nit: I wonder if while at it (as it was already there) we could not remove
> the "
> ORDER BY lsn DESC LIMIT 1" part of it.
> It does not change anything regarding the test but looks more appropriate to
> me.
>
Thanks for your reply. I agree with you and I removed it in the attached patch.
Regards,
Shi Yu
v2-0001-Fix-a-test-case-in-035_standby_logical_decoding.p.patch
Description: v2-0001-Fix-a-test-case-in-035_standby_logical_decoding.p.patch
Fix a test case in 035_standby_logical_decoding.pl
Hi hackers,
In 035_standby_logical_decoding.pl, I think that the check in the following test
case should be performed on the standby node, instead of the primary node, as
the slot is created on the standby node. The test currently passes because it
only checks the return value of psql. It might be more appropriate to check the
error message. Please see the attached patch.
```
$node_primary->safe_psql('postgres', 'CREATE DATABASE otherdb');
is( $node_primary->psql(
'otherdb',
"SELECT lsn FROM pg_logical_slot_peek_changes('behaves_ok_activeslot',
NULL, NULL) ORDER BY lsn DESC LIMIT 1;"
),
3,
'replaying logical slot from another database fails');
```
The regress log:
psql::1: ERROR: replication slot "behaves_ok_activeslot" does not exist
[11:23:21.859](0.086s) ok 8 - replaying logical slot from another database fails
Regards,
Shi Yu
v1-0001-Fix-a-test-case-in-035_standby_logical_decoding.p.patch
Description: v1-0001-Fix-a-test-case-in-035_standby_logical_decoding.p.patch
RE: Add two missing tests in 035_standby_logical_decoding.pl
On Mon, Apr 24, 2023 8:07 PM Drouvot, Bertrand
wrote:
>
> On 4/24/23 11:45 AM, Amit Kapila wrote:
> > On Mon, Apr 24, 2023 at 11:54 AM Amit Kapila
> wrote:
> >>
> >> On Mon, Apr 24, 2023 at 11:24 AM Drouvot, Bertrand
> >> wrote:
> >>>
> >>
> >> Few comments:
> >>
> >>
> >
> > +# We can not test if the WAL file still exists immediately.
> > +# We need to let some time to the standby to actually "remove" it.
> > +my $i = 0;
> > +while (1)
> > +{
> > + last if !-f $standby_walfile;
> > + if ($i++ == 10 * $default_timeout)
> > + {
> > + die
> > + "could not determine if WAL file has been retained or not, can't
> > continue";
> > + }
> > + usleep(100_000);
> > +}
> >
> > Is this adhoc wait required because we can't guarantee that the
> > checkpoint is complete on standby even after using wait_for_catchup?
>
> Yes, the restart point on the standby is not necessary completed even after
> wait_for_catchup is done.
>
I think that's because when replaying a checkpoint record, the startup process
of standby only saves the information of the checkpoint, and we need to wait for
the checkpointer to perform a restartpoint (see RecoveryRestartPoint), right? If
so, could we force a checkpoint on standby? After this, the standby should have
completed the restartpoint and we don't need to wait.
Besides, would it be better to wait for the cascading standby? If the wal log
file needed for cascading standby is removed on the standby, the subsequent test
will fail. Do we need to consider this scenario? I saw the following error
message after setting recovery_min_apply_delay to 5s on the cascading standby,
and the test failed due to a timeout while waiting for cascading standby.
Log of cascading standby node:
FATAL: could not receive data from WAL stream: ERROR: requested WAL segment
00010003 has already been removed
Regards,
Shi Yu
RE: Test failures of 100_bugs.pl
On Fri, Apr 21, 2023 1:48 PM Yu Shi (Fujitsu) wrote: > > I wrote a patch to dump rel state in wait_for_subscription_sync() as > suggested. > Please see the attached patch. > I will try to add some debug logs in code later. > Please see the attached v2 patch. I added some debug logs when invalidating syncing table states and updating table_states_not_ready list. I also adjusted the message level in the tests which failed before. Regards, Shi Yu v2-0001-dump-rel-state-in-wait_for_subscription_sync.patch Description: v2-0001-dump-rel-state-in-wait_for_subscription_sync.patch v2-0002-Add-logs-to-help-investigate-subscription-test-fa.patch Description: v2-0002-Add-logs-to-help-investigate-subscription-test-fa.patch
RE: Test failures of 100_bugs.pl
On Tue, Jan 24, 2023 7:41 PM Amit Kapila wrote:
>
> On Tue, Jan 24, 2023 at 8:53 AM Andres Freund wrote:
> >
> > cfbot, the buildfarm and locally I have seen 100_bugs.pl fail
> > occasionally. Just rarely enough that I never got around to looking into it
> > for real.
> >
> ...
> >
> > We see t2 added to the publication:
> > 2023-01-24 00:57:30.099 UTC [73654][client backend] [100_bugs.pl][7/5:0]
> LOG: statement: ALTER PUBLICATION testpub ADD TABLE t2
> >
> > And that *then* "t" was synchronized:
> > 2023-01-24 00:57:30.102 UTC [73640][logical replication worker] LOG:
> > logical
> replication table synchronization worker for subscription "testsub", table
> "t" has
> finished
> >
> > and then that the refresh was issued:
> > 2023-01-24 00:57:30.128 UTC [73657][client backend] [100_bugs.pl][5/10:0]
> LOG: statement: ALTER SUBSCRIPTION testsub REFRESH PUBLICATION
> >
> > And we see a walsender starting and the query to get the new tables being
> executed:
> > 2023-01-24 00:57:30.139 UTC [73660][walsender] [testsub][6/8:0] LOG:
> statement: SELECT DISTINCT t.schemaname, t.tablename
> > , t.attnames
> > FROM pg_catalog.pg_publication_tables t
> > WHERE t.pubname IN ('testpub')
> >
> >
> > And that's it, the rest of the time is just polling.
> >
> >
> > Perhaps wait_for_subscription_sync() should dump the set of rel states to
> make
> > something like this more debuggable?
> >
> >
> > The fact that the synchronization for t finished just before the refresh
> > makes
> > me wonder if a wakeup or a cache invalidation got lost?
> >
>
> From the LOGs, the only thing one could draw is lost invalidation
> because the nap time of the apply worker is 1s, so it should process
> invalidation during the time we are polling. Also, the rel should be
> added to pg_subscription_rel because the test is still polling for
> rels to be in 'ready' or 'done' state.
>
> I think we can do three things to debug (a) as you suggest dump the
> rel state in wait_for_subscription_sync; (b) add some DEBUG log in
> invalidate_syncing_table_states() to ensure that invalidation has been
> processed; (c) print rel states and relids from table_states_not_ready
> in process_syncing_tables_for_apply() to see if t2 has ever appeared
> in that list.
>
There was a similar buildfarm failure on francolin recently[1]. I think the
problem is that after REFRESH PUBLICATION, the table sync worker for the new
table test_mismatching_types was not started. So, the test timed out while
waiting for an ERROR message that should have been reported by the table sync
worker.
--
regress_log_014_binary:
timed out waiting for match: (?^:ERROR: ( [A-Z0-9]+:)? incorrect binary data
format) at
/home/bf/bf-build/francolin/HEAD/pgsql/src/test/subscription/t/014_binary.pl
line 269.
014_binary_subscriber.log:
2023-04-16 18:18:38.455 UTC [3079482] 014_binary.pl LOG: statement: ALTER
SUBSCRIPTION tsub REFRESH PUBLICATION;
2023-04-16 18:21:39.219 UTC [3079474] ERROR: could not receive data from WAL
stream: server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
--
I wrote a patch to dump rel state in wait_for_subscription_sync() as suggested.
Please see the attached patch.
I will try to add some debug logs in code later.
[1]https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=francolin=2023-04-16%2018%3A17%3A09
Regards,
Shi Yu
v1-0001-dump-rel-state-in-wait_for_subscription_sync.patch
Description: v1-0001-dump-rel-state-in-wait_for_subscription_sync.patch
RE: Support logical replication of DDLs
On Fri, Apr 7, 2023 11:23 AM houzj.f...@fujitsu.com
wrote:
>
> On Friday, April 7, 2023 11:13 AM houzj.f...@fujitsu.com
>
> >
> > On Tuesday, April 4, 2023 7:35 PM shveta malik
> > wrote:
> > >
> > > On Tue, Apr 4, 2023 at 8:43 AM houzj.f...@fujitsu.com
> > > wrote:
> > >
> > > > Attach the new version patch set which did the following changes:
> > > >
> > >
> > > Hello,
> > >
> > > I tried below:
> > > pubnew=# ALTER PUBLICATION mypub2 SET (ddl = 'table'); ALTER
> > > PUBLICATION
> > >
> > > pubnew=# \dRp+
> > > Publication mypub2 Owner |
> > > All tables
> > > | All DDLs | Table DDLs |
> > > ++--++-
> > > shveta | t | f | f
> > > (1 row)
> > >
> > > I still see 'Table DDLs' as false and ddl replication did not work for
> > > this case.
> >
> > Thanks for reporting.
> >
> > Attach the new version patch which include the following changes:
> > * Fix the above bug for ALTER PUBLICATION SET.
> > * Modify the corresponding event trigger when user execute ALTER
> > PUBLICATION SET to change the ddl option.
> > * Fix a miss in pg_dump's code which causes CFbot failure.
> > * Rebase the patch due to recent commit 4826759.
>
> Sorry, there was a miss when rebasing the patch which could cause the
> CFbot to fail and here is the correct patch set.
>
Hi,
Thanks for your patch. Here are some comments.
1.
I saw a problem in the following case.
create type rewritetype as (a int);
alter type rewritetype add attribute b int cascade;
For the ALTER TYPE command, the deparse result is:
ALTER TYPE public.rewritetype ADD ATTRIBUTE b pg_catalog.int4 STORAGE plain
"STORAGE" is not supported for TYPE. Besides, "CASCADE" is missed.
I think that's because in deparse_AlterRelation(), we process ALTER TYPE ADD
ATTRIBUTE the same way as ALTER TABLE ADD COLUMN. It looks we need some
modification for ALTER TYPE.
2.
in 0001 patch
+ tmp_obj2 = new_objtree_VA("CASCADE", 1,
+
"present", ObjTypeBool, subcmd->behavior);
Would it be better to use "subcmd->behavior == DROP_CASCADE" here?
Regards,
Shi Yu
RE: PGDOCS - function pg_get_publication_tables is not documented?
On Fri, Mar 24, 2023 6:26 AM Tom Lane wrote: > > I do see a docs change that I think would be worth making: get > rid of the explicit mention of it in create_subscription.sgml > in favor of using that view. > I agree and I tried to modify the query to use the view. Please see the attached patch. Regards, Shi Yu v1-0001-Avoid-using-pg_get_publication_tables-in-create_s.patch Description: v1-0001-Avoid-using-pg_get_publication_tables-in-create_s.patch
