Re: run pgindent on a regular basis / scripted manner
On 2023-04-28 Fr 05:25, Alvaro Herrera wrote:
On 2023-Feb-05, Andrew Dunstan wrote:
So here's a diff made from running perltidy v20221112 with the additional
setting --valign-exclusion-list=", = => || && if unless"
I ran this experimentally with perltidy 20230309, and compared that with
the --novalign behavior (not to propose the latter -- just to be aware
of what else is vertical alignment doing.)
Thanks for looking.
Based on the differences between both, I think we'll definitely want to
include =~ and |= in this list, and I think we should discuss whether to
also include "or" (for "do_stuff or die()" type of constructs) and "qw"
(mainly used in 'use Foo qw(one two)' import lists). All these have
effects (albeit smaller than the list you gave) on our existing code.
I'm good with all of these I think
If you change from an exclusion list to --novalign then you lose
alignment of trailing # comments, which personally I find a loss, even
though they're still a multi-line effect. Another change would be that
it ditches alignment of "{" but that only changes msvc/Install.pm, so I
think we shouldn't worry; and then there's this one:
-use PostgreSQL::Test::Utils ();
+use PostgreSQL::Test::Utils ();
use PostgreSQL::Test::BackgroundPsql ();
which I think we could just change to qw() if we cared enough (but I bet
we don't).
Yeah, me too.
All in all, I think sticking to
--valign-exclusion-list=", = => =~ |= || && if or qw unless"
is a good deal.
wfm
cheers
andrew
--
Andrew Dunstan
EDB:https://www.enterprisedb.com
Re: run pgindent on a regular basis / scripted manner
On 2023-04-28 Fr 14:08, Bruce Momjian wrote: On Wed, Apr 26, 2023 at 03:44:47PM -0400, Andrew Dunstan wrote: On 2023-04-26 We 09:27, Tom Lane wrote: I doubt there's something like that. You can freeze arbitrary blocks of code like this (from the manual) #<<< format skipping: do not let perltidy change my nice formatting my @list = (1, 1, 1, 1, 2, 1, 1, 3, 3, 1, 1, 4, 6, 4, 1,); #>>> But that gets old and ugly pretty quickly. Can those comments be added by a preprocessor before calling perltidy, and then removed on completion? I imagine so, but we'd need a way of determining algorithmically which lines to protect. That might not be at all simple. And then we'd have the maintenance burden of the preprocessor. cheers andrew -- Andrew Dunstan EDB:https://www.enterprisedb.com
Re: run pgindent on a regular basis / scripted manner
Andrew Dunstan writes: > On 2023-04-28 Fr 14:08, Bruce Momjian wrote: >> Can those comments be added by a preprocessor before calling perltidy, >> and then removed on completion? > I imagine so, but we'd need a way of determining algorithmically which > lines to protect. That might not be at all simple. And then we'd have > the maintenance burden of the preprocessor. Yeah, it's hard to see how you'd do that without writing a full Perl parser. regards, tom lane
Re: Possible regression setting GUCs on \connect
On 4/28/23 12:29 PM, Pavel Borisov wrote: Hi! On Fri, 28 Apr 2023 at 17:42, Jonathan S. Katz wrote: On 4/27/23 8:04 PM, Alexander Korotkov wrote: On Fri, Apr 28, 2023 at 2:30 AM Alexander Korotkov wrote: Additionally, I think if we start recording role OID, then we need a full set of management clauses for each individual option ownership. Otherwise, we would leave this new role OID without necessarily management facilities. But with them, the whole stuff will look like awful overengineering. I can also predict a lot of ambiguous cases. For instance, we existing setting can be overridden with a different role OID. If it has been overridden can the overwriter turn it back? [RMT hat] While the initial bug has been fixed, given there is discussion on reverting 096dd80f3, I've added this as an open item. I want to study this a bit more before providing my own opinion on revert. I see that 096dd80f3 is a lot simpler in implementation than a0ffa885e, so I agree Alexander's opinion that it's good not to overengineer what could be done simple. If we patched corner cases of a0ffa885e before (by 13d838815), why not patch minor things in 096dd80f3 instead of reverting? As I see in [1] there is some demand from users regarding this option. [RMT hat] I read through the original thread[1] to understand the use case and also the concerns, but I need to study [1] and this thread a bit more before I can form an opinion. The argument that there is "demand from users" is certainly one I relate to, but there have been high-demand features in the past (e.g. MERGE, SQL/JSON) that have been reverted and released later due to various concerns around implementation, etc. The main job of the RMT is to ensure a major release is on time and is as stable as possible, which will be a major factor into any decisions if there is lack of community consensus on an open item. Thanks, Jonathan [1] https://postgr.es/m/CAGRrpzawQSbuEedicOLRjQRCmSh6nC3HeMNvnQdBVmPMg7AvQw%40mail.gmail.com OpenPGP_signature Description: OpenPGP digital signature
Re: [PATCH] Clarify the behavior of the system when approaching XID wraparound
On Sat, Apr 29, 2023 at 7:30 PM John Naylor wrote: > How about > > -HINT: To avoid a database shutdown, [...] > +HINT: To prevent XID exhaustion, [...] > > ...and "MXID", respectively? We could explain in the docs that vacuum and > read-only queries still work "when XIDs have been exhausted", etc. I think that that particular wording works in this example -- we *are* avoiding XID exhaustion. But it still doesn't really address my concern -- at least not on its own. I think that we need a term for xidStopLimit mode (and perhaps multiStopLimit) itself. This is a discrete state/mode that is associated with a specific mechanism. I'd like to emphasize the purpose of xidStopLimit (over when xidStopLimit happens) in choosing this user-facing name. As you know, the point of xidStopLimit mode is to give autovacuum the chance to catch up with managing the XID space through freezing: the available supply of XIDs doesn't meet present demand, and hasn't for some time, so it finally came to this. Even if we had true 64-bit XIDs we'd probably still need something similar -- there would still have to be *some* point that allowing the "freezing deficit" to continue to grow just wasn't tenable. If a person consistently spends more than they take in, their "initial bankroll" isn't necessarily relevant. If our ~2.1 billion XID "bankroll" wasn't enough to avoid xidStopLimit, why would we expect 8 billion or 20 billion XIDs to have been enough? I'm thinking of a user-facing name for xidStopLimit along the lines of "emergency XID allocation restoration mode" (admittedly that's quite a mouthful). Something that carries the implication of "imbalance". The system was configured in a way that turned out to be unsustainable. The system was therefore forced to "restore sustainability" using the only tool that remained. This is closely related to the failsafe. As bad as xidStopLimit is, it won't always be the end of the world -- much depends on individual application requirements. > (I should probably also add in the commit message that the "shutdown" in the > message was carried over to MXIDs when they arrived also in 2005). > > > Separately, there is a need to update a couple of other places to use > > this new terminology. The documentation for vacuum_sailsafe_age and > > vacuum_multixact_failsafe_age refer to "system-wide transaction ID > > wraparound failure", which seems less than ideal, even without your > > patch. > > Right, I'll have a look. As you know, there is a more general problem with the use of the term "wraparound" in the docs, and in the system itself (in places like pg_stat_activity). Even the very basic terminology in this area is needlessly scary. Terms like "VACUUM (to prevent wraparound)" are uncomfortably close to "a race against time to avoid data corruption". The system isn't ever supposed to corrupt data, even if misconfigured (unless the misconfiguration is very low-level, such as "fsync=off"). Users should be able to take that much for granted. I don't expect either of us to address that problem in the short term -- the term "wraparound" is too baked-in for it to be okay to just remove it overnight. But, it could still make sense for your patch (or my own) to fully own the fact that "wraparound" is actually a misnomer. At least when used in contexts like "to prevent wraparound" (xidStopLimit actually "prevents wraparound", though we shouldn't say anything about it in a place of prominence). Let's reassure users that they should continue to take "we won't corrupt your data for no good reason" for granted. > I think the docs would do well to have ordered steps for recovering from both > XID and MXID exhaustion. I had planned to address this with my ongoing work on the "Routine Vacuuming" docs, but I think that you're right about the necessity of addressing it as part of this patch. These extra steps will be required whenever the problem is a leaked prepared transaction, or something along those lines. That is increasingly likely to turn out to be the underlying cause of entering xidStopLimit, given the work we've done on VACUUM over the years. I still think that "imbalance" is the right way to frame discussion of xidStopLimit. After all, autovacuum/VACUUM will still spin its wheels in a futile effort to "restore balance". So it's kinda still about restoring imbalance IMV. -- Peter Geoghegan
Re: Direct I/O
On Sun, Apr 30, 2023 at 6:35 PM Thomas Munro wrote:
> On Sun, Apr 30, 2023 at 4:11 PM Noah Misch wrote:
> > Speaking of the developer-only status, I find the io_direct name more
> > enticing
> > than force_parallel_mode, which PostgreSQL renamed due to overuse from
> > people
> > expecting non-developer benefits. Should this have a name starting with
> > debug_?
>
> Hmm, yeah I think people coming from other databases would be tempted
> by it. But, unlike the
> please-jam-a-gather-node-on-top-of-the-plan-so-I-can-debug-the-parallel-executor
> switch, I think of this thing more like an experimental feature that
> is just waiting for more features to make it useful. What about a
> warning message about that at startup if it's on?
Something like this? Better words welcome.
$ ~/install//bin/postgres -D pgdata -c io_direct=data
2023-05-01 09:44:37.460 NZST [99675] LOG: starting PostgreSQL 16devel
on x86_64-unknown-freebsd13.2, compiled by FreeBSD clang version
14.0.5 (https://github.com/llvm/llvm-project.git
llvmorg-14.0.5-0-gc12386ae247c), 64-bit
2023-05-01 09:44:37.460 NZST [99675] LOG: listening on IPv6 address
"::1", port 5432
2023-05-01 09:44:37.460 NZST [99675] LOG: listening on IPv4 address
"127.0.0.1", port 5432
2023-05-01 09:44:37.461 NZST [99675] LOG: listening on Unix socket
"/tmp/.s.PGSQL.5432"
2023-05-01 09:44:37.463 NZST [99675] WARNING: io_direct is an
experimental setting for developer testing only
2023-05-01 09:44:37.463 NZST [99675] HINT: File I/O may be
inefficient or not work on some file systems.
2023-05-01 09:44:37.465 NZST [99678] LOG: database system was shut
down at 2023-05-01 09:43:51 NZST
2023-05-01 09:44:37.468 NZST [99675] LOG: database system is ready to
accept connections
From a9005129c939a8298c6668645588b2a8ef5064b6 Mon Sep 17 00:00:00 2001
From: Thomas Munro
Date: Mon, 1 May 2023 09:46:35 +1200
Subject: [PATCH] Log a warning about io_direct at startup time.
We've documented io_direct as an experimental developer-only feature,
but that documentation might be hard to find. Let's also display a
warning about that in the server log.
Later proposals will provide the infrastructure to use it efficiently,
but by releasing this switch earlier we can learn about direct I/O
quirks in systems in the wild.
Discussion: https://postgr.es/m/20230430041106.GA2268796%40rfd.leadboat.com
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index 4c49393fc5..8f5c03fa46 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -1432,6 +1432,12 @@ PostmasterMain(int argc, char *argv[])
errhint("Set the LC_ALL environment variable to a valid locale.")));
#endif
+ /* Temporary warning about experimental status of direct I/O support. */
+ if (io_direct_flags != 0)
+ ereport(WARNING,
+(errmsg("io_direct is an experimental setting for developer testing only"),
+ errhint("File I/O may be inefficient or not work on some file systems.")));
+
/*
* Remember postmaster startup time
*/
--
2.40.1
Re: Direct I/O
On Sun, Apr 30, 2023 at 06:35:30PM +1200, Thomas Munro wrote: > On Sun, Apr 30, 2023 at 4:11 PM Noah Misch wrote: > > Speaking of the developer-only status, I find the io_direct name more > > enticing > > than force_parallel_mode, which PostgreSQL renamed due to overuse from > > people > > expecting non-developer benefits. Should this have a name starting with > > debug_? > > Hmm, yeah I think people coming from other databases would be tempted > by it. But, unlike the > please-jam-a-gather-node-on-top-of-the-plan-so-I-can-debug-the-parallel-executor > switch, I think of this thing more like an experimental feature that > is just waiting for more features to make it useful. What about a > warning message about that at startup if it's on? Such a warning wouldn't be particularly likely to be seen by someone who already didn't read/understand the docs for the not-feature that they turned on. Since this is -currently- a developer-only feature, it seems reasonable to rename the GUC to debug_direct_io, and (at such time as it's considered to be helpful to users) later rename it to direct_io. That avoids the issue that random advice to enable direct_io=x under v17+ is applied by people running v16. +0.8 to do so. Maybe in the future, it should be added to GUC_EXPLAIN, too ? -- Justin
Re: Autogenerate some wait events code and documentation
On Fri, Apr 28, 2023 at 02:29:13PM +0200, Drouvot, Bertrand wrote:
> On 4/27/23 8:13 AM, Michael Paquier wrote:
>> Generating the contents of Lock would mean to gather in a single file
>> the data for the generation of LockTagType in lock.h, the list of
>> LockTagTypeNames in lockfuncs.c and the description of the docs. This
>> data being spread across three files is not really appealing to make
>> that generated.. LWLocks would mean to either extend lwlocknames.txt
>> with the description from the docs if we were to centralize the whole
>> thing.
>>
>> But do we need to merge more data than necessary? We could do things
>> in the simplest fashion possible while making the docs and code
>> user-friendly in the ordering: just add a section for Lock and LWLocks
>> in waiteventnames.txt with an extra comment in their headers and/or
>> data files to tell that waiteventnames.txt also needs a refresh.
>
> Agree that it would fix the doc ordering and that we could do that.
Not much a fan of the part where a full paragraph of the SGML docs is
added to the .txt, particularly with the new handling for "Notes".
I'd rather shape the perl script to be minimalistic and simpler, even
if it means moving this paragraph about LWLocks after all the tables
are generated.
Do we also need the comments in the generated header as well? My
initial impression was to just move these as comments of the .txt file
because that's where the new events would be added, as the .txt is the
main point of reference.
> It's done that way in V6.
>
> There is already comments about this in lockfuncs.c and lwlocknames.txt, so
> V6 updates those comments accordingly.
>
> Right, done that way in V6.
>
> Please note that it creates 2 new "wait events":
> WAIT_EVENT_EXTENSION and WAIT_EVENT_BUFFER_PIN.
Noted. Makes sense here.
> Then, they replace PG_WAIT_EXTENSION and PG_WAIT_BUFFER_PIN (resp.) where
> appropriate.
So, the change here..
+ # Exception here
+ if ($last =~ /^BufferPin/)
+ {
+ $last = "Buffer_Pin";
+ }
.. Implies the two following changes:
typedef enum
{
- WAIT_EVENT_BUFFER_PIN = PG_WAIT_BUFFER_PIN
+ WAIT_EVENT_BUFFER_PIN = PG_WAIT_BUFFERPIN
} WaitEventBufferPin;
[...]
static const char *
-pgstat_get_wait_buffer_pin(WaitEventBufferPin w)
+pgstat_get_wait_bufferpin(WaitEventBufferPin w)
I would be OK to remove this exception in the script as it does not
change anything for the end user (the wait event string is still
reported as "BufferPin"). This way, we keep things simpler in the
script. This has as extra consequence to require a change in
wait_event.h so as PG_WAIT_BUFFER_PIN is renamed to PG_WAIT_BUFFERPIN,
equally fine by me. Logically, this rename should be done in a patch
of its own, for clarity.
@@ -185,6 +193,7 @@ distprep:
$(MAKE) -C utilsdistprep
$(MAKE) -C utils/adtjsonpath_gram.c jsonpath_gram.h jsonpath_scan.c
$(MAKE) -C utils/misc guc-file.c
+ $(MAKE) -C utils/actvitywait_event_types.h pgstat_wait_event.c
Incorrect order, and incorrect name (s/actvity/activity/, lacking an
'i').
+printf $h $header_comment, 'wait_event_types.h';
+printf $h "#ifndef WAITEVENTNAMES_H\n";
+printf $h "#define WAITEVENTNAMES_H\n\n";
Inconsistency detected here.
It seems to me that we'd better have a .gitignore in utils/activity/
for the new files.
@@ -237,7 +237,7 @@ autoprewarm_main(Datum main_arg)
(void) WaitLatch(MyLatch,
WL_LATCH_SET | WL_EXIT_ON_PM_DEATH,
-1L,
-PG_WAIT_EXTENSION);
+WAIT_EVENT_EXTENSION);
Perhaps this should also be part of a first, separate patch, with the
introduction of the new pgstat_get_wait_extension/bufferpin()
functions. Okay, it is not a big deal because the main patch
generates the enum for extensions which would be used here, but for
the sake of history clarity I'd rather refactor and rename all that
first.
The choices of LWLOCK and LOCK for the internal names was a bit
surprising, while we can be consistent with the rest and use "LWLock"
and "Lock".
Attached is a v7 with the portions I have adjusted, which is mostly OK
by me at this point. We are still away from the next CF, but I'll
look at that again when the v17 branch opens.
--
Michael
From d6a5c46411b5506588c77ebe52310a324e99033f Mon Sep 17 00:00:00 2001
From: Michael Paquier
Date: Mon, 1 May 2023 08:51:05 +0900
Subject: [PATCH v7] Generating wait_event_types.h, pgstat_wait_event.c and
waiteventnames.sgml
Purpose is to auto-generate those files based on the newly created waiteventnames.txt file.
Having auto generated files would help to avoid:
- wait event without description like observed in https://www.postgresql.org/message-id/CA%2BhUKGJixAHc860Ej9Qzd_z96Z6aoajAgJ18bYfV3Lfn6t9%3D%2BQ%40mail.gmail.com
- orphaned wait event like observed in https://www.postgresql.org/message-id/CA%2BhUKGK6tqm59KuF1z%2Bh5Y8fsWcu5v8%2B84kduSHwRzwjB2aa_A%40mail.g
Re: Direct I/O
Justin Pryzby writes: > On Sun, Apr 30, 2023 at 06:35:30PM +1200, Thomas Munro wrote: >> What about a >> warning message about that at startup if it's on? > Such a warning wouldn't be particularly likely to be seen by someone who > already didn't read/understand the docs for the not-feature that they > turned on. Yeah, I doubt that that would be helpful at all. > Since this is -currently- a developer-only feature, it seems reasonable > to rename the GUC to debug_direct_io, and (at such time as it's > considered to be helpful to users) later rename it to direct_io. +1 regards, tom lane
Re: Direct I/O
On Mon, May 1, 2023 at 12:00 PM Tom Lane wrote:
> Justin Pryzby writes:
> > On Sun, Apr 30, 2023 at 06:35:30PM +1200, Thomas Munro wrote:
> >> What about a
> >> warning message about that at startup if it's on?
>
> > Such a warning wouldn't be particularly likely to be seen by someone who
> > already didn't read/understand the docs for the not-feature that they
> > turned on.
>
> Yeah, I doubt that that would be helpful at all.
Fair enough.
> > Since this is -currently- a developer-only feature, it seems reasonable
> > to rename the GUC to debug_direct_io, and (at such time as it's
> > considered to be helpful to users) later rename it to direct_io.
>
> +1
Yeah, the future cross-version confusion thing is compelling. OK,
here's a rename patch. I left all the variable names and related
symbols as they were, so it's just the GUC that gains the prefix. I
moved the documentation hunk up to be sorted alphabetically like
nearby entries, because that seemed to look nicer, even though the
list isn't globally sorted.
From f95fc08c47c7d429950e8f22ce93c35271adfb37 Mon Sep 17 00:00:00 2001
From: Thomas Munro
Date: Mon, 1 May 2023 14:03:30 +1200
Subject: [PATCH] Rename io_direct to debug_io_direct.
Give the new GUC introduced by d4e71df6 a name that is clearly not
intended for mainstream use quite yet.
Future proposals would drop the prefix only after adding infrastructure
to make it efficient. Having the switch in the tree sooner is good
because it might lead to new discoveries about the hazards awaiting us
on a wide range of systems, but that name was too enticing and could
lead to cross-version confusion in future, per complaints from Noah and
Justin.
Suggested-by: Noah Misch
Reviewed-by: Justin Pryzby
Reviewed-by: Tom Lane
Discussion: https://postgr.es/m/20230430041106.GA2268796%40rfd.leadboat.com
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index b56f073a91..4f4b5b0b74 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -11159,6 +11159,38 @@ dynamic_library_path = 'C:\tools\postgresql;H:\my_project\lib;$libdir'
+
+ debug_io_direct (string)
+
+debug_io_direct configuration parameter
+
+
+
+
+Ask the kernel to minimize caching effects for relation data and WAL
+files using O_DIRECT (most Unix-like systems),
+F_NOCACHE (macOS) or
+FILE_FLAG_NO_BUFFERING (Windows).
+
+
+May be set to an empty string (the default) to disable use of direct
+I/O, or a comma-separated list of operations that should use direct I/O.
+The valid options are data for
+main data files, wal for WAL files, and
+wal_init for WAL files when being initially
+allocated.
+
+
+Some operating systems and file systems do not support direct I/O, so
+non-default settings may be rejected at startup or cause errors.
+
+
+Currently this feature reduces performance, and is intended for
+developer testing only.
+
+
+
+
debug_parallel_query (enum)
@@ -11220,38 +11252,6 @@ dynamic_library_path = 'C:\tools\postgresql;H:\my_project\lib;$libdir'
-
- io_direct (string)
-
-io_direct configuration parameter
-
-
-
-
-Ask the kernel to minimize caching effects for relation data and WAL
-files using O_DIRECT (most Unix-like systems),
-F_NOCACHE (macOS) or
-FILE_FLAG_NO_BUFFERING (Windows).
-
-
-May be set to an empty string (the default) to disable use of direct
-I/O, or a comma-separated list of operations that should use direct I/O.
-The valid options are data for
-main data files, wal for WAL files, and
-wal_init for WAL files when being initially
-allocated.
-
-
-Some operating systems and file systems do not support direct I/O, so
-non-default settings may be rejected at startup or cause errors.
-
-
-Currently this feature reduces performance, and is intended for
-developer testing only.
-
-
-
-
post_auth_delay (integer)
diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c
index 2f42cebaf6..9f9cf7ce66 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -4568,7 +4568,7 @@ struct config_string ConfigureNamesString[] =
},
{
- {"io_direct", PGC_POSTMASTER, DEVELOPER_OPTIONS,
+ {"debug_io_direct", PGC_POSTMASTER, DEVELOPER_OPTIONS,
gettext_noop("Use direct I/O for file access."),
NULL,
GUC_LIST_INPUT | GUC_NOT_IN_SAMPLE
diff --git a/src/test/modules/test_misc/t/004_io_direct.pl b/src/test/modules/test_misc/t/004_io_direct.pl
index b8814bb640..dddcfb1aa9 100644
--- a/src/test/modules/test_misc/t/004_io_direct.pl
Re: Perform streaming logical transactions by background workers and parallel apply
On Fri, Apr 28, 2023 at 6:01 PM Amit Kapila wrote: > > On Fri, Apr 28, 2023 at 11:48 AM Masahiko Sawada > wrote: > > > > On Fri, Apr 28, 2023 at 11:51 AM Amit Kapila > > wrote: > > > > > > On Wed, Apr 26, 2023 at 4:11 PM Zhijie Hou (Fujitsu) > > > wrote: > > > > > > > > On Wednesday, April 26, 2023 5:00 PM Alexander Lakhin > > > > wrote: > > > > > > > > > > IIUC, that assert will fail in case of any error raised between > > > > > ApplyWorkerMain()->logicalrep_worker_attach()->before_shmem_exit() and > > > > > ApplyWorkerMain()->InitializeApplyWorker()->BackgroundWorkerInitializeC > > > > > onnectionByOid()->InitPostgres(). > > > > > > > > Thanks for reporting the issue. > > > > > > > > I think the problem is that it tried to release locks in > > > > logicalrep_worker_onexit() before the initialization of the process is > > > > complete > > > > because this callback function was registered before the init phase. So > > > > I think we > > > > can add a conditional statement before releasing locks. Please find an > > > > attached > > > > patch. > > > > > > > > > > Alexander, does the proposed patch fix the problem you are facing? > > > Sawada-San, and others, do you see any better way to fix it than what > > > has been proposed? > > > > I'm concerned that the idea of relying on IsNormalProcessingMode() > > might not be robust since if we change the meaning of > > IsNormalProcessingMode() some day it would silently break again. So I > > prefer using something like InitializingApplyWorker, > > > > I think if we change the meaning of IsNormalProcessingMode() then it > could also break the other places the similar check is being used. Right, but I think it's unclear the relationship between the processing modes and releasing session locks. If non-normal-processing mode means we're still in the process initialization phase, why we don't skip other cleanup works such as walrcv_disconnect() and FileSetDeleteAll()? > However, I am fine with InitializingApplyWorker as that could be used > at other places as well. I just want to avoid adding another variable > by using IsNormalProcessingMode. I think it's less confusing. > > > or another idea > > would be to do cleanup work (e.g., fileset deletion and lock release) > > in a separate callback that is registered after connecting to the > > database. > > > > Yeah, but not sure if it's worth having multiple callbacks for cleanup work. Fair point. Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com
