Re: Avoid circular header file dependency
On Sat, Apr 26, 2025 at 01:20:56AM -0400, Tom Lane wrote: > Bertrand Drouvot writes: >> While working on wait events I faced some compilation issues due to circular >> header file dependency (introduced in fa88928470b5) between wait_event.h and >> wait_event_types.h. > > Ugh. I still carry the scars of cleaning up after a previous > circular-inclusion mess (cf 1609797c2), so I'm always worried about > introducing new cases. I don't have an opinion about whether this > specific refactoring is the best way to deal with this case, but > I definitely feel that we mustn't allow the situation to persist. This one is my fault, so I'll take care of it. Splitting the values of the wait classes into their own header makes sense, but the header name wait_class_constants.h sounds a bit off. Why not a simpler "wait_classes.h" that gets included by wait_event.h and wait_event_types.h? -- Michael signature.asc Description: PGP signature
Re: Fix premature xmin advancement during fast forward decoding
On Sat, Apr 26, 2025 at 5:01 AM Amit Kapila wrote: > > On Sat, Apr 26, 2025 at 12:01 AM Masahiko Sawada > wrote: > > > > On Fri, Apr 25, 2025 at 4:42 AM Amit Kapila wrote: > > > > > > Can you think of any better ideas? > > > > No idea. Hmm, there seems no reasonable way to fix this issue for back > > branches. I consented to the view that these costs were something that > > we should have paid from the beginning. > > > > Right, I feel we should go with the simple change proposed by Hou-San > for now to fix the bug. If, in the future, we encounter any cases > where such optimizations can help for fast-forward mode, then we can > consider it. Does that sound reasonable to you? Yes, agreed with this approach. Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com
Re: Logical Replication of sequences
Hi Vignesh.
Some review comments for v20250426-0005.
==
doc/src/sgml/catalogs.sgml
1.
- State code:
+ State code for tables:
i = initialize,
d = data is being copied,
f = finished table copy,
s = synchronized,
r = ready (normal replication)
+
+
+ State code for sequences:
+ i = initialize,
+ r = ready
1a.
There should be an introductory sentence to say what this field is.
e.g. "State code for the table or sequence."
~
1b.
/State code for tables/State codes for tables/
~
1c.
/State code for sequences/State codes for sequences/
==
doc/src/sgml/logical-replication.sgml
2.
+ or FOR ALL SEQUENCES. Unlike tables, sequences
allow users
+ to synchronize their current state at any given time. For more information,
+ refer to .
This is OK, but maybe the "sequences allow users..." is worded
strangely. How about below?
SUGGESTION
Unlike tables, the current state of sequences may be synchronised at any time.
~~~
3.
+ Incremental sequence changes are not replicated. The data in serial or
+ identity columns backed by sequences will of course be replicated as part
+ of the table, the sequences themselves do not replicate ongoing changes.
Seems to be a missing word here
/The data in serial/Although the data in serial/
OR
Just change the punctuation to a semicolon.
/of the table,/of the table;/
==
doc/src/sgml/ref/alter_subscription.sgml
4.
+
+ Previously subscribed sequences are not re-synchronized. To do that,
+ see
+ ALTER SUBSCRIPTION ... REFRESH PUBLICATION
SEQUENCES
+
4a.
Missing period in the last sentence.
~
4b.
AFAIK, when copy_data=false, then not only will *existing* sequences
not be synchronised, but even the *new* sequences will not be
synchronised. Effectively, when copy_data = false, then nothing at all
happens for sequences as far as what the user sees, right?
Experiment:
test_pub=# create publication pub1 for all sequences;
CREATE PUBLICATION
test_sub=# create sequence s1;
CREATE SEQUENCE
NOTICE: created replication slot "sub1" on publisher
CREATE SUBSCRIPTION
test_pub=# create sequence s1;
CREATE SEQUENCE
test_pub=# select * from nextval('s1');
nextval
-
1
(1 row)
test_pub=# select * from nextval('s1');
nextval
-
2
(1 row)
test_pub=# select * from nextval('s1');
nextval
-
3
(1 row)
test_sub=# alter subscription sub1 refresh publication with (copy_data=false);
ALTER SUBSCRIPTION
test_sub=# select * from s1;
last_value | log_cnt | is_called
+-+---
1 | 0 | f
(1 row)
So, subscriber side s1 is unaffected.
Maybe it is not worth the effort, but doesn't this mean that you could
optimise the AlterSubscription_refresh() logic to completely skip all
processing for sequences when copy_data=false. e.g. what's the point
of gathering publisher sequence lists and setting INIT states for
them, etc, when it won't synchronise anything because copy_data=false?
Everything will be synchronised later anyway when the user does
REFRESH PUBLICATION SEQUENCES.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
Re: Avoid circular header file dependency
Hi, On Sat, Apr 26, 2025 at 04:42:56PM +0900, Michael Paquier wrote: > This one is my fault, I do feel guilty too... > Splitting the values > of the wait classes into their own header makes sense, but the header > name wait_class_constants.h sounds a bit off. Why not a simpler > "wait_classes.h" that gets included by wait_event.h and > wait_event_types.h? Yeah, better. Done that way in the attached. Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com >From c6d1883b6fef09e4c3512b6f6704c1913ff719ce Mon Sep 17 00:00:00 2001 From: Bertrand Drouvot Date: Thu, 24 Apr 2025 17:11:03 + Subject: [PATCH v2] Avoid including wait_event.h in wait_event_types.h Adding wait_classes.h to avoid including wait_event.h in wait_event_types.h (that produced a circular include). --- .../activity/generate-wait_event_types.pl | 2 +- src/include/utils/wait_classes.h | 29 +++ src/include/utils/wait_event.h| 17 ++- 3 files changed, 32 insertions(+), 16 deletions(-) create mode 100644 src/include/utils/wait_classes.h diff --git a/src/backend/utils/activity/generate-wait_event_types.pl b/src/backend/utils/activity/generate-wait_event_types.pl index 171bf2ae632..424ad9f115d 100644 --- a/src/backend/utils/activity/generate-wait_event_types.pl +++ b/src/backend/utils/activity/generate-wait_event_types.pl @@ -168,7 +168,7 @@ if ($gen_code) printf $h $header_comment, 'wait_event_types.h'; printf $h "#ifndef WAIT_EVENT_TYPES_H\n"; printf $h "#define WAIT_EVENT_TYPES_H\n\n"; - printf $h "#include \"utils/wait_event.h\"\n\n"; + printf $h "#include \"utils/wait_classes.h\"\n\n"; printf $c $header_comment, 'pgstat_wait_event.c'; diff --git a/src/include/utils/wait_classes.h b/src/include/utils/wait_classes.h new file mode 100644 index 000..b8e7b1f4c24 --- /dev/null +++ b/src/include/utils/wait_classes.h @@ -0,0 +1,29 @@ +/*- + * wait_classes.h + * Define the wait classes + * + * Copyright (c) 2001-2025, PostgreSQL Global Development Group + * + * src/include/utils/wait_classes.h + * -- + */ +#ifndef WAIT_CLASSES_H +#define WAIT_CLASSES_H + + +/* -- + * Wait Classes + * -- + */ +#define PG_WAIT_LWLOCK0x0100U +#define PG_WAIT_LOCK0x0300U +#define PG_WAIT_BUFFERPIN 0x0400U +#define PG_WAIT_ACTIVITY 0x0500U +#define PG_WAIT_CLIENT0x0600U +#define PG_WAIT_EXTENSION 0x0700U +#define PG_WAIT_IPC 0x0800U +#define PG_WAIT_TIMEOUT0x0900U +#define PG_WAIT_IO 0x0A00U +#define PG_WAIT_INJECTIONPOINT 0x0B00U + +#endif /* WAIT_CLASSES_H */ diff --git a/src/include/utils/wait_event.h b/src/include/utils/wait_event.h index b8cb3e5a430..7829bee5188 100644 --- a/src/include/utils/wait_event.h +++ b/src/include/utils/wait_event.h @@ -10,21 +10,8 @@ #ifndef WAIT_EVENT_H #define WAIT_EVENT_H - -/* -- - * Wait Classes - * -- - */ -#define PG_WAIT_LWLOCK0x0100U -#define PG_WAIT_LOCK0x0300U -#define PG_WAIT_BUFFERPIN 0x0400U -#define PG_WAIT_ACTIVITY 0x0500U -#define PG_WAIT_CLIENT0x0600U -#define PG_WAIT_EXTENSION 0x0700U -#define PG_WAIT_IPC 0x0800U -#define PG_WAIT_TIMEOUT0x0900U -#define PG_WAIT_IO 0x0A00U -#define PG_WAIT_INJECTIONPOINT 0x0B00U +/* wait classes */ +#include "utils/wait_classes.h" /* enums for wait events */ #include "utils/wait_event_types.h" -- 2.34.1
Re: Logical Replication of sequences
Hi Vignesh. FYI, patch v20250424-0004 reported whitespace errors when applied. [postgres@CentOS7-x64 oss_postgres_misc]$ git apply ../patches_misc/v20250424-0004-Enhance-sequence-synchronization-during-su.patch ../patches_misc/v20250424-0004-Enhance-sequence-synchronization-during-su.patch:366: trailing whitespace. * warning: 1 line adds whitespace errors. == Kind Regards, Peter Smith. Fujitsu Australia
Re: Fix premature xmin advancement during fast forward decoding
On Sat, Apr 26, 2025 at 12:01 AM Masahiko Sawada wrote: > > On Fri, Apr 25, 2025 at 4:42 AM Amit Kapila wrote: > > > > On Fri, Apr 25, 2025 at 10:46 AM Masahiko Sawada > > wrote: > > > > > > What I'm concerned about is the back branches. With this approach all > > > back branches will have such degradations and it doesn't make sense to > > > me to optimize SnapBuildCommitTxn() codes in back branches. > > > > > > > One possibility could be that instead of maintaining an entire > > snapshot in fast_forward mode, we can maintain snapshot's xmin in each > > ReorderBufferTXN. But then also, how would we get the minimum > > txns_by_base_snapshot_lsn as we are getting now in > > ReorderBufferGetOldestXmin? I think we need to traverse the entire > > list of txns to get it in fast_forward mode but that may not show up > > because it will not be done for each transaction. We can try such a > > thing, but it won't be clean to have fast_forward specific code and > > also it would be better to add such things only for HEAD. > > Agreed. > We may need to consider handling xmin in SnapBuildCommitTxn(), where we also set the base snapshot. > > Can you think of any better ideas? > > No idea. Hmm, there seems no reasonable way to fix this issue for back > branches. I consented to the view that these costs were something that > we should have paid from the beginning. > Right, I feel we should go with the simple change proposed by Hou-San for now to fix the bug. If, in the future, we encounter any cases where such optimizations can help for fast-forward mode, then we can consider it. Does that sound reasonable to you? -- With Regards, Amit Kapila.
Re: [BUG] temporary file usage report with extended protocol and unnamed portals
I found several issues with v4. It does not deal correctly with pipelining, and we should only really be concerned with dropping an unnamed portal only. So, v5 now moves the DropPortal call after the unnamed portal was executed to completion ( as v4 was doing ), but does so only in the case in which we are not inside a transaction-control statement or the portal was executing a command that can be run inside a transaction block. Also, I realize that explicit cursors ( DECLARE CURSOR ) will only log temp file at cursor close and in which case, the statement associated with the temp file logging is the CLOSE command: i.e. ``` 2025-04-26 18:46:38.084 UTC [10415] LOG: temporary file: path "base/pgsql_tmp/pgsql_tmp10415.0", size 1014030336 2025-04-26 18:46:38.084 UTC [10415] STATEMENT: close mycursor_1; ``` I don't think there is much we can do there, or should we. -- Sami Imseih v5-0001-Correct-timing-of-portal-drop-in-an-execute-messa.patch Description: Binary data
Re: Avoid circular header file dependency
Hi, On Sat, Apr 26, 2025 at 01:20:56AM -0400, Tom Lane wrote: > Bertrand Drouvot writes: > > While working on wait events I faced some compilation issues due to circular > > header file dependency (introduced in fa88928470b5) between wait_event.h and > > wait_event_types.h. > > I don't have an opinion about whether this > specific refactoring is the best way to deal with this case, but > I definitely feel that we mustn't allow the situation to persist. Thanks for sharing your thoughts! yeah, I'm not 100% sure that the proposed refactoring is the best way but it looks simple enough and allows wait_event_types.h to include "only" what it really needs. > > Out of curiosity, I ran clang-tidy with misc-header-include-cycle ([1]) and > > it > > also reports: > > ../src/pl/plpython/plpy_util.h:9:10: warning: circular header file > > dependency detected while including 'plpython.h' > > This one worries me less because plpy_util.h only contains simple external > > function declarations. > > Whatever it contains, we need to kill it with fire before the problem > metastasizes like it did the last time. (yeah, yeah, badly mixed > metaphors) I can take a look at this one over the weekend if nobody > beats me to it. I had a look at it, what do you think about 0002 attached? (Not 100% sure that's the best approach though). > I am very glad to hear that there's a readily available tool to > catch such cases. +1, and it's good to see that there are only 2 cases in the code base. > We ought to run it every so often. Yeah, also probably with readability-inconsistent-declaration-parameter-name that Peter used in [1]. How/where do we "automate" this kind of regular tasks? (I tried to find the answer in [2] but that thread is pretty long). [1]: https://www.postgresql.org/message-id/CAH2-WznJt9CMM9KJTMjJh_zbL5hD9oX44qdJ4aqZtjFi-zA3Tg%40mail.gmail.com [2]: https://www.postgresql.org/message-id/flat/20200812223409.6di3y2qsnvynao7a%40alap3.anarazel.de Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com >From 2156bf212ce7aabc40e2843c77d796249d12d4f9 Mon Sep 17 00:00:00 2001 From: Bertrand Drouvot Date: Thu, 24 Apr 2025 17:11:03 + Subject: [PATCH v1 1/2] Avoid including wait_event.h in wait_event_types.h Adding wait_class_constants.h to avoid including wait_event.h in wait_event_types.h (that produced a circular include). --- .../activity/generate-wait_event_types.pl | 2 +- src/include/utils/wait_class_constants.h | 29 +++ src/include/utils/wait_event.h| 17 ++- 3 files changed, 32 insertions(+), 16 deletions(-) create mode 100644 src/include/utils/wait_class_constants.h diff --git a/src/backend/utils/activity/generate-wait_event_types.pl b/src/backend/utils/activity/generate-wait_event_types.pl index 171bf2ae632..52b6b7b2ae6 100644 --- a/src/backend/utils/activity/generate-wait_event_types.pl +++ b/src/backend/utils/activity/generate-wait_event_types.pl @@ -168,7 +168,7 @@ if ($gen_code) printf $h $header_comment, 'wait_event_types.h'; printf $h "#ifndef WAIT_EVENT_TYPES_H\n"; printf $h "#define WAIT_EVENT_TYPES_H\n\n"; - printf $h "#include \"utils/wait_event.h\"\n\n"; + printf $h "#include \"utils/wait_class_constants.h\"\n\n"; printf $c $header_comment, 'pgstat_wait_event.c'; diff --git a/src/include/utils/wait_class_constants.h b/src/include/utils/wait_class_constants.h new file mode 100644 index 000..286f9c2daec --- /dev/null +++ b/src/include/utils/wait_class_constants.h @@ -0,0 +1,29 @@ +/*- + * wait_class_constants.h + * Constants related to wait classes + * + * Copyright (c) 2001-2025, PostgreSQL Global Development Group + * + * src/include/utils/wait_class_constants.h + * -- + */ +#ifndef WAIT_CLASS_CONSTANTS_H +#define WAIT_CLASS_CONSTANTS_H + + +/* -- + * Wait Classes + * -- + */ +#define PG_WAIT_LWLOCK0x0100U +#define PG_WAIT_LOCK0x0300U +#define PG_WAIT_BUFFERPIN 0x0400U +#define PG_WAIT_ACTIVITY 0x0500U +#define PG_WAIT_CLIENT0x0600U +#define PG_WAIT_EXTENSION 0x0700U +#define PG_WAIT_IPC 0x0800U +#define PG_WAIT_TIMEOUT0x0900U +#define PG_WAIT_IO 0x0A00U +#define PG_WAIT_INJECTIONPOINT 0x0B00U + +#endif /* WAIT_CLASS_CONSTANTS_H */ diff --git a/src/include/utils/wait_event.h b/src/include/utils/wait_event.h index b8cb3e5a430..c3c1ce6ef2e 100644 --- a/src/include/utils/wait_event.h +++ b/src/include/utils/wait_event.h @@ -10,21 +10,8 @@ #ifndef WAIT_EVENT_H #define WAIT_EVENT_H - -/* -- - * Wait Classes - * -- - */ -#define PG_WAIT_LWLOCK0x0100U -#define PG_WAIT_LOCK0x0300U -#define PG_WAIT_BUFFERPIN 0x0400U -#define PG_WAIT_ACTIVITY 0x0500U -#define PG_WAIT_CLIENT0x0600U -#define PG_WAIT_EXTENSION 0x0700U
Re: Avoid circular header file dependency
Bertrand Drouvot writes: > On Sat, Apr 26, 2025 at 01:20:56AM -0400, Tom Lane wrote: >> Whatever it contains, we need to kill it with fire before the problem >> metastasizes like it did the last time. (yeah, yeah, badly mixed >> metaphors) I can take a look at this one over the weekend if nobody >> beats me to it. > I had a look at it, what do you think about 0002 attached? (Not 100% sure > that's the best approach though). After looking at this, I think the actual problem is that plpython.h does this: /* * Used throughout, so it's easier to just include it everywhere. */ #include "plpy_util.h" which is exactly the sort of cowboy modularity violation that hurts when you have to clean it up. Taking that out requires having to manually include plpy_util.h in all the relevant .c files, but on the other hand we can remove their vestigial direct inclusions of plpython.h. It was always pretty silly to #include that after including some plpy_foo.h files, so let's stop doing so. The attached patch therefore boils down in most places to s/plpython.h/plpy_util.h/. (A small number of these files still compiled without that, indicating that they're not actually using plpy_util.h today. But I figured we might as well just do it uniformly.) regards, tom lane diff --git a/contrib/hstore_plpython/hstore_plpython.c b/contrib/hstore_plpython/hstore_plpython.c index 8812fb3f3e4..e2bfc6da38e 100644 --- a/contrib/hstore_plpython/hstore_plpython.c +++ b/contrib/hstore_plpython/hstore_plpython.c @@ -3,7 +3,7 @@ #include "fmgr.h" #include "hstore/hstore.h" #include "plpy_typeio.h" -#include "plpython.h" +#include "plpy_util.h" PG_MODULE_MAGIC_EXT( .name = "hstore_plpython", diff --git a/contrib/jsonb_plpython/jsonb_plpython.c b/contrib/jsonb_plpython/jsonb_plpython.c index 680445a006f..9383615abbf 100644 --- a/contrib/jsonb_plpython/jsonb_plpython.c +++ b/contrib/jsonb_plpython/jsonb_plpython.c @@ -2,7 +2,7 @@ #include "plpy_elog.h" #include "plpy_typeio.h" -#include "plpython.h" +#include "plpy_util.h" #include "utils/fmgrprotos.h" #include "utils/jsonb.h" #include "utils/numeric.h" diff --git a/contrib/ltree_plpython/ltree_plpython.c b/contrib/ltree_plpython/ltree_plpython.c index ba5926b8be6..0493aeb2423 100644 --- a/contrib/ltree_plpython/ltree_plpython.c +++ b/contrib/ltree_plpython/ltree_plpython.c @@ -2,7 +2,7 @@ #include "fmgr.h" #include "ltree/ltree.h" -#include "plpython.h" +#include "plpy_util.h" PG_MODULE_MAGIC_EXT( .name = "ltree_plpython", diff --git a/src/pl/plpython/plpy_cursorobject.c b/src/pl/plpython/plpy_cursorobject.c index 1c6be756120..37d7efca77c 100644 --- a/src/pl/plpython/plpy_cursorobject.c +++ b/src/pl/plpython/plpy_cursorobject.c @@ -16,7 +16,7 @@ #include "plpy_planobject.h" #include "plpy_resultobject.h" #include "plpy_spi.h" -#include "plpython.h" +#include "plpy_util.h" #include "utils/memutils.h" static PyObject *PLy_cursor_query(const char *query); diff --git a/src/pl/plpython/plpy_elog.c b/src/pl/plpython/plpy_elog.c index 70de5ba13d7..ddf3573f0e7 100644 --- a/src/pl/plpython/plpy_elog.c +++ b/src/pl/plpython/plpy_elog.c @@ -10,7 +10,7 @@ #include "plpy_elog.h" #include "plpy_main.h" #include "plpy_procedure.h" -#include "plpython.h" +#include "plpy_util.h" PyObject *PLy_exc_error = NULL; PyObject *PLy_exc_fatal = NULL; diff --git a/src/pl/plpython/plpy_exec.c b/src/pl/plpython/plpy_exec.c index 00747bb811b..28fbd443b98 100644 --- a/src/pl/plpython/plpy_exec.c +++ b/src/pl/plpython/plpy_exec.c @@ -17,7 +17,7 @@ #include "plpy_main.h" #include "plpy_procedure.h" #include "plpy_subxactobject.h" -#include "plpython.h" +#include "plpy_util.h" #include "utils/fmgrprotos.h" #include "utils/rel.h" diff --git a/src/pl/plpython/plpy_main.c b/src/pl/plpython/plpy_main.c index 8f56155f006..f36eadbadc6 100644 --- a/src/pl/plpython/plpy_main.c +++ b/src/pl/plpython/plpy_main.c @@ -18,7 +18,7 @@ #include "plpy_plpymodule.h" #include "plpy_procedure.h" #include "plpy_subxactobject.h" -#include "plpython.h" +#include "plpy_util.h" #include "utils/guc.h" #include "utils/memutils.h" #include "utils/rel.h" diff --git a/src/pl/plpython/plpy_planobject.c b/src/pl/plpython/plpy_planobject.c index 3e38e5e..6044893afdd 100644 --- a/src/pl/plpython/plpy_planobject.c +++ b/src/pl/plpython/plpy_planobject.c @@ -9,7 +9,7 @@ #include "plpy_cursorobject.h" #include "plpy_planobject.h" #include "plpy_spi.h" -#include "plpython.h" +#include "plpy_util.h" #include "utils/memutils.h" static void PLy_plan_dealloc(PLyPlanObject *self); diff --git a/src/pl/plpython/plpy_plpymodule.c b/src/pl/plpython/plpy_plpymodule.c index ea06d9a52b1..1f980b44b2a 100644 --- a/src/pl/plpython/plpy_plpymodule.c +++ b/src/pl/plpython/plpy_plpymodule.c @@ -14,7 +14,7 @@ #include "plpy_resultobject.h" #include "plpy_spi.h" #include "plpy_subxactobject.h" -#include "plpython.h" +#include "plpy_util.h" #include "utils/builtins.h" HT
Re: Removing unneeded self joins
On Sat, Apr 26, 2025 at 11:04 PM Alexander Korotkov wrote: > On Sun, Apr 6, 2025 at 12:02 AM Alena Rybakina > wrote: > > Should we add more regression tests covering these cases? > > > > I experimented with some examples like this and noticed that it does affect > > cardinality estimation, though I'm not sure the impact is significant. > > I used the tables from the regression tests, so if they’re appropriate for > > reproducing this case, it should be straightforward to add them. > > Thank you for your feedback. I've check the cases you've provided. I > found that the differences here are related to the SJE itself, not to > changes regarding PHVs handling. I think it generally OK that > estimates are somewhat changed due to such significant query > transformation. Hopefully they should be improved in the majority of > cases. > > I did some improvements to PHVs patch: revised comments and commit > message. I'm going to push it if no objections. Uh, v2 was there already. That should be v3. -- Regards, Alexander Korotkov Supabase v3-0001-Disallow-removing-placeholders-during-Self-Join-E.patch Description: Binary data
Re: Removing unneeded self joins
Hi, Alena! On Sun, Apr 6, 2025 at 12:02 AM Alena Rybakina wrote: > Should we add more regression tests covering these cases? > > I experimented with some examples like this and noticed that it does affect > cardinality estimation, though I'm not sure the impact is significant. > I used the tables from the regression tests, so if they’re appropriate for > reproducing this case, it should be straightforward to add them. Thank you for your feedback. I've check the cases you've provided. I found that the differences here are related to the SJE itself, not to changes regarding PHVs handling. I think it generally OK that estimates are somewhat changed due to such significant query transformation. Hopefully they should be improved in the majority of cases. I did some improvements to PHVs patch: revised comments and commit message. I'm going to push it if no objections. -- Regards, Alexander Korotkov Supabase v2-0001-Disallow-removing-placeholders-during-Self-Join-E.patch Description: Binary data
Re: Get rid of integer divide in FAST_PATH_REL_GROUP() macro
On 4/14/25 04:09, David Rowley wrote: > I noticed a while ago that the new fast-path locking code uses integer > division to figure out the fast-path locking group slot. To me, this > seems a bit unnecessary as FastPathLockGroupsPerBackend is always a > power-of-two value, so we can use bitwise-AND instead. > > I don't think FAST_PATH_REL_GROUP() is in any particularly hot code > paths, but still, having the divide in there isn't sitting well with > me. Can we get rid of it? > Yes, we can get rid of the divide - if we assume power-of-two value (which seems fine, we already do that, IIRC). > I've attached a patch for that. I also adjusted the method used to > calculate FastPathLockGroupsPerBackend. Also, the Assert that was > going on at the end of the loop in InitializeFastPathLocks() looked a > little odd as it seems to be verifying something that the loop > condition was checking already. I thought it was better to check that > we end up with a power-of-two. > > Please see the attached patch. > Thanks. Those changes seem fine to me to. Do you intend to push these, or do you want me to do it? regards -- Tomas Vondra
Re: pgsql: Add function to get memory context stats for processes
On 4/23/25 20:14, Tom Lane wrote: > Robert Haas writes: >> My primary concern about the patch is that >> ProcessGetMemoryContextInterrupt() can be called from any >> CHECK_FOR_INTERRUPTS() and calls lots of DSA functions, including >> dsa_create() and, via PublishMemoryContext(), dsa_allocate0(). I'm >> shocked to hear that you and Andres think that's safe to do at any >> current or future CHECK_FOR_INTERRUPTS() anywhere in the code; but >> Andres seems very confident that it's fine, so perhaps I should just >> stop worrying and be happy that we have the feature. > > Just for the record, it sounds quite unsafe to me too. I could > credit it being all right to examine the process' MemoryContext data > structures, but calling dsa_create() from CFI seems really insane. > Way too many moving parts there. > Maybe I'm oblivious to some very obvious issues, but why is this so different from everything else that is already called from ProcessInterrupts()? Perhaps dsa_create() is more complex compared to the other stuff (haven't checked), but why would it be unsafe? The one risk I can think of is a risk of recursion - if any of the functions called from ProcessGetMemoryContextInterrupt() does CFI, could that be a problem if there's another pending signal. I see some other handlers (e.g. ProcessParallelApplyMessages) handle this by explicitly holding interrupts. Should ProcessGetMemoryContextInterrupt() do the same thing? In any case, if DSA happens to not be the right way to transfer this, what should we use instead? The only thing I can think of is some sort of pre-allocated chunk of shared memory. regards -- Tomas Vondra
Re: Does RENAME TABLE rename associated identity sequence?
Hi all, Thank you for the detailed discussion and clarifications. I sincerely appreciate everyone's time and valuable insights shared in this discussion.
Re: Fix slot synchronization with two_phase decoding enabled
On Fri, Apr 25, 2025 at 9:57 PM Masahiko Sawada wrote: > > On Fri, Apr 25, 2025 at 3:43 AM Amit Kapila wrote: > > > > On Fri, Apr 25, 2025 at 6:02 AM Masahiko Sawada > > wrote: > > > > > > I realized that users who create a logical slot using > > > pg_create_logical_replication_slot() would not be able to enable both > > > options at slot creation, and there is no easy way to enable the > > > failover after two_phase-enabled-slot creation. Users would need to > > > use ALTER_REPLICATION_SLOT replication command, which seems > > > unrealistics for users to use. On the other hand, if we allow creating > > > a logical slot with enabling failover and two_phase using SQL API, > > > there is still a chance for this bug to occur. Would it be worth > > > considering that if a logical slot is created with enabling failover > > > and two_phase using SQL API, we create the slot with only > > > two_phase=true, then advance the slot until the slot satisfies > > > restart_lsn >= two_phase_at, and then enable the failover? > > > > > > > This means we either need to maintain somewhere that user has provided > > failover flag till restart_lsn >= two_phase_at or and then set > > failover flag in the slot > > I was thinking of this idea. > > > or initially mark it but enable the > > functionality of failover when we reach the condition restart_lsn >= > > two_phase_at. > > IIUC the slot could be synchronized to the standby as soon as we > complete DecodingContextFindStartpoint() for a failover-enabled slot. > So we would need some mechanisms to make sure that the slot is not > synchronized while we're waiting to reach the condition restart_lsn >= > two_phase_at even if the failover is enabled. > So, then we need any state or persistent flag for this. > > Both seem to have different kinds of problems. The first > > idea seems to have an issue with persistence, which means we can lose > > track of the flag after the restart. > > I think we can do this series of operations while the slot is not > persistent, that is the slot is still RS_EPHEMERAL. > But we still need a persistent flag to indicate such slots shouldn't be synced to standby till we reach the condition restart_lsn >= two_phase_at. > > The second can mislead the user > > for a long period in cases where prepare and commit have a large time > > gap. I feel this will introduce complexity either in the form of code > > or in giving the information to the user. > > Agreed. Both ways introduce complexity so we need to consider the > user-unfriendliness (by not having a proper way to enable failover for > the two_phase-enabled-slot using SQL API) vs. risk (of introducing > complexity). > Right, to me it sounds risky to provide such functionality for SQL API in the back branch. -- With Regards, Amit Kapila.
Re: Back-patch of: avoid multiple hard links to same WAL file after a crash
On Fri, Apr 25, 2025 at 03:35:06PM -0400, Andres Freund wrote: > On 2025-04-20 14:53:39 -0700, Noah Misch wrote: > > The checkpoints and WAL creation took 30s, but archiving was only 20% done > > (based on file name 0001006D) at the 360s PGCTLTIMEOUT. > > Huh. That seems surprisingly slow, even for valgrind. I guess it's one more > example for why the single-threaded archiving approach sucks so badly :) Yes! I also didn't expect that v14/v13 would run much faster. > I just changed the config to --trace-children=no. There already is a valgrind > run in progress, so it won't be in effect for the next run. Works for me. I see that resolved things.
Re: Get rid of integer divide in FAST_PATH_REL_GROUP() macro
On Sun, 27 Apr 2025 at 08:44, Tomas Vondra wrote: > Thanks. Those changes seem fine to me to. Thanks for checking. > Do you intend to push these, or do you want me to do it? I made a few tweaks to the comments and pushed. David
