pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedSubscription tests fail under CLOBBER_CACHE_ALWAYS
28+ messages / 6 participants
[nested] [flat]
Commits from this thread:
2021-05-23 bc2a389efb Be more verbose when the postmaster unexpectedly quits.
2021-05-23 5b4791b458 b39630fd41 Fix access to no-longer-open relcache entry in logical-rep worker.
* Subscription tests fail under CLOBBER_CACHE_ALWAYS
@ 2021-05-18 23:42 Tom Lane <tgl@sss.pgh.pa.us>
2021-05-19 03:03 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Michael Paquier <michael@paquier.xyz>
2021-05-19 12:54 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Tomas Vondra <tomas.vondra@enterprisedb.com>
2021-05-21 22:14 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 3 replies; 28+ messages in thread
From: Tom Lane @ 2021-05-18 23:42 UTC (permalink / raw)
To: pgsql-hackers@lists.postgresql.org
I discovered $SUBJECT after wondering why hyrax hadn't reported
in recently, and trying to run check-world under CCA to see if
anything got stuck. Indeed it did --- although this doesn't
explain the radio silence from hyrax, because that animal doesn't
run any TAP tests. (Neither does avocet, which I think is the
only other active CCA critter. So this could have been broken
for a very long time.)
I count three distinct bugs that were exposed by this attempt:
1. In the part of 013_partition.pl that tests firing AFTER
triggers on partitioned tables, we have a case of continuing
to access a relcache entry that's already been closed.
(I'm not quite sure why prion's -DRELCACHE_FORCE_RELEASE
hasn't exposed this.) It looks to me like instead we had
a relcache reference leak before f3b141c48, but now, the
only relcache reference count on a partition child table
is dropped by ExecCleanupTupleRouting, which logical/worker.c
invokes before it fires triggers on that table. Kaboom.
This might go away if worker.c weren't so creatively different
from the other code paths concerned with executor shutdown.
2. Said bug causes a segfault in the apply worker process.
This causes the parent postmaster to give up and die.
I don't understand why we don't treat that like a crash
in a regular backend, considering that an apply worker
is running largely user-defined code.
3. Once the subscriber1 postmaster has exited, the TAP
test will eventually time out, and then this happens:
timed out waiting for catchup at t/013_partition.pl line 219.
### Stopping node "publisher" using mode immediate
# Running: pg_ctl -D /Users/tgl/pgsql/src/test/subscription/tmp_check/t_013_partition_publisher_data/pgdata -m immediate stop
waiting for server to shut down.... done
server stopped
# No postmaster PID for node "publisher"
### Stopping node "subscriber1" using mode immediate
# Running: pg_ctl -D /Users/tgl/pgsql/src/test/subscription/tmp_check/t_013_partition_subscriber1_data/pgdata -m immediate stop
pg_ctl: PID file "/Users/tgl/pgsql/src/test/subscription/tmp_check/t_013_partition_subscriber1_data/pgdata/postmaster.pid" does not exist
Is server running?
Bail out! system pg_ctl failed
That is, because we failed to shut down subscriber1, the
test script neglects to shut down subscriber2, and now
things just sit indefinitely. So that's a robustness
problem in the TAP infrastructure, rather than a bug in
PG proper; but I still say it's a bug that needs fixing.
regards, tom lane
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS
2021-05-18 23:42 Subscription tests fail under CLOBBER_CACHE_ALWAYS Tom Lane <tgl@sss.pgh.pa.us>
@ 2021-05-19 03:03 ` Michael Paquier <michael@paquier.xyz>
2021-05-19 03:32 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Amit Langote <amitlangote09@gmail.com>
2021-05-19 03:46 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Tom Lane <tgl@sss.pgh.pa.us>
2021-05-19 18:36 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Andrew Dunstan <andrew@dunslane.net>
2 siblings, 3 replies; 28+ messages in thread
From: Michael Paquier @ 2021-05-19 03:03 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: pgsql-hackers@lists.postgresql.org
On Tue, May 18, 2021 at 07:42:08PM -0400, Tom Lane wrote:
> I count three distinct bugs that were exposed by this attempt:
>
> 1. In the part of 013_partition.pl that tests firing AFTER
> triggers on partitioned tables, we have a case of continuing
> to access a relcache entry that's already been closed.
> (I'm not quite sure why prion's -DRELCACHE_FORCE_RELEASE
> hasn't exposed this.) It looks to me like instead we had
> a relcache reference leak before f3b141c48, but now, the
> only relcache reference count on a partition child table
> is dropped by ExecCleanupTupleRouting, which logical/worker.c
> invokes before it fires triggers on that table. Kaboom.
> This might go away if worker.c weren't so creatively different
> from the other code paths concerned with executor shutdown.
The tuple routing has made the whole worker logic messier by a larger
degree compared to when this stuff was only able to apply DMLs changes
on the partition leaves. I know that it is not that great to be more
creative here, but we need to make sure that AfterTriggerEndQuery() is
moved before ExecCleanupTupleRouting(). We could either keep the
ExecCleanupTupleRouting() calls as they are now, and call
AfterTriggerEndQuery() in more code paths. Or we could have one
PartitionTupleRouting and one ModifyTableState created beforehand
in create_estate_for_relation() if applying the change on a
partitioned table but that means manipulating more structures across
more layers of this code. Something like the attached fixes the
problem for me, but honestly it does not help in clarifying this code
more. I was not patient enough to wait for CLOBBER_CACHE_ALWAYS to
initialize the nodes in the TAP tests, so I have tested that with a
setup initialized with a non-clobber build, and reproduced the problem
with CLOBBER_CACHE_ALWAYS builds on those same nodes.
You are right that this is not a problem of 14~. I can reproduce the
problem on 13 as well, and we have no coverage for tuple routing with
triggers on this branch, so this would never have been stressed in the
buildfarm. There is a good argument to be made here in cherry-picking
2ecfeda3 to REL_13_STABLE.
> 2. Said bug causes a segfault in the apply worker process.
> This causes the parent postmaster to give up and die.
> I don't understand why we don't treat that like a crash
> in a regular backend, considering that an apply worker
> is running largely user-defined code.
CleanupBackgroundWorker() and CleanupBackend() have a lot of common
points. Are you referring to an inconsistent behavior with
restart_after_crash that gets ignored for bgworkers? We disable it by
default in the TAP tests.
> 3. Once the subscriber1 postmaster has exited, the TAP
> test will eventually time out, and then this happens:
>
> [.. logs ..]
>
> That is, because we failed to shut down subscriber1, the
> test script neglects to shut down subscriber2, and now
> things just sit indefinitely. So that's a robustness
> problem in the TAP infrastructure, rather than a bug in
> PG proper; but I still say it's a bug that needs fixing.
This one comes down to teardown_node() that uses system_or_bail(),
leaving things unfinished. I guess that we could be more aggressive
and ignore failures if we have a non-zero error code and that not all
the tests have passed within the END block of PostgresNode.pm.
--
Michael
Attachments:
logirep-partition-clobber.patch (text/x-diff, 2.0K) download
diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c
index 1432554d5a..01c1bd14f6 100644
--- a/src/backend/replication/logical/worker.c
+++ b/src/backend/replication/logical/worker.c
@@ -394,9 +394,6 @@ create_estate_for_relation(LogicalRepRelMapEntry *rel,
static void
finish_estate(EState *estate)
{
- /* Handle any queued AFTER triggers. */
- AfterTriggerEndQuery(estate);
-
/* Cleanup. */
ExecResetTupleTable(estate->es_tupleTable, false);
FreeExecutorState(estate);
@@ -1226,9 +1223,14 @@ apply_handle_insert(StringInfo s)
apply_handle_tuple_routing(resultRelInfo, estate,
remoteslot, NULL, rel, CMD_INSERT);
else
+ {
apply_handle_insert_internal(resultRelInfo, estate,
remoteslot);
+ /* Handle any queued AFTER triggers */
+ AfterTriggerEndQuery(estate);
+ }
+
PopActiveSnapshot();
finish_estate(estate);
@@ -1371,9 +1373,14 @@ apply_handle_update(StringInfo s)
apply_handle_tuple_routing(resultRelInfo, estate,
remoteslot, &newtup, rel, CMD_UPDATE);
else
+ {
apply_handle_update_internal(resultRelInfo, estate,
remoteslot, &newtup, rel);
+ /* Handle any queued AFTER triggers */
+ AfterTriggerEndQuery(estate);
+ }
+
PopActiveSnapshot();
finish_estate(estate);
@@ -1494,9 +1501,14 @@ apply_handle_delete(StringInfo s)
apply_handle_tuple_routing(resultRelInfo, estate,
remoteslot, NULL, rel, CMD_DELETE);
else
+ {
apply_handle_delete_internal(resultRelInfo, estate,
remoteslot, &rel->remoterel);
+ /* Handle any queued AFTER triggers */
+ AfterTriggerEndQuery(estate);
+ }
+
PopActiveSnapshot();
finish_estate(estate);
@@ -1798,6 +1810,12 @@ apply_handle_tuple_routing(ResultRelInfo *relinfo,
break;
}
+ /*
+ * Handle any queued AFTER triggers. Note that this needs to be done
+ * before closing any references to the relation used for tuple routing.
+ */
+ AfterTriggerEndQuery(estate);
+
ExecCleanupTupleRouting(mtstate, proute);
}
* Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS
2021-05-18 23:42 Subscription tests fail under CLOBBER_CACHE_ALWAYS Tom Lane <tgl@sss.pgh.pa.us>
2021-05-19 03:03 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Michael Paquier <michael@paquier.xyz>
@ 2021-05-19 03:32 ` Amit Langote <amitlangote09@gmail.com>
2 siblings, 0 replies; 28+ messages in thread
From: Amit Langote @ 2021-05-19 03:32 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Wed, May 19, 2021 at 12:04 PM Michael Paquier <michael@paquier.xyz> wrote:
> On Tue, May 18, 2021 at 07:42:08PM -0400, Tom Lane wrote:
> > I count three distinct bugs that were exposed by this attempt:
> >
> > 1. In the part of 013_partition.pl that tests firing AFTER
> > triggers on partitioned tables, we have a case of continuing
> > to access a relcache entry that's already been closed.
> > (I'm not quite sure why prion's -DRELCACHE_FORCE_RELEASE
> > hasn't exposed this.) It looks to me like instead we had
> > a relcache reference leak before f3b141c48, but now, the
> > only relcache reference count on a partition child table
> > is dropped by ExecCleanupTupleRouting, which logical/worker.c
> > invokes before it fires triggers on that table. Kaboom.
Oops.
> > This might go away if worker.c weren't so creatively different
> > from the other code paths concerned with executor shutdown.
>
> The tuple routing has made the whole worker logic messier by a larger
> degree compared to when this stuff was only able to apply DMLs changes
> on the partition leaves. I know that it is not that great to be more
> creative here, but we need to make sure that AfterTriggerEndQuery() is
> moved before ExecCleanupTupleRouting(). We could either keep the
> ExecCleanupTupleRouting() calls as they are now, and call
> AfterTriggerEndQuery() in more code paths.
Yeah, that's what I thought to propose doing too. Your patch looks
enough in that regard. Thanks for writing it.
> Or we could have one
> PartitionTupleRouting and one ModifyTableState created beforehand
> in create_estate_for_relation() if applying the change on a
> partitioned table but that means manipulating more structures across
> more layers of this code.
Yeah, that seems like too much change to me too.
> Something like the attached fixes the
> problem for me, but honestly it does not help in clarifying this code
> more. I was not patient enough to wait for CLOBBER_CACHE_ALWAYS to
> initialize the nodes in the TAP tests, so I have tested that with a
> setup initialized with a non-clobber build, and reproduced the problem
> with CLOBBER_CACHE_ALWAYS builds on those same nodes.
I have checked the fix works with a CLOBBER_CACHE_ALWAYS build.
> You are right that this is not a problem of 14~. I can reproduce the
> problem on 13 as well, and we have no coverage for tuple routing with
> triggers on this branch, so this would never have been stressed in the
> buildfarm. There is a good argument to be made here in cherry-picking
> 2ecfeda3 to REL_13_STABLE.
+1
--
Amit Langote
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS
2021-05-18 23:42 Subscription tests fail under CLOBBER_CACHE_ALWAYS Tom Lane <tgl@sss.pgh.pa.us>
2021-05-19 03:03 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Michael Paquier <michael@paquier.xyz>
@ 2021-05-19 03:46 ` Tom Lane <tgl@sss.pgh.pa.us>
2021-05-19 04:23 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Michael Paquier <michael@paquier.xyz>
2 siblings, 1 reply; 28+ messages in thread
From: Tom Lane @ 2021-05-19 03:46 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: pgsql-hackers@lists.postgresql.org
Michael Paquier <michael@paquier.xyz> writes:
> On Tue, May 18, 2021 at 07:42:08PM -0400, Tom Lane wrote:
>> This might go away if worker.c weren't so creatively different
>> from the other code paths concerned with executor shutdown.
> The tuple routing has made the whole worker logic messier by a larger
> degree compared to when this stuff was only able to apply DMLs changes
> on the partition leaves. I know that it is not that great to be more
> creative here, but we need to make sure that AfterTriggerEndQuery() is
> moved before ExecCleanupTupleRouting(). We could either keep the
> ExecCleanupTupleRouting() calls as they are now, and call
> AfterTriggerEndQuery() in more code paths. Or we could have one
> PartitionTupleRouting and one ModifyTableState created beforehand
> in create_estate_for_relation() if applying the change on a
> partitioned table but that means manipulating more structures across
> more layers of this code. Something like the attached fixes the
> problem for me, but honestly it does not help in clarifying this code
> more.
I was wondering if we could move the ExecCleanupTupleRouting call
into finish_estate. copyfrom.c, for example, does that during
its shutdown function. Compare also the worker.c changes proposed
in
https://www.postgresql.org/message-id/3362608.1621367104%40sss.pgh.pa.us
which are because I discovered it's unsafe to pop the snapshot
before running AfterTriggerEndQuery.
regards, tom lane
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS
2021-05-18 23:42 Subscription tests fail under CLOBBER_CACHE_ALWAYS Tom Lane <tgl@sss.pgh.pa.us>
2021-05-19 03:03 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Michael Paquier <michael@paquier.xyz>
2021-05-19 03:46 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Tom Lane <tgl@sss.pgh.pa.us>
@ 2021-05-19 04:23 ` Michael Paquier <michael@paquier.xyz>
2021-05-19 04:56 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Amit Kapila <amit.kapila16@gmail.com>
0 siblings, 1 reply; 28+ messages in thread
From: Michael Paquier @ 2021-05-19 04:23 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: pgsql-hackers@lists.postgresql.org
On Tue, May 18, 2021 at 11:46:25PM -0400, Tom Lane wrote:
> I was wondering if we could move the ExecCleanupTupleRouting call
> into finish_estate. copyfrom.c, for example, does that during
> its shutdown function. Compare also the worker.c changes proposed
> in
Yeah, the first patch I wrote for this thread was pushing out
PopActiveSnapshot() into the finish() routine, but I really found the
creation of the ModifyTableState stuff needed for a partitioned table
done in create_estate_for_relation() to make the code more confusing,
as that's only a piece needed for the tuple routing path. Saying
that, minimizing calls to PopActiveSnapshot() and PushActiveSnapshot()
is an improvement. That's why I settled into more calls to
AfterTriggerEndQuery() in the 4 code paths of any apply (tuple routing
+ 3 DMLs).
> https://www.postgresql.org/message-id/3362608.1621367104%40sss.pgh.pa.us
>
> which are because I discovered it's unsafe to pop the snapshot
> before running AfterTriggerEndQuery.
Didn't remember this one. This reminds me of something similar I did
a couple of weeks ago for the worker code, similar to what you have
here. Moving the snapshot push to be earlier, as your other patch is
doing, was bringing a bit more sanity when it came to opening the
indexes of the relation on which a change is applied as we need an
active snapshot for predicates and expressions (aka ExecOpenIndices
and ExecCloseIndices).
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS
2021-05-18 23:42 Subscription tests fail under CLOBBER_CACHE_ALWAYS Tom Lane <tgl@sss.pgh.pa.us>
2021-05-19 03:03 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Michael Paquier <michael@paquier.xyz>
2021-05-19 03:46 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Tom Lane <tgl@sss.pgh.pa.us>
2021-05-19 04:23 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Michael Paquier <michael@paquier.xyz>
@ 2021-05-19 04:56 ` Amit Kapila <amit.kapila16@gmail.com>
2021-05-19 05:05 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 28+ messages in thread
From: Amit Kapila @ 2021-05-19 04:56 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Wed, May 19, 2021 at 9:54 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Tue, May 18, 2021 at 11:46:25PM -0400, Tom Lane wrote:
> > I was wondering if we could move the ExecCleanupTupleRouting call
> > into finish_estate. copyfrom.c, for example, does that during
> > its shutdown function. Compare also the worker.c changes proposed
> > in
>
> Yeah, the first patch I wrote for this thread was pushing out
> PopActiveSnapshot() into the finish() routine, but I really found the
> creation of the ModifyTableState stuff needed for a partitioned table
> done in create_estate_for_relation() to make the code more confusing,
> as that's only a piece needed for the tuple routing path.
>
How about moving AfterTriggerEndQuery() to apply_handle_*_internal
calls? That way, we might not even need to change Push/Pop calls.
--
With Regards,
Amit Kapila.
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS
2021-05-18 23:42 Subscription tests fail under CLOBBER_CACHE_ALWAYS Tom Lane <tgl@sss.pgh.pa.us>
2021-05-19 03:03 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Michael Paquier <michael@paquier.xyz>
2021-05-19 03:46 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Tom Lane <tgl@sss.pgh.pa.us>
2021-05-19 04:23 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Michael Paquier <michael@paquier.xyz>
2021-05-19 04:56 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Amit Kapila <amit.kapila16@gmail.com>
@ 2021-05-19 05:05 ` Michael Paquier <michael@paquier.xyz>
2021-05-19 05:21 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Amit Kapila <amit.kapila16@gmail.com>
2021-05-19 06:53 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Amit Langote <amitlangote09@gmail.com>
0 siblings, 2 replies; 28+ messages in thread
From: Michael Paquier @ 2021-05-19 05:05 UTC (permalink / raw)
To: Amit Kapila <amit.kapila16@gmail.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Wed, May 19, 2021 at 10:26:28AM +0530, Amit Kapila wrote:
> How about moving AfterTriggerEndQuery() to apply_handle_*_internal
> calls? That way, we might not even need to change Push/Pop calls.
Isn't that going to be a problem when a tuple is moved to a new
partition in the tuple routing? This does a DELETE followed by an
INSERT, but the operation is an UPDATE.
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS
2021-05-18 23:42 Subscription tests fail under CLOBBER_CACHE_ALWAYS Tom Lane <tgl@sss.pgh.pa.us>
2021-05-19 03:03 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Michael Paquier <michael@paquier.xyz>
2021-05-19 03:46 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Tom Lane <tgl@sss.pgh.pa.us>
2021-05-19 04:23 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Michael Paquier <michael@paquier.xyz>
2021-05-19 04:56 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Amit Kapila <amit.kapila16@gmail.com>
2021-05-19 05:05 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Michael Paquier <michael@paquier.xyz>
@ 2021-05-19 05:21 ` Amit Kapila <amit.kapila16@gmail.com>
1 sibling, 0 replies; 28+ messages in thread
From: Amit Kapila @ 2021-05-19 05:21 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Wed, May 19, 2021 at 10:35 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Wed, May 19, 2021 at 10:26:28AM +0530, Amit Kapila wrote:
> > How about moving AfterTriggerEndQuery() to apply_handle_*_internal
> > calls? That way, we might not even need to change Push/Pop calls.
>
> Isn't that going to be a problem when a tuple is moved to a new
> partition in the tuple routing?
>
Right, it won't work.
--
With Regards,
Amit Kapila.
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS
2021-05-18 23:42 Subscription tests fail under CLOBBER_CACHE_ALWAYS Tom Lane <tgl@sss.pgh.pa.us>
2021-05-19 03:03 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Michael Paquier <michael@paquier.xyz>
2021-05-19 03:46 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Tom Lane <tgl@sss.pgh.pa.us>
2021-05-19 04:23 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Michael Paquier <michael@paquier.xyz>
2021-05-19 04:56 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Amit Kapila <amit.kapila16@gmail.com>
2021-05-19 05:05 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Michael Paquier <michael@paquier.xyz>
@ 2021-05-19 06:53 ` Amit Langote <amitlangote09@gmail.com>
2021-05-19 20:23 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Tom Lane <tgl@sss.pgh.pa.us>
1 sibling, 1 reply; 28+ messages in thread
From: Amit Langote @ 2021-05-19 06:53 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Amit Kapila <amit.kapila16@gmail.com>; Tom Lane <tgl@sss.pgh.pa.us>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Wed, May 19, 2021 at 2:05 PM Michael Paquier <michael@paquier.xyz> wrote:
> On Wed, May 19, 2021 at 10:26:28AM +0530, Amit Kapila wrote:
> > How about moving AfterTriggerEndQuery() to apply_handle_*_internal
> > calls? That way, we might not even need to change Push/Pop calls.
>
> Isn't that going to be a problem when a tuple is moved to a new
> partition in the tuple routing? This does a DELETE followed by an
> INSERT, but the operation is an UPDATE.
That indeed doesn't work. Once AfterTriggerEndQuery() would get
called for DELETE from apply_handle_delete_internal(), after triggers
of the subsequent INSERT can't be processed, instead causing:
ERROR: AfterTriggerSaveEvent() called outside of query
IOW, the patch you posted earlier seems like the way to go.
--
Amit Langote
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS
2021-05-18 23:42 Subscription tests fail under CLOBBER_CACHE_ALWAYS Tom Lane <tgl@sss.pgh.pa.us>
2021-05-19 03:03 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Michael Paquier <michael@paquier.xyz>
2021-05-19 03:46 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Tom Lane <tgl@sss.pgh.pa.us>
2021-05-19 04:23 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Michael Paquier <michael@paquier.xyz>
2021-05-19 04:56 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Amit Kapila <amit.kapila16@gmail.com>
2021-05-19 05:05 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Michael Paquier <michael@paquier.xyz>
2021-05-19 06:53 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Amit Langote <amitlangote09@gmail.com>
@ 2021-05-19 20:23 ` Tom Lane <tgl@sss.pgh.pa.us>
2021-05-19 23:56 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Michael Paquier <michael@paquier.xyz>
2021-05-20 00:32 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Amit Langote <amitlangote09@gmail.com>
0 siblings, 2 replies; 28+ messages in thread
From: Tom Lane @ 2021-05-19 20:23 UTC (permalink / raw)
To: Amit Langote <amitlangote09@gmail.com>; +Cc: Michael Paquier <michael@paquier.xyz>; Amit Kapila <amit.kapila16@gmail.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Amit Langote <amitlangote09@gmail.com> writes:
> IOW, the patch you posted earlier seems like the way to go.
I really dislike that patch. I think it's doubling down on the messy,
unstructured coding patterns that got us into this situation to begin
with. I'd prefer to expend a little effort on refactoring so that
the ExecCleanupTupleRouting call can be moved to the cleanup function
where it belongs.
So, I propose the attached, which invents a new struct to carry
the stuff we've discovered to be necessary. This makes the APIs
noticeably cleaner IMHO.
I did not touch the APIs of the apply_XXX_internal functions,
as it didn't really seem to offer any notational advantage.
We can't simply collapse them to take an "edata" as I did for
apply_handle_tuple_routing, because the ResultRelInfo they're
supposed to operate on could be different from the original one.
I considered a couple of alternatives:
* Replace their estate arguments with edata, but keep the separate
ResultRelInfo arguments. This might be worth doing in future, if we
add more fields to ApplyExecutionData. Right now it'd save nothing,
and it'd create a risk of confusion about when to use the
ResultRelInfo argument vs. edata->resultRelInfo.
* Allow apply_handle_tuple_routing to overwrite edata->resultRelInfo
with the partition child's RRI, then simplify the apply_XXX_internal
functions to take just edata instead of separate estate and
resultRelInfo args. I think this would work right now, but it seems
grotty, and it might cause problems in future.
* Replace the edata->resultRelInfo field with two fields, one for
the original parent and one for the actual/current target. Perhaps
this is worth doing, not sure.
Thoughts?
regards, tom lane
Attachments:
postpone-tuple-routing-cleanup.patch (text/x-diff, 9.4K) download
diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c
index 1432554d5a..c51a83f797 100644
--- a/src/backend/replication/logical/worker.c
+++ b/src/backend/replication/logical/worker.c
@@ -178,6 +178,14 @@ static HTAB *xidhash = NULL;
/* BufFile handle of the current streaming file */
static BufFile *stream_fd = NULL;
+typedef struct ApplyExecutionData
+{
+ EState *estate; /* always needed */
+ ResultRelInfo *resultRelInfo; /* always needed */
+ ModifyTableState *mtstate; /* used for partitioned target rel */
+ PartitionTupleRouting *proute; /* used for partitioned target rel */
+} ApplyExecutionData;
+
typedef struct SubXactInfo
{
TransactionId xid; /* XID of the subxact */
@@ -239,8 +247,7 @@ static bool FindReplTupleInLocalRel(EState *estate, Relation localrel,
LogicalRepRelation *remoterel,
TupleTableSlot *remoteslot,
TupleTableSlot **localslot);
-static void apply_handle_tuple_routing(ResultRelInfo *relinfo,
- EState *estate,
+static void apply_handle_tuple_routing(ApplyExecutionData *edata,
TupleTableSlot *remoteslot,
LogicalRepTupleData *newtup,
LogicalRepRelMapEntry *relmapentry,
@@ -336,20 +343,21 @@ handle_streamed_transaction(LogicalRepMsgType action, StringInfo s)
/*
* Executor state preparation for evaluation of constraint expressions,
- * indexes and triggers.
+ * indexes and triggers for the specified relation.
*
- * resultRelInfo is a ResultRelInfo for the relation to be passed to the
- * executor routines. The caller must open and close any indexes to be
- * updated independently of the relation registered here.
+ * Note that the caller must open and close any indexes to be updated.
*/
-static EState *
-create_estate_for_relation(LogicalRepRelMapEntry *rel,
- ResultRelInfo **resultRelInfo)
+static ApplyExecutionData *
+create_edata_for_relation(LogicalRepRelMapEntry *rel)
{
+ ApplyExecutionData *edata;
EState *estate;
RangeTblEntry *rte;
+ ResultRelInfo *resultRelInfo;
- estate = CreateExecutorState();
+ edata = (ApplyExecutionData *) palloc0(sizeof(ApplyExecutionData));
+
+ edata->estate = estate = CreateExecutorState();
rte = makeNode(RangeTblEntry);
rte->rtekind = RTE_RELATION;
@@ -358,13 +366,13 @@ create_estate_for_relation(LogicalRepRelMapEntry *rel,
rte->rellockmode = AccessShareLock;
ExecInitRangeTable(estate, list_make1(rte));
- *resultRelInfo = makeNode(ResultRelInfo);
+ edata->resultRelInfo = resultRelInfo = makeNode(ResultRelInfo);
/*
* Use Relation opened by logicalrep_rel_open() instead of opening it
* again.
*/
- InitResultRelInfo(*resultRelInfo, rel->localrel, 1, NULL, 0);
+ InitResultRelInfo(resultRelInfo, rel->localrel, 1, NULL, 0);
/*
* We put the ResultRelInfo in the es_opened_result_relations list, even
@@ -377,29 +385,38 @@ create_estate_for_relation(LogicalRepRelMapEntry *rel,
* an apply operation being responsible for that.
*/
estate->es_opened_result_relations =
- lappend(estate->es_opened_result_relations, *resultRelInfo);
+ lappend(estate->es_opened_result_relations, resultRelInfo);
estate->es_output_cid = GetCurrentCommandId(true);
/* Prepare to catch AFTER triggers. */
AfterTriggerBeginQuery();
- return estate;
+ /* other fields of edata remain NULL for now */
+
+ return edata;
}
/*
* Finish any operations related to the executor state created by
- * create_estate_for_relation().
+ * create_edata_for_relation().
*/
static void
-finish_estate(EState *estate)
+finish_edata(ApplyExecutionData *edata)
{
+ EState *estate = edata->estate;
+
/* Handle any queued AFTER triggers. */
AfterTriggerEndQuery(estate);
+ /* Shut down tuple routing, if any was done. */
+ if (edata->proute)
+ ExecCleanupTupleRouting(edata->mtstate, edata->proute);
+
/* Cleanup. */
ExecResetTupleTable(estate->es_tupleTable, false);
FreeExecutorState(estate);
+ pfree(edata);
}
/*
@@ -1181,10 +1198,10 @@ GetRelationIdentityOrPK(Relation rel)
static void
apply_handle_insert(StringInfo s)
{
- ResultRelInfo *resultRelInfo;
LogicalRepRelMapEntry *rel;
LogicalRepTupleData newtup;
LogicalRepRelId relid;
+ ApplyExecutionData *edata;
EState *estate;
TupleTableSlot *remoteslot;
MemoryContext oldctx;
@@ -1207,7 +1224,8 @@ apply_handle_insert(StringInfo s)
}
/* Initialize the executor state. */
- estate = create_estate_for_relation(rel, &resultRelInfo);
+ edata = create_edata_for_relation(rel);
+ estate = edata->estate;
remoteslot = ExecInitExtraTupleSlot(estate,
RelationGetDescr(rel->localrel),
&TTSOpsVirtual);
@@ -1223,15 +1241,15 @@ apply_handle_insert(StringInfo s)
/* For a partitioned table, insert the tuple into a partition. */
if (rel->localrel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
- apply_handle_tuple_routing(resultRelInfo, estate,
+ apply_handle_tuple_routing(edata,
remoteslot, NULL, rel, CMD_INSERT);
else
- apply_handle_insert_internal(resultRelInfo, estate,
+ apply_handle_insert_internal(edata->resultRelInfo, estate,
remoteslot);
PopActiveSnapshot();
- finish_estate(estate);
+ finish_edata(edata);
logicalrep_rel_close(rel, NoLock);
@@ -1293,9 +1311,9 @@ check_relation_updatable(LogicalRepRelMapEntry *rel)
static void
apply_handle_update(StringInfo s)
{
- ResultRelInfo *resultRelInfo;
LogicalRepRelMapEntry *rel;
LogicalRepRelId relid;
+ ApplyExecutionData *edata;
EState *estate;
LogicalRepTupleData oldtup;
LogicalRepTupleData newtup;
@@ -1326,7 +1344,8 @@ apply_handle_update(StringInfo s)
check_relation_updatable(rel);
/* Initialize the executor state. */
- estate = create_estate_for_relation(rel, &resultRelInfo);
+ edata = create_edata_for_relation(rel);
+ estate = edata->estate;
remoteslot = ExecInitExtraTupleSlot(estate,
RelationGetDescr(rel->localrel),
&TTSOpsVirtual);
@@ -1368,15 +1387,15 @@ apply_handle_update(StringInfo s)
/* For a partitioned table, apply update to correct partition. */
if (rel->localrel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
- apply_handle_tuple_routing(resultRelInfo, estate,
+ apply_handle_tuple_routing(edata,
remoteslot, &newtup, rel, CMD_UPDATE);
else
- apply_handle_update_internal(resultRelInfo, estate,
+ apply_handle_update_internal(edata->resultRelInfo, estate,
remoteslot, &newtup, rel);
PopActiveSnapshot();
- finish_estate(estate);
+ finish_edata(edata);
logicalrep_rel_close(rel, NoLock);
@@ -1448,10 +1467,10 @@ apply_handle_update_internal(ResultRelInfo *relinfo,
static void
apply_handle_delete(StringInfo s)
{
- ResultRelInfo *resultRelInfo;
LogicalRepRelMapEntry *rel;
LogicalRepTupleData oldtup;
LogicalRepRelId relid;
+ ApplyExecutionData *edata;
EState *estate;
TupleTableSlot *remoteslot;
MemoryContext oldctx;
@@ -1477,7 +1496,8 @@ apply_handle_delete(StringInfo s)
check_relation_updatable(rel);
/* Initialize the executor state. */
- estate = create_estate_for_relation(rel, &resultRelInfo);
+ edata = create_edata_for_relation(rel);
+ estate = edata->estate;
remoteslot = ExecInitExtraTupleSlot(estate,
RelationGetDescr(rel->localrel),
&TTSOpsVirtual);
@@ -1491,15 +1511,15 @@ apply_handle_delete(StringInfo s)
/* For a partitioned table, apply delete to correct partition. */
if (rel->localrel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
- apply_handle_tuple_routing(resultRelInfo, estate,
+ apply_handle_tuple_routing(edata,
remoteslot, NULL, rel, CMD_DELETE);
else
- apply_handle_delete_internal(resultRelInfo, estate,
+ apply_handle_delete_internal(edata->resultRelInfo, estate,
remoteslot, &rel->remoterel);
PopActiveSnapshot();
- finish_estate(estate);
+ finish_edata(edata);
logicalrep_rel_close(rel, NoLock);
@@ -1582,16 +1602,17 @@ FindReplTupleInLocalRel(EState *estate, Relation localrel,
* This handles insert, update, delete on a partitioned table.
*/
static void
-apply_handle_tuple_routing(ResultRelInfo *relinfo,
- EState *estate,
+apply_handle_tuple_routing(ApplyExecutionData *edata,
TupleTableSlot *remoteslot,
LogicalRepTupleData *newtup,
LogicalRepRelMapEntry *relmapentry,
CmdType operation)
{
+ EState *estate = edata->estate;
+ ResultRelInfo *relinfo = edata->resultRelInfo;
Relation parentrel = relinfo->ri_RelationDesc;
- ModifyTableState *mtstate = NULL;
- PartitionTupleRouting *proute = NULL;
+ ModifyTableState *mtstate;
+ PartitionTupleRouting *proute;
ResultRelInfo *partrelinfo;
Relation partrel;
TupleTableSlot *remoteslot_part;
@@ -1599,12 +1620,14 @@ apply_handle_tuple_routing(ResultRelInfo *relinfo,
MemoryContext oldctx;
/* ModifyTableState is needed for ExecFindPartition(). */
- mtstate = makeNode(ModifyTableState);
+ edata->mtstate = mtstate = makeNode(ModifyTableState);
mtstate->ps.plan = NULL;
mtstate->ps.state = estate;
mtstate->operation = operation;
mtstate->resultRelInfo = relinfo;
- proute = ExecSetupPartitionTupleRouting(estate, parentrel);
+
+ /* ... as is PartitionTupleRouting. */
+ edata->proute = proute = ExecSetupPartitionTupleRouting(estate, parentrel);
/*
* Find the partition to which the "search tuple" belongs.
@@ -1797,8 +1820,6 @@ apply_handle_tuple_routing(ResultRelInfo *relinfo,
elog(ERROR, "unrecognized CmdType: %d", (int) operation);
break;
}
-
- ExecCleanupTupleRouting(mtstate, proute);
}
/*
* Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS
2021-05-18 23:42 Subscription tests fail under CLOBBER_CACHE_ALWAYS Tom Lane <tgl@sss.pgh.pa.us>
2021-05-19 03:03 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Michael Paquier <michael@paquier.xyz>
2021-05-19 03:46 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Tom Lane <tgl@sss.pgh.pa.us>
2021-05-19 04:23 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Michael Paquier <michael@paquier.xyz>
2021-05-19 04:56 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Amit Kapila <amit.kapila16@gmail.com>
2021-05-19 05:05 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Michael Paquier <michael@paquier.xyz>
2021-05-19 06:53 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Amit Langote <amitlangote09@gmail.com>
2021-05-19 20:23 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Tom Lane <tgl@sss.pgh.pa.us>
@ 2021-05-19 23:56 ` Michael Paquier <michael@paquier.xyz>
2021-05-20 00:49 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Tom Lane <tgl@sss.pgh.pa.us>
1 sibling, 1 reply; 28+ messages in thread
From: Michael Paquier @ 2021-05-19 23:56 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Amit Langote <amitlangote09@gmail.com>; Amit Kapila <amit.kapila16@gmail.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Wed, May 19, 2021 at 04:23:55PM -0400, Tom Lane wrote:
> I really dislike that patch. I think it's doubling down on the messy,
> unstructured coding patterns that got us into this situation to begin
> with. I'd prefer to expend a little effort on refactoring so that
> the ExecCleanupTupleRouting call can be moved to the cleanup function
> where it belongs.
Okay.
> I did not touch the APIs of the apply_XXX_internal functions,
> as it didn't really seem to offer any notational advantage.
> We can't simply collapse them to take an "edata" as I did for
> apply_handle_tuple_routing, because the ResultRelInfo they're
> supposed to operate on could be different from the original one.
> I considered a couple of alternatives:
>
> * Replace their estate arguments with edata, but keep the separate
> ResultRelInfo arguments. This might be worth doing in future, if we
> add more fields to ApplyExecutionData. Right now it'd save nothing,
> and it'd create a risk of confusion about when to use the
> ResultRelInfo argument vs. edata->resultRelInfo.
Not sure about this one. It may be better to wait until this gets
more expanded, if it gets expanded.
> * Allow apply_handle_tuple_routing to overwrite edata->resultRelInfo
> with the partition child's RRI, then simplify the apply_XXX_internal
> functions to take just edata instead of separate estate and
> resultRelInfo args. I think this would work right now, but it seems
> grotty, and it might cause problems in future.
Agreed that it does not seem like a good idea to blindly overwrite
resultRelInfo with the partition targetted for the apply.
> * Replace the edata->resultRelInfo field with two fields, one for
> the original parent and one for the actual/current target. Perhaps
> this is worth doing, not sure.
This one sounds more natural to me, though.
> Thoughts?
May I ask why you are not moving the snapshot pop and push into the
finish() and create() routines for this patch? Also, any thoughts
about adding the trigger tests from 013_partition.pl to REL_13_STABLE?
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS
2021-05-18 23:42 Subscription tests fail under CLOBBER_CACHE_ALWAYS Tom Lane <tgl@sss.pgh.pa.us>
2021-05-19 03:03 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Michael Paquier <michael@paquier.xyz>
2021-05-19 03:46 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Tom Lane <tgl@sss.pgh.pa.us>
2021-05-19 04:23 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Michael Paquier <michael@paquier.xyz>
2021-05-19 04:56 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Amit Kapila <amit.kapila16@gmail.com>
2021-05-19 05:05 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Michael Paquier <michael@paquier.xyz>
2021-05-19 06:53 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Amit Langote <amitlangote09@gmail.com>
2021-05-19 20:23 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Tom Lane <tgl@sss.pgh.pa.us>
2021-05-19 23:56 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Michael Paquier <michael@paquier.xyz>
@ 2021-05-20 00:49 ` Tom Lane <tgl@sss.pgh.pa.us>
2021-05-20 18:57 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 28+ messages in thread
From: Tom Lane @ 2021-05-20 00:49 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Amit Langote <amitlangote09@gmail.com>; Amit Kapila <amit.kapila16@gmail.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Michael Paquier <michael@paquier.xyz> writes:
> On Wed, May 19, 2021 at 04:23:55PM -0400, Tom Lane wrote:
>> * Replace the edata->resultRelInfo field with two fields, one for
>> the original parent and one for the actual/current target. Perhaps
>> this is worth doing, not sure.
> This one sounds more natural to me, though.
OK, I'll give that a try tomorrow.
> May I ask why you are not moving the snapshot pop and push into the
> finish() and create() routines for this patch?
That does need to happen, but I figured I'd leave it to the other
patch, since there are other things to change too for that snapshot
problem. Obviously, whichever patch goes in second will need trivial
adjustments, but I think it's logically clearer that way.
> Also, any thoughts
> about adding the trigger tests from 013_partition.pl to REL_13_STABLE?
Yeah, if this is a pre-existing problem then we should back-port the
tests that revealed it.
regards, tom lane
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS
2021-05-18 23:42 Subscription tests fail under CLOBBER_CACHE_ALWAYS Tom Lane <tgl@sss.pgh.pa.us>
2021-05-19 03:03 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Michael Paquier <michael@paquier.xyz>
2021-05-19 03:46 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Tom Lane <tgl@sss.pgh.pa.us>
2021-05-19 04:23 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Michael Paquier <michael@paquier.xyz>
2021-05-19 04:56 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Amit Kapila <amit.kapila16@gmail.com>
2021-05-19 05:05 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Michael Paquier <michael@paquier.xyz>
2021-05-19 06:53 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Amit Langote <amitlangote09@gmail.com>
2021-05-19 20:23 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Tom Lane <tgl@sss.pgh.pa.us>
2021-05-19 23:56 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Michael Paquier <michael@paquier.xyz>
2021-05-20 00:49 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Tom Lane <tgl@sss.pgh.pa.us>
@ 2021-05-20 18:57 ` Tom Lane <tgl@sss.pgh.pa.us>
2021-05-21 00:59 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 28+ messages in thread
From: Tom Lane @ 2021-05-20 18:57 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Amit Langote <amitlangote09@gmail.com>; Amit Kapila <amit.kapila16@gmail.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
I wrote:
> Michael Paquier <michael@paquier.xyz> writes:
>> On Wed, May 19, 2021 at 04:23:55PM -0400, Tom Lane wrote:
>>> * Replace the edata->resultRelInfo field with two fields, one for
>>> the original parent and one for the actual/current target. Perhaps
>>> this is worth doing, not sure.
>> This one sounds more natural to me, though.
> OK, I'll give that a try tomorrow.
Here's a version that does it like that. I'm not entirely convinced
whether this is better or not.
regards, tom lane
Attachments:
postpone-tuple-routing-cleanup-2.patch (text/x-diff, 13.8K) download
diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c
index 1432554d5a..d19269ebce 100644
--- a/src/backend/replication/logical/worker.c
+++ b/src/backend/replication/logical/worker.c
@@ -178,6 +178,18 @@ static HTAB *xidhash = NULL;
/* BufFile handle of the current streaming file */
static BufFile *stream_fd = NULL;
+typedef struct ApplyExecutionData
+{
+ EState *estate; /* executor state, used to track resources */
+ ResultRelInfo *origRelInfo; /* the originally-named target relation */
+ ResultRelInfo *activeRelInfo; /* the actual target relation */
+ /* activeRelInfo can equal origRelInfo, or be for a partition of it */
+
+ /* These fields are used when the target relation is partitioned: */
+ ModifyTableState *mtstate; /* dummy ModifyTable state */
+ PartitionTupleRouting *proute; /* partition routing info */
+} ApplyExecutionData;
+
typedef struct SubXactInfo
{
TransactionId xid; /* XID of the subxact */
@@ -226,21 +238,20 @@ static void apply_dispatch(StringInfo s);
static void apply_handle_commit_internal(StringInfo s,
LogicalRepCommitData *commit_data);
-static void apply_handle_insert_internal(ResultRelInfo *relinfo,
- EState *estate, TupleTableSlot *remoteslot);
-static void apply_handle_update_internal(ResultRelInfo *relinfo,
- EState *estate, TupleTableSlot *remoteslot,
+static void apply_handle_insert_internal(ApplyExecutionData *edata,
+ TupleTableSlot *remoteslot);
+static void apply_handle_update_internal(ApplyExecutionData *edata,
+ TupleTableSlot *remoteslot,
LogicalRepTupleData *newtup,
LogicalRepRelMapEntry *relmapentry);
-static void apply_handle_delete_internal(ResultRelInfo *relinfo, EState *estate,
+static void apply_handle_delete_internal(ApplyExecutionData *edata,
TupleTableSlot *remoteslot,
LogicalRepRelation *remoterel);
static bool FindReplTupleInLocalRel(EState *estate, Relation localrel,
LogicalRepRelation *remoterel,
TupleTableSlot *remoteslot,
TupleTableSlot **localslot);
-static void apply_handle_tuple_routing(ResultRelInfo *relinfo,
- EState *estate,
+static void apply_handle_tuple_routing(ApplyExecutionData *edata,
TupleTableSlot *remoteslot,
LogicalRepTupleData *newtup,
LogicalRepRelMapEntry *relmapentry,
@@ -336,20 +347,21 @@ handle_streamed_transaction(LogicalRepMsgType action, StringInfo s)
/*
* Executor state preparation for evaluation of constraint expressions,
- * indexes and triggers.
+ * indexes and triggers for the specified relation.
*
- * resultRelInfo is a ResultRelInfo for the relation to be passed to the
- * executor routines. The caller must open and close any indexes to be
- * updated independently of the relation registered here.
+ * Note that the caller must open and close any indexes to be updated.
*/
-static EState *
-create_estate_for_relation(LogicalRepRelMapEntry *rel,
- ResultRelInfo **resultRelInfo)
+static ApplyExecutionData *
+create_edata_for_relation(LogicalRepRelMapEntry *rel)
{
+ ApplyExecutionData *edata;
EState *estate;
RangeTblEntry *rte;
+ ResultRelInfo *resultRelInfo;
- estate = CreateExecutorState();
+ edata = (ApplyExecutionData *) palloc0(sizeof(ApplyExecutionData));
+
+ edata->estate = estate = CreateExecutorState();
rte = makeNode(RangeTblEntry);
rte->rtekind = RTE_RELATION;
@@ -358,13 +370,16 @@ create_estate_for_relation(LogicalRepRelMapEntry *rel,
rte->rellockmode = AccessShareLock;
ExecInitRangeTable(estate, list_make1(rte));
- *resultRelInfo = makeNode(ResultRelInfo);
+ edata->origRelInfo = resultRelInfo = makeNode(ResultRelInfo);
+
+ /* Initially, set activeRelInfo to be the named rel */
+ edata->activeRelInfo = resultRelInfo;
/*
* Use Relation opened by logicalrep_rel_open() instead of opening it
* again.
*/
- InitResultRelInfo(*resultRelInfo, rel->localrel, 1, NULL, 0);
+ InitResultRelInfo(resultRelInfo, rel->localrel, 1, NULL, 0);
/*
* We put the ResultRelInfo in the es_opened_result_relations list, even
@@ -377,29 +392,38 @@ create_estate_for_relation(LogicalRepRelMapEntry *rel,
* an apply operation being responsible for that.
*/
estate->es_opened_result_relations =
- lappend(estate->es_opened_result_relations, *resultRelInfo);
+ lappend(estate->es_opened_result_relations, resultRelInfo);
estate->es_output_cid = GetCurrentCommandId(true);
/* Prepare to catch AFTER triggers. */
AfterTriggerBeginQuery();
- return estate;
+ /* other fields of edata remain NULL for now */
+
+ return edata;
}
/*
* Finish any operations related to the executor state created by
- * create_estate_for_relation().
+ * create_edata_for_relation().
*/
static void
-finish_estate(EState *estate)
+finish_edata(ApplyExecutionData *edata)
{
+ EState *estate = edata->estate;
+
/* Handle any queued AFTER triggers. */
AfterTriggerEndQuery(estate);
+ /* Shut down tuple routing, if any was done. */
+ if (edata->proute)
+ ExecCleanupTupleRouting(edata->mtstate, edata->proute);
+
/* Cleanup. */
ExecResetTupleTable(estate->es_tupleTable, false);
FreeExecutorState(estate);
+ pfree(edata);
}
/*
@@ -1181,10 +1205,10 @@ GetRelationIdentityOrPK(Relation rel)
static void
apply_handle_insert(StringInfo s)
{
- ResultRelInfo *resultRelInfo;
LogicalRepRelMapEntry *rel;
LogicalRepTupleData newtup;
LogicalRepRelId relid;
+ ApplyExecutionData *edata;
EState *estate;
TupleTableSlot *remoteslot;
MemoryContext oldctx;
@@ -1207,7 +1231,8 @@ apply_handle_insert(StringInfo s)
}
/* Initialize the executor state. */
- estate = create_estate_for_relation(rel, &resultRelInfo);
+ edata = create_edata_for_relation(rel);
+ estate = edata->estate;
remoteslot = ExecInitExtraTupleSlot(estate,
RelationGetDescr(rel->localrel),
&TTSOpsVirtual);
@@ -1223,15 +1248,14 @@ apply_handle_insert(StringInfo s)
/* For a partitioned table, insert the tuple into a partition. */
if (rel->localrel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
- apply_handle_tuple_routing(resultRelInfo, estate,
+ apply_handle_tuple_routing(edata,
remoteslot, NULL, rel, CMD_INSERT);
else
- apply_handle_insert_internal(resultRelInfo, estate,
- remoteslot);
+ apply_handle_insert_internal(edata, remoteslot);
PopActiveSnapshot();
- finish_estate(estate);
+ finish_edata(edata);
logicalrep_rel_close(rel, NoLock);
@@ -1240,9 +1264,13 @@ apply_handle_insert(StringInfo s)
/* Workhorse for apply_handle_insert() */
static void
-apply_handle_insert_internal(ResultRelInfo *relinfo,
- EState *estate, TupleTableSlot *remoteslot)
+apply_handle_insert_internal(ApplyExecutionData *edata,
+ TupleTableSlot *remoteslot)
{
+ EState *estate = edata->estate;
+ ResultRelInfo *relinfo = edata->activeRelInfo;
+
+ /* We must open indexes here. */
ExecOpenIndices(relinfo, false);
/* Do the insert. */
@@ -1293,9 +1321,9 @@ check_relation_updatable(LogicalRepRelMapEntry *rel)
static void
apply_handle_update(StringInfo s)
{
- ResultRelInfo *resultRelInfo;
LogicalRepRelMapEntry *rel;
LogicalRepRelId relid;
+ ApplyExecutionData *edata;
EState *estate;
LogicalRepTupleData oldtup;
LogicalRepTupleData newtup;
@@ -1326,7 +1354,8 @@ apply_handle_update(StringInfo s)
check_relation_updatable(rel);
/* Initialize the executor state. */
- estate = create_estate_for_relation(rel, &resultRelInfo);
+ edata = create_edata_for_relation(rel);
+ estate = edata->estate;
remoteslot = ExecInitExtraTupleSlot(estate,
RelationGetDescr(rel->localrel),
&TTSOpsVirtual);
@@ -1368,15 +1397,15 @@ apply_handle_update(StringInfo s)
/* For a partitioned table, apply update to correct partition. */
if (rel->localrel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
- apply_handle_tuple_routing(resultRelInfo, estate,
+ apply_handle_tuple_routing(edata,
remoteslot, &newtup, rel, CMD_UPDATE);
else
- apply_handle_update_internal(resultRelInfo, estate,
+ apply_handle_update_internal(edata,
remoteslot, &newtup, rel);
PopActiveSnapshot();
- finish_estate(estate);
+ finish_edata(edata);
logicalrep_rel_close(rel, NoLock);
@@ -1385,11 +1414,13 @@ apply_handle_update(StringInfo s)
/* Workhorse for apply_handle_update() */
static void
-apply_handle_update_internal(ResultRelInfo *relinfo,
- EState *estate, TupleTableSlot *remoteslot,
+apply_handle_update_internal(ApplyExecutionData *edata,
+ TupleTableSlot *remoteslot,
LogicalRepTupleData *newtup,
LogicalRepRelMapEntry *relmapentry)
{
+ EState *estate = edata->estate;
+ ResultRelInfo *relinfo = edata->activeRelInfo;
Relation localrel = relinfo->ri_RelationDesc;
EPQState epqstate;
TupleTableSlot *localslot;
@@ -1448,10 +1479,10 @@ apply_handle_update_internal(ResultRelInfo *relinfo,
static void
apply_handle_delete(StringInfo s)
{
- ResultRelInfo *resultRelInfo;
LogicalRepRelMapEntry *rel;
LogicalRepTupleData oldtup;
LogicalRepRelId relid;
+ ApplyExecutionData *edata;
EState *estate;
TupleTableSlot *remoteslot;
MemoryContext oldctx;
@@ -1477,7 +1508,8 @@ apply_handle_delete(StringInfo s)
check_relation_updatable(rel);
/* Initialize the executor state. */
- estate = create_estate_for_relation(rel, &resultRelInfo);
+ edata = create_edata_for_relation(rel);
+ estate = edata->estate;
remoteslot = ExecInitExtraTupleSlot(estate,
RelationGetDescr(rel->localrel),
&TTSOpsVirtual);
@@ -1491,15 +1523,14 @@ apply_handle_delete(StringInfo s)
/* For a partitioned table, apply delete to correct partition. */
if (rel->localrel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
- apply_handle_tuple_routing(resultRelInfo, estate,
+ apply_handle_tuple_routing(edata,
remoteslot, NULL, rel, CMD_DELETE);
else
- apply_handle_delete_internal(resultRelInfo, estate,
- remoteslot, &rel->remoterel);
+ apply_handle_delete_internal(edata, remoteslot, &rel->remoterel);
PopActiveSnapshot();
- finish_estate(estate);
+ finish_edata(edata);
logicalrep_rel_close(rel, NoLock);
@@ -1508,10 +1539,12 @@ apply_handle_delete(StringInfo s)
/* Workhorse for apply_handle_delete() */
static void
-apply_handle_delete_internal(ResultRelInfo *relinfo, EState *estate,
+apply_handle_delete_internal(ApplyExecutionData *edata,
TupleTableSlot *remoteslot,
LogicalRepRelation *remoterel)
{
+ EState *estate = edata->estate;
+ ResultRelInfo *relinfo = edata->activeRelInfo;
Relation localrel = relinfo->ri_RelationDesc;
EPQState epqstate;
TupleTableSlot *localslot;
@@ -1582,16 +1615,17 @@ FindReplTupleInLocalRel(EState *estate, Relation localrel,
* This handles insert, update, delete on a partitioned table.
*/
static void
-apply_handle_tuple_routing(ResultRelInfo *relinfo,
- EState *estate,
+apply_handle_tuple_routing(ApplyExecutionData *edata,
TupleTableSlot *remoteslot,
LogicalRepTupleData *newtup,
LogicalRepRelMapEntry *relmapentry,
CmdType operation)
{
+ EState *estate = edata->estate;
+ ResultRelInfo *relinfo = edata->origRelInfo;
Relation parentrel = relinfo->ri_RelationDesc;
- ModifyTableState *mtstate = NULL;
- PartitionTupleRouting *proute = NULL;
+ ModifyTableState *mtstate;
+ PartitionTupleRouting *proute;
ResultRelInfo *partrelinfo;
Relation partrel;
TupleTableSlot *remoteslot_part;
@@ -1599,12 +1633,14 @@ apply_handle_tuple_routing(ResultRelInfo *relinfo,
MemoryContext oldctx;
/* ModifyTableState is needed for ExecFindPartition(). */
- mtstate = makeNode(ModifyTableState);
+ edata->mtstate = mtstate = makeNode(ModifyTableState);
mtstate->ps.plan = NULL;
mtstate->ps.state = estate;
mtstate->operation = operation;
mtstate->resultRelInfo = relinfo;
- proute = ExecSetupPartitionTupleRouting(estate, parentrel);
+
+ /* ... as is PartitionTupleRouting. */
+ edata->proute = proute = ExecSetupPartitionTupleRouting(estate, parentrel);
/*
* Find the partition to which the "search tuple" belongs.
@@ -1616,6 +1652,9 @@ apply_handle_tuple_routing(ResultRelInfo *relinfo,
Assert(partrelinfo != NULL);
partrel = partrelinfo->ri_RelationDesc;
+ /* Make that partition the active target rel. */
+ edata->activeRelInfo = partrelinfo;
+
/*
* To perform any of the operations below, the tuple must match the
* partition's rowtype. Convert if needed or just copy, using a dedicated
@@ -1638,12 +1677,12 @@ apply_handle_tuple_routing(ResultRelInfo *relinfo,
switch (operation)
{
case CMD_INSERT:
- apply_handle_insert_internal(partrelinfo, estate,
+ apply_handle_insert_internal(edata,
remoteslot_part);
break;
case CMD_DELETE:
- apply_handle_delete_internal(partrelinfo, estate,
+ apply_handle_delete_internal(edata,
remoteslot_part,
&relmapentry->remoterel);
break;
@@ -1757,11 +1796,12 @@ apply_handle_tuple_routing(ResultRelInfo *relinfo,
Assert(partrelinfo_new != partrelinfo);
/* DELETE old tuple found in the old partition. */
- apply_handle_delete_internal(partrelinfo, estate,
+ apply_handle_delete_internal(edata,
localslot,
&relmapentry->remoterel);
/* INSERT new tuple into the new partition. */
+ edata->activeRelInfo = partrelinfo_new;
/*
* Convert the replacement tuple to match the destination
@@ -1787,7 +1827,7 @@ apply_handle_tuple_routing(ResultRelInfo *relinfo,
slot_getallattrs(remoteslot);
}
MemoryContextSwitchTo(oldctx);
- apply_handle_insert_internal(partrelinfo_new, estate,
+ apply_handle_insert_internal(edata,
remoteslot_part);
}
}
@@ -1797,8 +1837,6 @@ apply_handle_tuple_routing(ResultRelInfo *relinfo,
elog(ERROR, "unrecognized CmdType: %d", (int) operation);
break;
}
-
- ExecCleanupTupleRouting(mtstate, proute);
}
/*
* Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS
2021-05-18 23:42 Subscription tests fail under CLOBBER_CACHE_ALWAYS Tom Lane <tgl@sss.pgh.pa.us>
2021-05-19 03:03 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Michael Paquier <michael@paquier.xyz>
2021-05-19 03:46 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Tom Lane <tgl@sss.pgh.pa.us>
2021-05-19 04:23 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Michael Paquier <michael@paquier.xyz>
2021-05-19 04:56 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Amit Kapila <amit.kapila16@gmail.com>
2021-05-19 05:05 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Michael Paquier <michael@paquier.xyz>
2021-05-19 06:53 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Amit Langote <amitlangote09@gmail.com>
2021-05-19 20:23 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Tom Lane <tgl@sss.pgh.pa.us>
2021-05-19 23:56 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Michael Paquier <michael@paquier.xyz>
2021-05-20 00:49 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Tom Lane <tgl@sss.pgh.pa.us>
2021-05-20 18:57 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Tom Lane <tgl@sss.pgh.pa.us>
@ 2021-05-21 00:59 ` Michael Paquier <michael@paquier.xyz>
2021-05-21 05:15 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Amit Kapila <amit.kapila16@gmail.com>
0 siblings, 1 reply; 28+ messages in thread
From: Michael Paquier @ 2021-05-21 00:59 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Amit Langote <amitlangote09@gmail.com>; Amit Kapila <amit.kapila16@gmail.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Thu, May 20, 2021 at 02:57:40PM -0400, Tom Lane wrote:
> Here's a version that does it like that. I'm not entirely convinced
> whether this is better or not.
Hmm. I think that this is better. This makes the code easier to
follow, and the extra information is useful for debugging.
The change looks good to me.
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS
2021-05-18 23:42 Subscription tests fail under CLOBBER_CACHE_ALWAYS Tom Lane <tgl@sss.pgh.pa.us>
2021-05-19 03:03 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Michael Paquier <michael@paquier.xyz>
2021-05-19 03:46 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Tom Lane <tgl@sss.pgh.pa.us>
2021-05-19 04:23 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Michael Paquier <michael@paquier.xyz>
2021-05-19 04:56 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Amit Kapila <amit.kapila16@gmail.com>
2021-05-19 05:05 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Michael Paquier <michael@paquier.xyz>
2021-05-19 06:53 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Amit Langote <amitlangote09@gmail.com>
2021-05-19 20:23 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Tom Lane <tgl@sss.pgh.pa.us>
2021-05-19 23:56 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Michael Paquier <michael@paquier.xyz>
2021-05-20 00:49 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Tom Lane <tgl@sss.pgh.pa.us>
2021-05-20 18:57 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Tom Lane <tgl@sss.pgh.pa.us>
2021-05-21 00:59 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Michael Paquier <michael@paquier.xyz>
@ 2021-05-21 05:15 ` Amit Kapila <amit.kapila16@gmail.com>
0 siblings, 0 replies; 28+ messages in thread
From: Amit Kapila @ 2021-05-21 05:15 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Amit Langote <amitlangote09@gmail.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Fri, May 21, 2021 at 6:29 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Thu, May 20, 2021 at 02:57:40PM -0400, Tom Lane wrote:
> > Here's a version that does it like that. I'm not entirely convinced
> > whether this is better or not.
>
> Hmm. I think that this is better. This makes the code easier to
> follow, and the extra information is useful for debugging.
>
> The change looks good to me.
>
Yeah, the change looks good to me as well but I think we should
consider Amit L's point that maintaining this extra activeRelInfo
might be prone to bugs if the partitioning logic needs to be extended
at other places in the worker.c. As the code stands today, it doesn't
seem problematic so we can go with the second patch if both Tom and
you feel that is a better option.
--
With Regards,
Amit Kapila.
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS
2021-05-18 23:42 Subscription tests fail under CLOBBER_CACHE_ALWAYS Tom Lane <tgl@sss.pgh.pa.us>
2021-05-19 03:03 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Michael Paquier <michael@paquier.xyz>
2021-05-19 03:46 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Tom Lane <tgl@sss.pgh.pa.us>
2021-05-19 04:23 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Michael Paquier <michael@paquier.xyz>
2021-05-19 04:56 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Amit Kapila <amit.kapila16@gmail.com>
2021-05-19 05:05 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Michael Paquier <michael@paquier.xyz>
2021-05-19 06:53 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Amit Langote <amitlangote09@gmail.com>
2021-05-19 20:23 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Tom Lane <tgl@sss.pgh.pa.us>
@ 2021-05-20 00:32 ` Amit Langote <amitlangote09@gmail.com>
2021-05-21 21:01 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Tom Lane <tgl@sss.pgh.pa.us>
1 sibling, 1 reply; 28+ messages in thread
From: Amit Langote @ 2021-05-20 00:32 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Michael Paquier <michael@paquier.xyz>; Amit Kapila <amit.kapila16@gmail.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Thu, May 20, 2021 at 5:23 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Amit Langote <amitlangote09@gmail.com> writes:
> > IOW, the patch you posted earlier seems like the way to go.
>
> I really dislike that patch. I think it's doubling down on the messy,
> unstructured coding patterns that got us into this situation to begin
> with. I'd prefer to expend a little effort on refactoring so that
> the ExecCleanupTupleRouting call can be moved to the cleanup function
> where it belongs.
>
> So, I propose the attached, which invents a new struct to carry
> the stuff we've discovered to be necessary. This makes the APIs
> noticeably cleaner IMHO.
Larger footprint, but definitely cleaner. Thanks.
> I did not touch the APIs of the apply_XXX_internal functions,
> as it didn't really seem to offer any notational advantage.
> We can't simply collapse them to take an "edata" as I did for
> apply_handle_tuple_routing, because the ResultRelInfo they're
> supposed to operate on could be different from the original one.
> I considered a couple of alternatives:
>
> * Replace their estate arguments with edata, but keep the separate
> ResultRelInfo arguments. This might be worth doing in future, if we
> add more fields to ApplyExecutionData. Right now it'd save nothing,
> and it'd create a risk of confusion about when to use the
> ResultRelInfo argument vs. edata->resultRelInfo.
>
> * Allow apply_handle_tuple_routing to overwrite edata->resultRelInfo
> with the partition child's RRI, then simplify the apply_XXX_internal
> functions to take just edata instead of separate estate and
> resultRelInfo args. I think this would work right now, but it seems
> grotty, and it might cause problems in future.
>
> * Replace the edata->resultRelInfo field with two fields, one for
> the original parent and one for the actual/current target. Perhaps
> this is worth doing, not sure.
>
> Thoughts?
IMHO, it would be better to keep the lowest-level
apply_handle_XXX_internal() out of this, because presumably we're only
cleaning up the mess in higher-level callers. Somewhat related, one
of the intentions behind a04daa97a43, which removed
es_result_relation_info in favor of passing the ResultRelInfo
explicitly to the executor's lower-level functions, was to avoid bugs
caused by failing to set/reset that global field correctly in
higher-level callers. Now worker.c is pretty small compared with the
executor, but still it seems like a good idea to follow the same
principle here.
--
Amit Langote
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS
2021-05-18 23:42 Subscription tests fail under CLOBBER_CACHE_ALWAYS Tom Lane <tgl@sss.pgh.pa.us>
2021-05-19 03:03 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Michael Paquier <michael@paquier.xyz>
2021-05-19 03:46 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Tom Lane <tgl@sss.pgh.pa.us>
2021-05-19 04:23 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Michael Paquier <michael@paquier.xyz>
2021-05-19 04:56 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Amit Kapila <amit.kapila16@gmail.com>
2021-05-19 05:05 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Michael Paquier <michael@paquier.xyz>
2021-05-19 06:53 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Amit Langote <amitlangote09@gmail.com>
2021-05-19 20:23 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Tom Lane <tgl@sss.pgh.pa.us>
2021-05-20 00:32 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Amit Langote <amitlangote09@gmail.com>
@ 2021-05-21 21:01 ` Tom Lane <tgl@sss.pgh.pa.us>
2021-05-22 03:06 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Amit Langote <amitlangote09@gmail.com>
0 siblings, 1 reply; 28+ messages in thread
From: Tom Lane @ 2021-05-21 21:01 UTC (permalink / raw)
To: Amit Langote <amitlangote09@gmail.com>; +Cc: Michael Paquier <michael@paquier.xyz>; Amit Kapila <amit.kapila16@gmail.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Amit Langote <amitlangote09@gmail.com> writes:
> IMHO, it would be better to keep the lowest-level
> apply_handle_XXX_internal() out of this, because presumably we're only
> cleaning up the mess in higher-level callers. Somewhat related, one
> of the intentions behind a04daa97a43, which removed
> es_result_relation_info in favor of passing the ResultRelInfo
> explicitly to the executor's lower-level functions, was to avoid bugs
> caused by failing to set/reset that global field correctly in
> higher-level callers.
Yeah, that's a fair point, and after some reflection I think that
repeatedly changing the "active" field of the struct is exactly
what was bothering me about the v2 patch. So in the attached v3,
I went back to passing that as an explicit argument. The state
struct now has no fields that need to change after first being set.
I did notice that we could remove some other random arguments
by adding the LogicalRepRelMapEntry* to the state struct,
so this also does that.
regards, tom lane
Attachments:
postpone-tuple-routing-cleanup-3.patch (text/x-diff, 14.4K) download
diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c
index 452e5f3df7..6a30662f37 100644
--- a/src/backend/replication/logical/worker.c
+++ b/src/backend/replication/logical/worker.c
@@ -178,6 +178,18 @@ static HTAB *xidhash = NULL;
/* BufFile handle of the current streaming file */
static BufFile *stream_fd = NULL;
+typedef struct ApplyExecutionData
+{
+ EState *estate; /* executor state, used to track resources */
+
+ LogicalRepRelMapEntry *targetRel; /* replication target rel */
+ ResultRelInfo *targetRelInfo; /* ResultRelInfo for same */
+
+ /* These fields are used when the target relation is partitioned: */
+ ModifyTableState *mtstate; /* dummy ModifyTable state */
+ PartitionTupleRouting *proute; /* partition routing info */
+} ApplyExecutionData;
+
typedef struct SubXactInfo
{
TransactionId xid; /* XID of the subxact */
@@ -226,24 +238,23 @@ static void apply_dispatch(StringInfo s);
static void apply_handle_commit_internal(StringInfo s,
LogicalRepCommitData *commit_data);
-static void apply_handle_insert_internal(ResultRelInfo *relinfo,
- EState *estate, TupleTableSlot *remoteslot);
-static void apply_handle_update_internal(ResultRelInfo *relinfo,
- EState *estate, TupleTableSlot *remoteslot,
- LogicalRepTupleData *newtup,
- LogicalRepRelMapEntry *relmapentry);
-static void apply_handle_delete_internal(ResultRelInfo *relinfo, EState *estate,
+static void apply_handle_insert_internal(ApplyExecutionData *edata,
+ ResultRelInfo *relinfo,
+ TupleTableSlot *remoteslot);
+static void apply_handle_update_internal(ApplyExecutionData *edata,
+ ResultRelInfo *relinfo,
TupleTableSlot *remoteslot,
- LogicalRepRelation *remoterel);
+ LogicalRepTupleData *newtup);
+static void apply_handle_delete_internal(ApplyExecutionData *edata,
+ ResultRelInfo *relinfo,
+ TupleTableSlot *remoteslot);
static bool FindReplTupleInLocalRel(EState *estate, Relation localrel,
LogicalRepRelation *remoterel,
TupleTableSlot *remoteslot,
TupleTableSlot **localslot);
-static void apply_handle_tuple_routing(ResultRelInfo *relinfo,
- EState *estate,
+static void apply_handle_tuple_routing(ApplyExecutionData *edata,
TupleTableSlot *remoteslot,
LogicalRepTupleData *newtup,
- LogicalRepRelMapEntry *relmapentry,
CmdType operation);
/*
@@ -336,18 +347,17 @@ handle_streamed_transaction(LogicalRepMsgType action, StringInfo s)
/*
* Executor state preparation for evaluation of constraint expressions,
- * indexes and triggers.
+ * indexes and triggers for the specified relation.
*
- * resultRelInfo is a ResultRelInfo for the relation to be passed to the
- * executor routines. The caller must open and close any indexes to be
- * updated independently of the relation registered here.
+ * Note that the caller must open and close any indexes to be updated.
*/
-static EState *
-create_estate_for_relation(LogicalRepRelMapEntry *rel,
- ResultRelInfo **resultRelInfo)
+static ApplyExecutionData *
+create_edata_for_relation(LogicalRepRelMapEntry *rel)
{
+ ApplyExecutionData *edata;
EState *estate;
RangeTblEntry *rte;
+ ResultRelInfo *resultRelInfo;
/*
* Input functions may need an active snapshot, as may AFTER triggers
@@ -356,7 +366,10 @@ create_estate_for_relation(LogicalRepRelMapEntry *rel,
*/
PushActiveSnapshot(GetTransactionSnapshot());
- estate = CreateExecutorState();
+ edata = (ApplyExecutionData *) palloc0(sizeof(ApplyExecutionData));
+ edata->targetRel = rel;
+
+ edata->estate = estate = CreateExecutorState();
rte = makeNode(RangeTblEntry);
rte->rtekind = RTE_RELATION;
@@ -365,13 +378,13 @@ create_estate_for_relation(LogicalRepRelMapEntry *rel,
rte->rellockmode = AccessShareLock;
ExecInitRangeTable(estate, list_make1(rte));
- *resultRelInfo = makeNode(ResultRelInfo);
+ edata->targetRelInfo = resultRelInfo = makeNode(ResultRelInfo);
/*
* Use Relation opened by logicalrep_rel_open() instead of opening it
* again.
*/
- InitResultRelInfo(*resultRelInfo, rel->localrel, 1, NULL, 0);
+ InitResultRelInfo(resultRelInfo, rel->localrel, 1, NULL, 0);
/*
* We put the ResultRelInfo in the es_opened_result_relations list, even
@@ -384,29 +397,39 @@ create_estate_for_relation(LogicalRepRelMapEntry *rel,
* an apply operation being responsible for that.
*/
estate->es_opened_result_relations =
- lappend(estate->es_opened_result_relations, *resultRelInfo);
+ lappend(estate->es_opened_result_relations, resultRelInfo);
estate->es_output_cid = GetCurrentCommandId(true);
/* Prepare to catch AFTER triggers. */
AfterTriggerBeginQuery();
- return estate;
+ /* other fields of edata remain NULL for now */
+
+ return edata;
}
/*
* Finish any operations related to the executor state created by
- * create_estate_for_relation().
+ * create_edata_for_relation().
*/
static void
-finish_estate(EState *estate)
+finish_edata(ApplyExecutionData *edata)
{
+ EState *estate = edata->estate;
+
/* Handle any queued AFTER triggers. */
AfterTriggerEndQuery(estate);
+ /* Shut down tuple routing, if any was done. */
+ if (edata->proute)
+ ExecCleanupTupleRouting(edata->mtstate, edata->proute);
+
/* Cleanup. */
ExecResetTupleTable(estate->es_tupleTable, false);
FreeExecutorState(estate);
+ pfree(edata);
+
PopActiveSnapshot();
}
@@ -1189,10 +1212,10 @@ GetRelationIdentityOrPK(Relation rel)
static void
apply_handle_insert(StringInfo s)
{
- ResultRelInfo *resultRelInfo;
LogicalRepRelMapEntry *rel;
LogicalRepTupleData newtup;
LogicalRepRelId relid;
+ ApplyExecutionData *edata;
EState *estate;
TupleTableSlot *remoteslot;
MemoryContext oldctx;
@@ -1215,7 +1238,8 @@ apply_handle_insert(StringInfo s)
}
/* Initialize the executor state. */
- estate = create_estate_for_relation(rel, &resultRelInfo);
+ edata = create_edata_for_relation(rel);
+ estate = edata->estate;
remoteslot = ExecInitExtraTupleSlot(estate,
RelationGetDescr(rel->localrel),
&TTSOpsVirtual);
@@ -1228,24 +1252,32 @@ apply_handle_insert(StringInfo s)
/* For a partitioned table, insert the tuple into a partition. */
if (rel->localrel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
- apply_handle_tuple_routing(resultRelInfo, estate,
- remoteslot, NULL, rel, CMD_INSERT);
+ apply_handle_tuple_routing(edata,
+ remoteslot, NULL, CMD_INSERT);
else
- apply_handle_insert_internal(resultRelInfo, estate,
+ apply_handle_insert_internal(edata, edata->targetRelInfo,
remoteslot);
- finish_estate(estate);
+ finish_edata(edata);
logicalrep_rel_close(rel, NoLock);
CommandCounterIncrement();
}
-/* Workhorse for apply_handle_insert() */
+/*
+ * Workhorse for apply_handle_insert()
+ * relinfo is for the relation we're actually inserting into
+ * (could be a child partition of edata->targetRelInfo)
+ */
static void
-apply_handle_insert_internal(ResultRelInfo *relinfo,
- EState *estate, TupleTableSlot *remoteslot)
+apply_handle_insert_internal(ApplyExecutionData *edata,
+ ResultRelInfo *relinfo,
+ TupleTableSlot *remoteslot)
{
+ EState *estate = edata->estate;
+
+ /* We must open indexes here. */
ExecOpenIndices(relinfo, false);
/* Do the insert. */
@@ -1296,9 +1328,9 @@ check_relation_updatable(LogicalRepRelMapEntry *rel)
static void
apply_handle_update(StringInfo s)
{
- ResultRelInfo *resultRelInfo;
LogicalRepRelMapEntry *rel;
LogicalRepRelId relid;
+ ApplyExecutionData *edata;
EState *estate;
LogicalRepTupleData oldtup;
LogicalRepTupleData newtup;
@@ -1329,7 +1361,8 @@ apply_handle_update(StringInfo s)
check_relation_updatable(rel);
/* Initialize the executor state. */
- estate = create_estate_for_relation(rel, &resultRelInfo);
+ edata = create_edata_for_relation(rel);
+ estate = edata->estate;
remoteslot = ExecInitExtraTupleSlot(estate,
RelationGetDescr(rel->localrel),
&TTSOpsVirtual);
@@ -1369,26 +1402,32 @@ apply_handle_update(StringInfo s)
/* For a partitioned table, apply update to correct partition. */
if (rel->localrel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
- apply_handle_tuple_routing(resultRelInfo, estate,
- remoteslot, &newtup, rel, CMD_UPDATE);
+ apply_handle_tuple_routing(edata,
+ remoteslot, &newtup, CMD_UPDATE);
else
- apply_handle_update_internal(resultRelInfo, estate,
- remoteslot, &newtup, rel);
+ apply_handle_update_internal(edata, edata->targetRelInfo,
+ remoteslot, &newtup);
- finish_estate(estate);
+ finish_edata(edata);
logicalrep_rel_close(rel, NoLock);
CommandCounterIncrement();
}
-/* Workhorse for apply_handle_update() */
+/*
+ * Workhorse for apply_handle_update()
+ * relinfo is for the relation we're actually updating in
+ * (could be a child partition of edata->targetRelInfo)
+ */
static void
-apply_handle_update_internal(ResultRelInfo *relinfo,
- EState *estate, TupleTableSlot *remoteslot,
- LogicalRepTupleData *newtup,
- LogicalRepRelMapEntry *relmapentry)
+apply_handle_update_internal(ApplyExecutionData *edata,
+ ResultRelInfo *relinfo,
+ TupleTableSlot *remoteslot,
+ LogicalRepTupleData *newtup)
{
+ EState *estate = edata->estate;
+ LogicalRepRelMapEntry *relmapentry = edata->targetRel;
Relation localrel = relinfo->ri_RelationDesc;
EPQState epqstate;
TupleTableSlot *localslot;
@@ -1447,10 +1486,10 @@ apply_handle_update_internal(ResultRelInfo *relinfo,
static void
apply_handle_delete(StringInfo s)
{
- ResultRelInfo *resultRelInfo;
LogicalRepRelMapEntry *rel;
LogicalRepTupleData oldtup;
LogicalRepRelId relid;
+ ApplyExecutionData *edata;
EState *estate;
TupleTableSlot *remoteslot;
MemoryContext oldctx;
@@ -1476,7 +1515,8 @@ apply_handle_delete(StringInfo s)
check_relation_updatable(rel);
/* Initialize the executor state. */
- estate = create_estate_for_relation(rel, &resultRelInfo);
+ edata = create_edata_for_relation(rel);
+ estate = edata->estate;
remoteslot = ExecInitExtraTupleSlot(estate,
RelationGetDescr(rel->localrel),
&TTSOpsVirtual);
@@ -1488,26 +1528,32 @@ apply_handle_delete(StringInfo s)
/* For a partitioned table, apply delete to correct partition. */
if (rel->localrel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
- apply_handle_tuple_routing(resultRelInfo, estate,
- remoteslot, NULL, rel, CMD_DELETE);
+ apply_handle_tuple_routing(edata,
+ remoteslot, NULL, CMD_DELETE);
else
- apply_handle_delete_internal(resultRelInfo, estate,
- remoteslot, &rel->remoterel);
+ apply_handle_delete_internal(edata, edata->targetRelInfo,
+ remoteslot);
- finish_estate(estate);
+ finish_edata(edata);
logicalrep_rel_close(rel, NoLock);
CommandCounterIncrement();
}
-/* Workhorse for apply_handle_delete() */
+/*
+ * Workhorse for apply_handle_delete()
+ * relinfo is for the relation we're actually deleting from
+ * (could be a child partition of edata->targetRelInfo)
+ */
static void
-apply_handle_delete_internal(ResultRelInfo *relinfo, EState *estate,
- TupleTableSlot *remoteslot,
- LogicalRepRelation *remoterel)
+apply_handle_delete_internal(ApplyExecutionData *edata,
+ ResultRelInfo *relinfo,
+ TupleTableSlot *remoteslot)
{
+ EState *estate = edata->estate;
Relation localrel = relinfo->ri_RelationDesc;
+ LogicalRepRelation *remoterel = &edata->targetRel->remoterel;
EPQState epqstate;
TupleTableSlot *localslot;
bool found;
@@ -1577,16 +1623,17 @@ FindReplTupleInLocalRel(EState *estate, Relation localrel,
* This handles insert, update, delete on a partitioned table.
*/
static void
-apply_handle_tuple_routing(ResultRelInfo *relinfo,
- EState *estate,
+apply_handle_tuple_routing(ApplyExecutionData *edata,
TupleTableSlot *remoteslot,
LogicalRepTupleData *newtup,
- LogicalRepRelMapEntry *relmapentry,
CmdType operation)
{
+ EState *estate = edata->estate;
+ LogicalRepRelMapEntry *relmapentry = edata->targetRel;
+ ResultRelInfo *relinfo = edata->targetRelInfo;
Relation parentrel = relinfo->ri_RelationDesc;
- ModifyTableState *mtstate = NULL;
- PartitionTupleRouting *proute = NULL;
+ ModifyTableState *mtstate;
+ PartitionTupleRouting *proute;
ResultRelInfo *partrelinfo;
Relation partrel;
TupleTableSlot *remoteslot_part;
@@ -1594,12 +1641,14 @@ apply_handle_tuple_routing(ResultRelInfo *relinfo,
MemoryContext oldctx;
/* ModifyTableState is needed for ExecFindPartition(). */
- mtstate = makeNode(ModifyTableState);
+ edata->mtstate = mtstate = makeNode(ModifyTableState);
mtstate->ps.plan = NULL;
mtstate->ps.state = estate;
mtstate->operation = operation;
mtstate->resultRelInfo = relinfo;
- proute = ExecSetupPartitionTupleRouting(estate, parentrel);
+
+ /* ... as is PartitionTupleRouting. */
+ edata->proute = proute = ExecSetupPartitionTupleRouting(estate, parentrel);
/*
* Find the partition to which the "search tuple" belongs.
@@ -1633,14 +1682,13 @@ apply_handle_tuple_routing(ResultRelInfo *relinfo,
switch (operation)
{
case CMD_INSERT:
- apply_handle_insert_internal(partrelinfo, estate,
+ apply_handle_insert_internal(edata, partrelinfo,
remoteslot_part);
break;
case CMD_DELETE:
- apply_handle_delete_internal(partrelinfo, estate,
- remoteslot_part,
- &relmapentry->remoterel);
+ apply_handle_delete_internal(edata, partrelinfo,
+ remoteslot_part);
break;
case CMD_UPDATE:
@@ -1752,9 +1800,8 @@ apply_handle_tuple_routing(ResultRelInfo *relinfo,
Assert(partrelinfo_new != partrelinfo);
/* DELETE old tuple found in the old partition. */
- apply_handle_delete_internal(partrelinfo, estate,
- localslot,
- &relmapentry->remoterel);
+ apply_handle_delete_internal(edata, partrelinfo,
+ localslot);
/* INSERT new tuple into the new partition. */
@@ -1782,7 +1829,7 @@ apply_handle_tuple_routing(ResultRelInfo *relinfo,
slot_getallattrs(remoteslot);
}
MemoryContextSwitchTo(oldctx);
- apply_handle_insert_internal(partrelinfo_new, estate,
+ apply_handle_insert_internal(edata, partrelinfo_new,
remoteslot_part);
}
}
@@ -1792,8 +1839,6 @@ apply_handle_tuple_routing(ResultRelInfo *relinfo,
elog(ERROR, "unrecognized CmdType: %d", (int) operation);
break;
}
-
- ExecCleanupTupleRouting(mtstate, proute);
}
/*
* Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS
2021-05-18 23:42 Subscription tests fail under CLOBBER_CACHE_ALWAYS Tom Lane <tgl@sss.pgh.pa.us>
2021-05-19 03:03 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Michael Paquier <michael@paquier.xyz>
2021-05-19 03:46 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Tom Lane <tgl@sss.pgh.pa.us>
2021-05-19 04:23 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Michael Paquier <michael@paquier.xyz>
2021-05-19 04:56 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Amit Kapila <amit.kapila16@gmail.com>
2021-05-19 05:05 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Michael Paquier <michael@paquier.xyz>
2021-05-19 06:53 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Amit Langote <amitlangote09@gmail.com>
2021-05-19 20:23 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Tom Lane <tgl@sss.pgh.pa.us>
2021-05-20 00:32 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Amit Langote <amitlangote09@gmail.com>
2021-05-21 21:01 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Tom Lane <tgl@sss.pgh.pa.us>
@ 2021-05-22 03:06 ` Amit Langote <amitlangote09@gmail.com>
2021-05-22 13:18 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 28+ messages in thread
From: Amit Langote @ 2021-05-22 03:06 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Michael Paquier <michael@paquier.xyz>; Amit Kapila <amit.kapila16@gmail.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Sat, May 22, 2021 at 6:01 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Amit Langote <amitlangote09@gmail.com> writes:
> > IMHO, it would be better to keep the lowest-level
> > apply_handle_XXX_internal() out of this, because presumably we're only
> > cleaning up the mess in higher-level callers. Somewhat related, one
> > of the intentions behind a04daa97a43, which removed
> > es_result_relation_info in favor of passing the ResultRelInfo
> > explicitly to the executor's lower-level functions, was to avoid bugs
> > caused by failing to set/reset that global field correctly in
> > higher-level callers.
>
> Yeah, that's a fair point, and after some reflection I think that
> repeatedly changing the "active" field of the struct is exactly
> what was bothering me about the v2 patch. So in the attached v3,
> I went back to passing that as an explicit argument. The state
> struct now has no fields that need to change after first being set.
Thanks, that looks good to me.
> I did notice that we could remove some other random arguments
> by adding the LogicalRepRelMapEntry* to the state struct,
> so this also does that.
That seems fine.
BTW, I think we'd need to cherry-pick f3b141c4825 (or maybe parts of
it) into v13 branch for back-patching this.
--
Amit Langote
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS
2021-05-18 23:42 Subscription tests fail under CLOBBER_CACHE_ALWAYS Tom Lane <tgl@sss.pgh.pa.us>
2021-05-19 03:03 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Michael Paquier <michael@paquier.xyz>
2021-05-19 03:46 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Tom Lane <tgl@sss.pgh.pa.us>
2021-05-19 04:23 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Michael Paquier <michael@paquier.xyz>
2021-05-19 04:56 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Amit Kapila <amit.kapila16@gmail.com>
2021-05-19 05:05 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Michael Paquier <michael@paquier.xyz>
2021-05-19 06:53 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Amit Langote <amitlangote09@gmail.com>
2021-05-19 20:23 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Tom Lane <tgl@sss.pgh.pa.us>
2021-05-20 00:32 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Amit Langote <amitlangote09@gmail.com>
2021-05-21 21:01 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Tom Lane <tgl@sss.pgh.pa.us>
2021-05-22 03:06 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Amit Langote <amitlangote09@gmail.com>
@ 2021-05-22 13:18 ` Tom Lane <tgl@sss.pgh.pa.us>
2021-05-22 15:32 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 28+ messages in thread
From: Tom Lane @ 2021-05-22 13:18 UTC (permalink / raw)
To: Amit Langote <amitlangote09@gmail.com>; +Cc: Michael Paquier <michael@paquier.xyz>; Amit Kapila <amit.kapila16@gmail.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Amit Langote <amitlangote09@gmail.com> writes:
> BTW, I think we'd need to cherry-pick f3b141c4825 (or maybe parts of
> it) into v13 branch for back-patching this.
I already did a fair amount of that yesterday, cf 84f5c2908 et al.
But that does raise the question of how far we need to back-patch this.
I gather that the whole issue might've started with 1375422c, so maybe
we don't really need a back-patch at all? But I'm sort of inclined to
back-patch to v11 as I did with 84f5c2908, mainly to keep the worker.c
code looking more alike in those branches.
regards, tom lane
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS
2021-05-18 23:42 Subscription tests fail under CLOBBER_CACHE_ALWAYS Tom Lane <tgl@sss.pgh.pa.us>
2021-05-19 03:03 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Michael Paquier <michael@paquier.xyz>
2021-05-19 03:46 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Tom Lane <tgl@sss.pgh.pa.us>
2021-05-19 04:23 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Michael Paquier <michael@paquier.xyz>
2021-05-19 04:56 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Amit Kapila <amit.kapila16@gmail.com>
2021-05-19 05:05 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Michael Paquier <michael@paquier.xyz>
2021-05-19 06:53 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Amit Langote <amitlangote09@gmail.com>
2021-05-19 20:23 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Tom Lane <tgl@sss.pgh.pa.us>
2021-05-20 00:32 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Amit Langote <amitlangote09@gmail.com>
2021-05-21 21:01 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Tom Lane <tgl@sss.pgh.pa.us>
2021-05-22 03:06 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Amit Langote <amitlangote09@gmail.com>
2021-05-22 13:18 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Tom Lane <tgl@sss.pgh.pa.us>
@ 2021-05-22 15:32 ` Tom Lane <tgl@sss.pgh.pa.us>
2021-05-23 01:28 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 28+ messages in thread
From: Tom Lane @ 2021-05-22 15:32 UTC (permalink / raw)
To: Amit Langote <amitlangote09@gmail.com>; +Cc: Michael Paquier <michael@paquier.xyz>; Amit Kapila <amit.kapila16@gmail.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
I wrote:
> Amit Langote <amitlangote09@gmail.com> writes:
>> BTW, I think we'd need to cherry-pick f3b141c4825 (or maybe parts of
>> it) into v13 branch for back-patching this.
> I already did a fair amount of that yesterday, cf 84f5c2908 et al.
> But that does raise the question of how far we need to back-patch this.
> I gather that the whole issue might've started with 1375422c, so maybe
> we don't really need a back-patch at all?
... wrong. Running v13 branch tip under CLOBBER_CACHE_ALWAYS provokes
a core dump in 013_partition.pl, so 1375422c is not to blame. Now
I'm wondering how far back there's a live issue.
regards, tom lane
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS
2021-05-18 23:42 Subscription tests fail under CLOBBER_CACHE_ALWAYS Tom Lane <tgl@sss.pgh.pa.us>
2021-05-19 03:03 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Michael Paquier <michael@paquier.xyz>
2021-05-19 03:46 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Tom Lane <tgl@sss.pgh.pa.us>
2021-05-19 04:23 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Michael Paquier <michael@paquier.xyz>
2021-05-19 04:56 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Amit Kapila <amit.kapila16@gmail.com>
2021-05-19 05:05 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Michael Paquier <michael@paquier.xyz>
2021-05-19 06:53 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Amit Langote <amitlangote09@gmail.com>
2021-05-19 20:23 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Tom Lane <tgl@sss.pgh.pa.us>
2021-05-20 00:32 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Amit Langote <amitlangote09@gmail.com>
2021-05-21 21:01 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Tom Lane <tgl@sss.pgh.pa.us>
2021-05-22 03:06 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Amit Langote <amitlangote09@gmail.com>
2021-05-22 13:18 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Tom Lane <tgl@sss.pgh.pa.us>
2021-05-22 15:32 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Tom Lane <tgl@sss.pgh.pa.us>
@ 2021-05-23 01:28 ` Tom Lane <tgl@sss.pgh.pa.us>
2021-05-23 05:05 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Amit Langote <amitlangote09@gmail.com>
0 siblings, 1 reply; 28+ messages in thread
From: Tom Lane @ 2021-05-23 01:28 UTC (permalink / raw)
To: Amit Langote <amitlangote09@gmail.com>; +Cc: Michael Paquier <michael@paquier.xyz>; Amit Kapila <amit.kapila16@gmail.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
I wrote:
> ... wrong. Running v13 branch tip under CLOBBER_CACHE_ALWAYS provokes
> a core dump in 013_partition.pl, so 1375422c is not to blame. Now
> I'm wondering how far back there's a live issue.
Oh, of course, it's directly the fault of the patch that added support
for partitioned target tables.
I concluded that a verbatim backpatch wasn't too suitable because
a04daa97a had changed a lot of the APIs here. So I left the APIs
for the xxx_internal() functions alone. Otherwise the patch
pretty much works as-is in v13.
regards, tom lane
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS
2021-05-18 23:42 Subscription tests fail under CLOBBER_CACHE_ALWAYS Tom Lane <tgl@sss.pgh.pa.us>
2021-05-19 03:03 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Michael Paquier <michael@paquier.xyz>
2021-05-19 03:46 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Tom Lane <tgl@sss.pgh.pa.us>
2021-05-19 04:23 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Michael Paquier <michael@paquier.xyz>
2021-05-19 04:56 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Amit Kapila <amit.kapila16@gmail.com>
2021-05-19 05:05 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Michael Paquier <michael@paquier.xyz>
2021-05-19 06:53 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Amit Langote <amitlangote09@gmail.com>
2021-05-19 20:23 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Tom Lane <tgl@sss.pgh.pa.us>
2021-05-20 00:32 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Amit Langote <amitlangote09@gmail.com>
2021-05-21 21:01 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Tom Lane <tgl@sss.pgh.pa.us>
2021-05-22 03:06 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Amit Langote <amitlangote09@gmail.com>
2021-05-22 13:18 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Tom Lane <tgl@sss.pgh.pa.us>
2021-05-22 15:32 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Tom Lane <tgl@sss.pgh.pa.us>
2021-05-23 01:28 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Tom Lane <tgl@sss.pgh.pa.us>
@ 2021-05-23 05:05 ` Amit Langote <amitlangote09@gmail.com>
2021-05-23 08:38 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 28+ messages in thread
From: Amit Langote @ 2021-05-23 05:05 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Michael Paquier <michael@paquier.xyz>; Amit Kapila <amit.kapila16@gmail.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Sun, May 23, 2021 at 10:28 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I wrote:
> > ... wrong. Running v13 branch tip under CLOBBER_CACHE_ALWAYS provokes
> > a core dump in 013_partition.pl, so 1375422c is not to blame. Now
> > I'm wondering how far back there's a live issue.
>
> Oh, of course, it's directly the fault of the patch that added support
> for partitioned target tables.
Yeah, the problem seems to affect only partition child tables, so yeah
this problem started with f1ac27bfda6.
> I concluded that a verbatim backpatch wasn't too suitable because
> a04daa97a had changed a lot of the APIs here. So I left the APIs
> for the xxx_internal() functions alone. Otherwise the patch
> pretty much works as-is in v13.
That looks reasonable.
Thanks.
--
Amit Langote
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS
2021-05-18 23:42 Subscription tests fail under CLOBBER_CACHE_ALWAYS Tom Lane <tgl@sss.pgh.pa.us>
2021-05-19 03:03 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Michael Paquier <michael@paquier.xyz>
2021-05-19 03:46 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Tom Lane <tgl@sss.pgh.pa.us>
2021-05-19 04:23 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Michael Paquier <michael@paquier.xyz>
2021-05-19 04:56 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Amit Kapila <amit.kapila16@gmail.com>
2021-05-19 05:05 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Michael Paquier <michael@paquier.xyz>
2021-05-19 06:53 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Amit Langote <amitlangote09@gmail.com>
2021-05-19 20:23 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Tom Lane <tgl@sss.pgh.pa.us>
2021-05-20 00:32 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Amit Langote <amitlangote09@gmail.com>
2021-05-21 21:01 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Tom Lane <tgl@sss.pgh.pa.us>
2021-05-22 03:06 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Amit Langote <amitlangote09@gmail.com>
2021-05-22 13:18 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Tom Lane <tgl@sss.pgh.pa.us>
2021-05-22 15:32 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Tom Lane <tgl@sss.pgh.pa.us>
2021-05-23 01:28 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Tom Lane <tgl@sss.pgh.pa.us>
2021-05-23 05:05 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Amit Langote <amitlangote09@gmail.com>
@ 2021-05-23 08:38 ` Michael Paquier <michael@paquier.xyz>
0 siblings, 0 replies; 28+ messages in thread
From: Michael Paquier @ 2021-05-23 08:38 UTC (permalink / raw)
To: Amit Langote <amitlangote09@gmail.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Amit Kapila <amit.kapila16@gmail.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Sun, May 23, 2021 at 02:05:59PM +0900, Amit Langote wrote:
> On Sun, May 23, 2021 at 10:28 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I wrote:
>> > ... wrong. Running v13 branch tip under CLOBBER_CACHE_ALWAYS provokes
>> > a core dump in 013_partition.pl, so 1375422c is not to blame. Now
>> > I'm wondering how far back there's a live issue.
>>
>> Oh, of course, it's directly the fault of the patch that added support
>> for partitioned target tables.
>
> Yeah, the problem seems to affect only partition child tables, so yeah
> this problem started with f1ac27bfda6.
Yep.
>> I concluded that a verbatim backpatch wasn't too suitable because
>> a04daa97a had changed a lot of the APIs here. So I left the APIs
>> for the xxx_internal() functions alone. Otherwise the patch
>> pretty much works as-is in v13.
Thanks for the backpatch of the partition tests via d18ee6f.
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS
2021-05-18 23:42 Subscription tests fail under CLOBBER_CACHE_ALWAYS Tom Lane <tgl@sss.pgh.pa.us>
2021-05-19 03:03 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Michael Paquier <michael@paquier.xyz>
@ 2021-05-19 18:36 ` Andrew Dunstan <andrew@dunslane.net>
2021-05-20 00:02 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Michael Paquier <michael@paquier.xyz>
2 siblings, 1 reply; 28+ messages in thread
From: Andrew Dunstan @ 2021-05-19 18:36 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; Tom Lane <tgl@sss.pgh.pa.us>; +Cc: pgsql-hackers@lists.postgresql.org
On 5/18/21 11:03 PM, Michael Paquier wrote:
>
>> 3. Once the subscriber1 postmaster has exited, the TAP
>> test will eventually time out, and then this happens:
>>
>> [.. logs ..]
>>
>> That is, because we failed to shut down subscriber1, the
>> test script neglects to shut down subscriber2, and now
>> things just sit indefinitely. So that's a robustness
>> problem in the TAP infrastructure, rather than a bug in
>> PG proper; but I still say it's a bug that needs fixing.
> This one comes down to teardown_node() that uses system_or_bail(),
> leaving things unfinished. I guess that we could be more aggressive
> and ignore failures if we have a non-zero error code and that not all
> the tests have passed within the END block of PostgresNode.pm.
Yeah, this area needs substantial improvement. I have seen similar sorts
of nasty hangs, where the script is waiting forever for some process
that hasn't got the shutdown message. At least we probably need some way
of making sure the END handler doesn't abort early. Maybe
PostgresNode::stop() needs a mode that handles failure more gracefully.
Maybe it needs to try shutting down all the nodes and only calling
BAIL_OUT after trying all of them and getting a failure. But that might
still leave us work to do on failures occuring pre-END.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS
2021-05-18 23:42 Subscription tests fail under CLOBBER_CACHE_ALWAYS Tom Lane <tgl@sss.pgh.pa.us>
2021-05-19 03:03 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Michael Paquier <michael@paquier.xyz>
2021-05-19 18:36 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Andrew Dunstan <andrew@dunslane.net>
@ 2021-05-20 00:02 ` Michael Paquier <michael@paquier.xyz>
0 siblings, 0 replies; 28+ messages in thread
From: Michael Paquier @ 2021-05-20 00:02 UTC (permalink / raw)
To: Andrew Dunstan <andrew@dunslane.net>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; pgsql-hackers@lists.postgresql.org
On Wed, May 19, 2021 at 02:36:03PM -0400, Andrew Dunstan wrote:
> Yeah, this area needs substantial improvement. I have seen similar sorts
> of nasty hangs, where the script is waiting forever for some process
> that hasn't got the shutdown message. At least we probably need some way
> of making sure the END handler doesn't abort early. Maybe
> PostgresNode::stop() needs a mode that handles failure more gracefully.
> Maybe it needs to try shutting down all the nodes and only calling
> BAIL_OUT after trying all of them and getting a failure. But that might
> still leave us work to do on failures occuring pre-END.
For that, we could just make the END block called run_log() directly
as well, as this catches stderr and an error code. What about making
the shutdown a two-phase logic by the way? Trigger an immediate stop,
and if it fails fallback to an extra kill9() to be on the safe side.
Have you seen this being a problem even in cases where the tests all
passed? If yes, it may be worth using the more aggressive flow even
in the case where the tests pass.
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS
2021-05-18 23:42 Subscription tests fail under CLOBBER_CACHE_ALWAYS Tom Lane <tgl@sss.pgh.pa.us>
@ 2021-05-19 12:54 ` Tomas Vondra <tomas.vondra@enterprisedb.com>
2 siblings, 0 replies; 28+ messages in thread
From: Tomas Vondra @ 2021-05-19 12:54 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; pgsql-hackers@lists.postgresql.org
On 5/19/21 1:42 AM, Tom Lane wrote:
> I discovered $SUBJECT after wondering why hyrax hadn't reported
> in recently, and trying to run check-world under CCA to see if
> anything got stuck. Indeed it did --- although this doesn't
> explain the radio silence from hyrax, because that animal doesn't
> run any TAP tests. (Neither does avocet, which I think is the
> only other active CCA critter. So this could have been broken
> for a very long time.)
>
There are three CCA animals on the same box (avocet, husky and
trilobite) with different compilers, running in a round-robin manner.
One cycle took about 14 days, but about a month ago the machine got
stuck, requiring a hard reboot about a week ago (no idea why it got
stuck). It has more CPU power now (8 cores instead of 2), so it should
take less time to run one test cycle.
avocet already ran all the tests, husky is running HEAD at the moment
and then it'll be trilobite's turn ... AFAICS none of those runs seems
to have failed or got stuck so far.
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS
2021-05-18 23:42 Subscription tests fail under CLOBBER_CACHE_ALWAYS Tom Lane <tgl@sss.pgh.pa.us>
@ 2021-05-21 22:14 ` Tom Lane <tgl@sss.pgh.pa.us>
2021-05-21 23:54 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Tom Lane <tgl@sss.pgh.pa.us>
2 siblings, 1 reply; 28+ messages in thread
From: Tom Lane @ 2021-05-21 22:14 UTC (permalink / raw)
To: pgsql-hackers@lists.postgresql.org
I wrote:
> I count three distinct bugs that were exposed by this attempt:
> ...
> 2. Said bug causes a segfault in the apply worker process.
> This causes the parent postmaster to give up and die.
> I don't understand why we don't treat that like a crash
> in a regular backend, considering that an apply worker
> is running largely user-defined code.
Figured that one out: we *do* treat it like a crash in a regular
backend, which explains the lack of field complaints. What's
contributing to the TAP test getting stuck is that PostgresNode.pm
does this:
open my $conf, '>>', "$pgdata/postgresql.conf";
print $conf "\n# Added by PostgresNode.pm\n";
...
print $conf "restart_after_crash = off\n";
So that'd be fine, if only the TAP tests were a bit more robust
about postmasters disappearing unexpectedly.
BTW, I wonder whether it wouldn't be a good idea for the
postmaster to log something along the lines of "stopping
because restart_after_crash is off". The present behavior
can be quite mysterious otherwise (it certainly confused me).
regards, tom lane
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS
2021-05-18 23:42 Subscription tests fail under CLOBBER_CACHE_ALWAYS Tom Lane <tgl@sss.pgh.pa.us>
2021-05-21 22:14 ` Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS Tom Lane <tgl@sss.pgh.pa.us>
@ 2021-05-21 23:54 ` Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 0 replies; 28+ messages in thread
From: Tom Lane @ 2021-05-21 23:54 UTC (permalink / raw)
To: pgsql-hackers@lists.postgresql.org
I wrote:
> BTW, I wonder whether it wouldn't be a good idea for the
> postmaster to log something along the lines of "stopping
> because restart_after_crash is off". The present behavior
> can be quite mysterious otherwise (it certainly confused me).
Concretely, I suggest the attached.
While checking the other ExitPostmaster calls to see if any of
them lacked suitable log messages, I noticed that there's one
after a call to AuxiliaryProcessMain, which is marked
pg_attribute_noreturn(). So that's dead code, and if it
weren't dead it'd be wrong, because we shouldn't use
ExitPostmaster to exit a child process.
regards, tom lane
Attachments:
log-explanation-when-postmaster-shuts-down.patch (text/x-diff, 1.6K) download
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index 5327859472..5a050898fe 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -3973,7 +3973,11 @@ PostmasterStateMachine(void)
if (ReachedNormalRunning)
CancelBackup();
- /* Normal exit from the postmaster is here */
+ /*
+ * Normal exit from the postmaster is here. We don't need to log
+ * anything here, since the UnlinkLockFiles proc_exit callback
+ * will do so, and that should be the last user-visible action.
+ */
ExitPostmaster(0);
}
}
@@ -3985,9 +3989,21 @@ PostmasterStateMachine(void)
* startup process fails, because more than likely it will just fail again
* and we will keep trying forever.
*/
- if (pmState == PM_NO_CHILDREN &&
- (StartupStatus == STARTUP_CRASHED || !restart_after_crash))
- ExitPostmaster(1);
+ if (pmState == PM_NO_CHILDREN)
+ {
+ if (StartupStatus == STARTUP_CRASHED)
+ {
+ ereport(LOG,
+ (errmsg("shutting down due to startup process failure")));
+ ExitPostmaster(1);
+ }
+ if (!restart_after_crash)
+ {
+ ereport(LOG,
+ (errmsg("shutting down because restart_after_crash is off")));
+ ExitPostmaster(1);
+ }
+ }
/*
* If we need to recover from a crash, wait for all non-syslogger children
@@ -5439,8 +5455,7 @@ StartChildProcess(AuxProcType type)
MemoryContextDelete(PostmasterContext);
PostmasterContext = NULL;
- AuxiliaryProcessMain(ac, av);
- ExitPostmaster(0);
+ AuxiliaryProcessMain(ac, av); /* does not return */
}
#endif /* EXEC_BACKEND */
=
end of thread, other threads:[~2021-05-23 08:38 UTC | newest]
Thread overview: 28+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2021-05-18 23:42 Subscription tests fail under CLOBBER_CACHE_ALWAYS Tom Lane <tgl@sss.pgh.pa.us>
2021-05-19 03:03 ` Michael Paquier <michael@paquier.xyz>
2021-05-19 03:32 ` Amit Langote <amitlangote09@gmail.com>
2021-05-19 03:46 ` Tom Lane <tgl@sss.pgh.pa.us>
2021-05-19 04:23 ` Michael Paquier <michael@paquier.xyz>
2021-05-19 04:56 ` Amit Kapila <amit.kapila16@gmail.com>
2021-05-19 05:05 ` Michael Paquier <michael@paquier.xyz>
2021-05-19 05:21 ` Amit Kapila <amit.kapila16@gmail.com>
2021-05-19 06:53 ` Amit Langote <amitlangote09@gmail.com>
2021-05-19 20:23 ` Tom Lane <tgl@sss.pgh.pa.us>
2021-05-19 23:56 ` Michael Paquier <michael@paquier.xyz>
2021-05-20 00:49 ` Tom Lane <tgl@sss.pgh.pa.us>
2021-05-20 18:57 ` Tom Lane <tgl@sss.pgh.pa.us>
2021-05-21 00:59 ` Michael Paquier <michael@paquier.xyz>
2021-05-21 05:15 ` Amit Kapila <amit.kapila16@gmail.com>
2021-05-20 00:32 ` Amit Langote <amitlangote09@gmail.com>
2021-05-21 21:01 ` Tom Lane <tgl@sss.pgh.pa.us>
2021-05-22 03:06 ` Amit Langote <amitlangote09@gmail.com>
2021-05-22 13:18 ` Tom Lane <tgl@sss.pgh.pa.us>
2021-05-22 15:32 ` Tom Lane <tgl@sss.pgh.pa.us>
2021-05-23 01:28 ` Tom Lane <tgl@sss.pgh.pa.us>
2021-05-23 05:05 ` Amit Langote <amitlangote09@gmail.com>
2021-05-23 08:38 ` Michael Paquier <michael@paquier.xyz>
2021-05-19 18:36 ` Andrew Dunstan <andrew@dunslane.net>
2021-05-20 00:02 ` Michael Paquier <michael@paquier.xyz>
2021-05-19 12:54 ` Tomas Vondra <tomas.vondra@enterprisedb.com>
2021-05-21 22:14 ` Tom Lane <tgl@sss.pgh.pa.us>
2021-05-21 23:54 ` Tom Lane <tgl@sss.pgh.pa.us>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox