Re: Doc: fix a description regarding WAL summarizer on glossary page
On Tue, Jun 11, 2024 at 06:06:11PM +0900, Masahiro Ikeda wrote: > While searching the definition of auxiliary processes, I noticed that the > description of the WAL summarizer is incorrect. Additionally, I think it's > better to add a description for the WAL writer similar to other Auxiliary > processes. What do you think? Good catch. Would you like to attach a patch? -- Michael signature.asc Description: PGP signature
Re: race condition in pg_class
On Mon, Jun 10, 2024 at 07:19:27PM -0700, Noah Misch wrote: > On Fri, Jun 07, 2024 at 09:08:03AM -0400, Robert Haas wrote: >> I think the core code should provide an "Injection Point" wait event >> type and let extensions add specific wait events there, just like you >> did for "Extension". > > Michael, could you accept the core code offering that, or not? If so, I am > content to implement that. If not, for injection point wait events, I have > just one priority. The isolation tester already detects lmgr locks without > the test writer teaching it about each lock individually. I want it to have > that same capability for injection points. Do you think we can find something > everyone can accept, having that property? These wait events show up in tests > only, and I'm happy to make the cosmetics be anything compatible with that > detection ability. Adding a wait event class for injection point is an interesting suggestion that would simplify the detection in the isolation function quite a bit. Are you sure that this is something that would be fit for v17 material? TBH, I am not sure. At the end, the test coverage has the highest priority and the bugs you are addressing are complex enough that isolation tests of this level are a necessity, so I don't object to what inplace050-tests-inj-v2.patch introduces with the naming dependency for the time being on HEAD. I'll just adapt and live with that depending on what I deal with, while trying to improve HEAD later on. I'm still wondering if there is something that could be more elegant than a dedicated class for injection points, but I cannot think about something that would be better for isolation tests on top of my head. If there is something I can think of, I'll just go and implement it :) -- Michael signature.asc Description: PGP signature
Re: Format the code in xact_decode
On Mon, Jun 10, 2024 at 06:03:40PM +0800, cca5507 wrote: > Thank you for reply! No objections here, either. > I have new a patch in commitfest:Format the code in xact_decode > (postgresql.org) Thanks for tracking that. For reference: https://commitfest.postgresql.org/48/5028/ -- Michael signature.asc Description: PGP signature
Re: 001_rep_changes.pl fails due to publisher stuck on shutdown
On Thu, Jun 06, 2024 at 03:19:20PM +0900, Kyotaro Horiguchi wrote: > During server shutdown, the latter half of the last continuation > record may fail to be flushed. This is similar to what is described in > the commit message of commit ff9f111bce. While shutting down, > WalSndLoop() waits for XLogSendLogical() to consume WAL up to > flushPtr, but in this case, the last record cannot complete without > the continuation part starting from flushPtr, which is > missing. However, in such cases, xlogreader.missingContrecPtr is set > to the beginning of the missing part, but something similar to -/* If EndRecPtr is still past our flushPtr, it means we caught up. */ -if (logical_decoding_ctx->reader->EndRecPtr >= flushPtr) +/* + * If EndRecPtr is still past our flushPtr, it means we caught up. When + * the server is shutting down, the latter part of a continuation record + * may be missing. If got_STOPPING is true, assume we are caught up if the + * last record is missing its continuation part at flushPtr. + */ +if (logical_decoding_ctx->reader->EndRecPtr >= flushPtr || +(got_STOPPING && + logical_decoding_ctx->reader->missingContrecPtr == flushPtr)) FWIW, I don't have a better idea than what you are proposing here. We just cannot receive more data past the page boundary in a shutdown sequence in this context, so checking after the missingContrecPtr is a good compromise to ensure that we don't remain stuck when shutting down a logical WAL sender. I'm surprised that we did not hear about that more often on the lists, or perhaps we did but just discarded it? This is going to take some time to check across all the branches down to v12 that this is stable, because this area of the code tends to change slightly every year.. Well, that's my job. > So, I believe the attached small patch fixes the behavior. I haven't > come up with a good test script for this issue. Something like > 026_overwrite_contrecord.pl might work, but this situation seems a bit > more complex than what it handles. Hmm. Indeed you will not be able to reuse the same trick with the end of a segment. Still you should be able to get a rather stable test by using the same tricks as 039_end_of_wal.pl to spawn a record across multiple pages, no? -- Michael signature.asc Description: PGP signature
Re: CheckMyDatabase some error messages in two lines.
On Sun, Jun 09, 2024 at 10:12:53PM -0400, Tom Lane wrote: > No doubt. People have done it both ways in the past, but I think > currently there's a weak consensus in favor of using one line for > such messages even when it runs past 80 columns, mainly because > that makes it easier to grep the source code for a message text. I recall the same consensus here. Greppability matters across the board. > But: I don't see too much value in changing this particular instance, > because the line break is in a place where it would not likely cause > you to miss finding the line. You might grep for the first part of > the string or the second part, but probably not for ", which is not". > If the line break were in the middle of a phrase, there'd be more > argument for collapsing it out. Not sure these ones are worth it, either, so I'd let them be. -- Michael signature.asc Description: PGP signature
Re: Proposal to add page headers to SLRU pages
On Mon, Jun 10, 2024 at 07:19:56AM +, Bertrand Drouvot wrote: > On Tue, Mar 19, 2024 at 06:48:33AM +, Li, Yong wrote: >> Unfortunately, the test requires a setup of two different versions of PG. I >> am not >> aware of an existing test infrastructure which can run automated tests using >> two >> PGs. I did manually verify the output of pg_upgrade. > > I think there is something in t/002_pg_upgrade.pl (see > src/bin/pg_upgrade/TESTING), > that could be used to run automated tests using an old and a current version. Cluster.pm relies on install_path for stuff, where it is possible to create tests with multiple nodes pointing to different installation paths. This allows mixing nodes with different build options, or just different major versions like pg_upgrade's perl tests. -- Michael signature.asc Description: PGP signature
Re: Historic snapshot doesn't track txns committed in BUILDING_SNAPSHOT state
On Sun, Jun 09, 2024 at 11:21:52PM +0800, cca5507 wrote: > Hello hackers, I found that wecurrently don't track txns committed in > BUILDING_SNAPSHOT state because of the code in xact_decode(): > /* >* If the snapshot isn't yet fully built, we cannot decode anything, so >* bail out. >*/ > if (SnapBuildCurrentState(builder) < SNAPBUILD_FULL_SNAPSHOT) > return; > > The output of pub's log: > ERROR: could not map filenumber "base/5/16395" to relation OID > > Is this a bug? Should we also track the txns committed in BUILDING_SNAPSHOT > state? Clearly, this is not an error you should be able to see as a user. So yes, that's something that needs to be fixed. -- Michael signature.asc Description: PGP signature
Re: Test slots invalidations in 035_standby_logical_decoding.pl only if dead rows are removed
On Mon, Jun 10, 2024 at 06:29:17AM +, Bertrand Drouvot wrote: > Thanks for the report! I think it makes sense to add it to the list of known > failures. > > One way to deal with those corner cases could be to make use of injection > points > around places where RUNNING_XACTS is emitted, thoughts? Ah. You mean to force a wait in the code path generating the standby snapshots for the sake of this test? That's interesting, I like it. -- Michael signature.asc Description: PGP signature
Re: Injection points: preloading and runtime arguments
On Sat, Jun 08, 2024 at 04:52:25PM +0500, Andrey M. Borodin wrote:
> Alvaro, here’s the test for multixact CV sleep that I was talking
> about on PGConf.
> It is needed to test [0]. It is based on loaded injection
> points.
> This technique is not committed yet, but the patch looks good.
OK, cool. I'll try to get that into the tree once v18 opens up. I
can see that GetNewMultiXactId() opens a critical section. I am
slightly surprised that you need both the SQL function
injection_points_load() and the macro INJECTION_POINT_LOAD().
Wouldn't one or the other be sufficient?
The test takes 20ms to run here, which is good enough.
+ INJECTION_POINT_LOAD("GetNewMultiXactId-done");
[...]
+ INJECTION_POINT("GetNewMultiXactId-done");
[...]
+ INJECTION_POINT("GetMultiXactIdMembers-CV-sleep");
Be careful about the naming here. All the points use lower case
characters currently.
+# and another multixact have no offest yet, we must wait until this offset
s/offest/offset/.
> When all prerequisites are ready I will post it to corresponding
> thread and create CF item.
OK, let's do that.
--
Michael
signature.asc
Description: PGP signature
Re: PgStat_KindInfo.named_on_disk not required in shared stats
On Fri, Jun 07, 2024 at 08:30:06AM -0700, Andres Freund wrote: > Yes, makes sense. Looks we changed direction during development a bunch of > times...q Thanks for looking, Andres! I guess I'll just apply that once v18 opens up. -- Michael signature.asc Description: PGP signature
Re: Re: Add support to TLS 1.3 cipher suites and curves lists
On Fri, Jun 07, 2024 at 06:02:37PM +0800, Erica Zhang wrote: > I see the https://commitfest.postgresql.org/48/ is still open, could > it be possible to target for PG17? As I know PG17 is going to be > release this year so that we can upgrade our instances to this new > version accodingly. Echoing with Peter, https://commitfest.postgresql.org/48/ is planned to be the first commit fest of the development cycle for Postgres 18. v17 is in feature freeze state and beta, where only bug fixes are accepted, and not new features. -- Michael signature.asc Description: PGP signature
Re: cannot drop intarray extension
On Fri, Jun 07, 2024 at 11:32:14AM +0800, jian he wrote: > in deleteObjectsInList, under certain conditions trying to sort the to > be deleted object list > by just using sort_object_addresses seems to work, > but it looks like a hack. > maybe the proper fix would be in findDependentObjects. @@ -1459,6 +1459,7 @@ RemoveRelations(DropStmt *drop) [...] - performMultipleDeletions(objects, drop->behavior, flags); + if (list_length(drop->objects) > 1) + sortable = false; I have not studied the patch in details, but this looks overcomplicated to me. All the callers of performMultipleDeletions pass down sortable as true, while deleteObjectsInList() uses this argument to avoid the sorting on nested calls. It seems to me that this could be simpler. -- Michael signature.asc Description: PGP signature
PgStat_KindInfo.named_on_disk not required in shared stats
Hi all,
(Relevant folks in CC.)
While hacking on the area of pgstat_*.c, I have noticed the existence
of named_on_disk in PgStat_KindInfo, that is here to track the fact
that replication slots are a particular case in the PgStat_HashKey for
the dshash table of the stats because this kind of stats requires a
mapping between the replication slot name and the hash key.
As far as I can see, this field is not required and is used nowhere,
because the code relies on the existence of the to_serialized_name and
from_serialized_name callbacks to do the mapping.
Wouldn't it make sense to remove it? This field is defined since
5891c7a8ed8f that introduced the shmem stats, and has never been used
since.
This frees an extra bit in PgStat_KindInfo, which is going to help me
a bit with what I'm doing with this area of the code while keeping the
structure size the same.
Thoughts?
--
Michael
From 68c6e8401baea7ba1f0c616bbcd74c19daab770e Mon Sep 17 00:00:00 2001
From: Michael Paquier
Date: Fri, 7 Jun 2024 14:04:06 +0900
Subject: [PATCH] Remove PgStat_KindInfo.named_on_disk
This field is used to track a special case for replication slots that
need a mapping between the dshash key and the slot names, but it is used
nowhere as callbacks take care of sanity checks.
---
src/include/utils/pgstat_internal.h | 8 +---
src/backend/utils/activity/pgstat.c | 1 -
2 files changed, 1 insertion(+), 8 deletions(-)
diff --git a/src/include/utils/pgstat_internal.h b/src/include/utils/pgstat_internal.h
index dbbca31602..f6031995a9 100644
--- a/src/include/utils/pgstat_internal.h
+++ b/src/include/utils/pgstat_internal.h
@@ -193,12 +193,6 @@ typedef struct PgStat_KindInfo
*/
bool accessed_across_databases:1;
- /*
- * For variable-numbered stats: Identified on-disk using a name, rather
- * than PgStat_HashKey. Probably only needed for replication slot stats.
- */
- bool named_on_disk:1;
-
/*
* The size of an entry in the shared stats hash table (pointed to by
* PgStatShared_HashEntry->body).
@@ -239,7 +233,7 @@ typedef struct PgStat_KindInfo
void (*reset_timestamp_cb) (PgStatShared_Common *header, TimestampTz ts);
/*
- * For variable-numbered stats with named_on_disk. Optional.
+ * For variable-numbered stats. Optional.
*/
void (*to_serialized_name) (const PgStat_HashKey *key,
const PgStatShared_Common *header, NameData *name);
diff --git a/src/backend/utils/activity/pgstat.c b/src/backend/utils/activity/pgstat.c
index dcc2ad8d95..44f0d3ede7 100644
--- a/src/backend/utils/activity/pgstat.c
+++ b/src/backend/utils/activity/pgstat.c
@@ -307,7 +307,6 @@ static const PgStat_KindInfo pgstat_kind_infos[PGSTAT_NUM_KINDS] = {
.fixed_amount = false,
.accessed_across_databases = true,
- .named_on_disk = true,
.shared_size = sizeof(PgStatShared_ReplSlot),
.shared_data_off = offsetof(PgStatShared_ReplSlot, stats),
--
2.43.0
signature.asc
Description: PGP signature
Re: Injection points: preloading and runtime arguments
On Thu, Jun 06, 2024 at 03:47:47PM +0500, Andrey M. Borodin wrote: > Is it OK to detach() before wakeup()? Or, perhaps, can a detach() do a > wakeup() automatically? It is OK to do a detach before a wakeup. Noah has been relying on this behavior in an isolation test for a patch he's worked on. See inplace110-successors-v1.patch here: https://www.postgresql.org/message-id/20240512232923.aa.nmi...@google.com That's also something we've discussed for 33181b48fd0e, where Noah wanted to emulate in an automated fashion what one can do with a debugger and one or more breakpoints. Not sure that wakeup() involving a automated detach() is the behavior to hide long-term, actually, as there is also an argument for waking up a point and *not* detach it to force multiple waits. -- Michael signature.asc Description: PGP signature
Re: race condition in pg_class
On Thu, Jun 06, 2024 at 09:48:51AM -0400, Robert Haas wrote: > It's not this patch set's fault, but I'm not very pleased to see that > the injection point wait events have been shoehorned into the > "Extension" category - which they are not - instead of being a new > wait_event_type. That would have avoided the ugly wait-event naming > pattern, inconsistent with everything else, introduced by > inplace050-tests-inj-v1.patch. Not sure to agree with that. The set of core backend APIs supporting injection points have nothing to do with wait events. The library attached to one or more injection points *may* decide to use a wait event like what the wait/wakeup calls in modules/injection_points do, but that's entirely optional. These rely on custom wait events, plugged into the Extension category as the code run is itself in an extension. I am not arguing against the point that it may be interesting to plug in custom wait event categories, but the current design of wait events makes that much harder than what core is currently able to handle, and I am not sure that this brings much at the end as long as the wait event strings can be customized. I've voiced upthread concerns over the naming enforced by the patch and the way it plugs the namings into the isolation functions, by the way. -- Michael signature.asc Description: PGP signature
Re: Fix use of possible uninitialized variable retval (src/pl/plpgsql/src/pl_handler.c)
On Wed, Jun 05, 2024 at 10:27:51PM +0800, Julien Rouhaud wrote: > Note that I removed the -Werror from lapwing a long time ago, so at > least this animal shouldn't lead to hackish fixes for false positive > anymore. That's good to know. Thanks for the update. -- Michael signature.asc Description: PGP signature
Re: ALTER TABLE SET ACCESS METHOD on partitioned tables
On Tue, May 21, 2024 at 08:33:51AM -0500, Justin Pryzby wrote:
> It occurred to me that psql \dP+ should show the AM of partitioned
> tables (and other partitioned rels).
> Arguably, this could've been done when \dP was introduced in v12, but
> at that point would've shown the AM only for partitioned indexes.
> But it makes a lot of sense to do it now that partitioned tables support
> AMs. I suggest to consider this for v17.
Not sure that this is a must-have. It is nice to have, but extra
information is a new feature IMO. Any extra opinions?
I would suggest to attach a patch, that makes review easier. And so
here is one.
--
Michael
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index f67bf0b892..7c9a1f234c 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -4113,7 +4113,7 @@ listPartitionedTables(const char *reltypes, const char *pattern, bool verbose)
PQExpBufferData title;
PGresult *res;
printQueryOpt myopt = pset.popt;
- bool translate_columns[] = {false, false, false, false, false, false, false, false, false};
+ bool translate_columns[] = {false, false, false, false, false, false, false, false, false, false};
const char *tabletitle;
bool mixed_output = false;
@@ -4181,6 +4181,13 @@ listPartitionedTables(const char *reltypes, const char *pattern, bool verbose)
if (verbose)
{
+ /*
+ * Table access methods were introduced in v12, and can be set on
+ * partitioned tables since v17.
+ */
+ appendPQExpBuffer(, ",\n am.amname as \"%s\"",
+ gettext_noop("Access method"));
+
if (showNested)
{
appendPQExpBuffer(,
@@ -4216,6 +4223,9 @@ listPartitionedTables(const char *reltypes, const char *pattern, bool verbose)
if (verbose)
{
+ appendPQExpBufferStr(,
+ "\n LEFT JOIN pg_catalog.pg_am am ON c.relam = am.oid");
+
if (pset.sversion < 12)
{
appendPQExpBufferStr(,
signature.asc
Description: PGP signature
Re: Test to dump and restore objects left behind by regression
On Wed, Jun 05, 2024 at 05:09:58PM +0530, Ashutosh Bapat wrote: > Thanks for the suggestion. I didn't understand the dependency with the > buildfarm module. Will the new module be used in buildfarm separately? I > will work on this soon. Thanks for changing CF entry to WoA. I had some doubts about PGBuild/Modules/TestUpgradeXversion.pm, but after double-checking it loads dynamically AdjustUpgrade from the core tree based on the base path where all the modules are: # load helper module from source tree unshift(@INC, "$srcdir/src/test/perl"); require PostgreSQL::Test::AdjustUpgrade; PostgreSQL::Test::AdjustUpgrade->import; shift(@INC); It would be annoying to tweak the buildfarm code more to have a different behavior depending on the branch of Postgres tested. Anyway, from what I can see, you could create a new module with the dump filtering rules that AdjustUpgrade requires without having to update the buildfarm code. > Changing the physical order of column of a child table based on the > inherited table seems intentional as per MergeAttributes(). That logic > looks sane by itself. In binary mode pg_dump works very hard to retain the > column order by issuing UPDATE commands against catalog tables. I don't > think mimicking that behaviour is the right choice for non-binary dump. I > agree with your conclusion that we fix it in by fixing the diffs. The code > to do that will be part of a separate module. Thanks. -- Michael signature.asc Description: PGP signature
Re: pg_parse_json() should not leak token copies on failure
On Tue, Jun 04, 2024 at 10:10:06AM -0700, Jacob Champion wrote: > On Mon, Jun 3, 2024 at 9:32 PM Kyotaro Horiguchi > wrote: >> I understand that the part enclosed in parentheses refers to the "if >> (ptr) pfree(ptr)" structure. I believe we rely on the behavior of >> free(NULL), which safely does nothing. (I couldn't find the related >> discussion due to a timeout error on the ML search page.) > > For the frontend, right. For the backend, pfree(NULL) causes a crash. > I think [1] is a related discussion on the list, maybe the one you > were looking for? FYI, these choices relate also to 805a397db40b, e890ce7a4feb and 098c703d308f. -- Michael signature.asc Description: PGP signature
Re: Infinite loop in XLogPageRead() on standby
On Tue, Jun 04, 2024 at 04:16:43PM +0200, Alexander Kukushkin wrote: > Now that beta1 was released I hope you are not so busy and hence would like > to follow up on this problem. I am still working on something for the v18 cycle that I'd like to present before the beginning of the next commit fest, so I am a bit busy to get that out first. Fingers crossed to not have open items to look at.. This thread is one of the things I have marked as an item to look at, yes. -- Michael signature.asc Description: PGP signature
Re: Fix use of possible uninitialized variable retval (src/pl/plpgsql/src/pl_handler.c)
On Wed, Jun 05, 2024 at 01:12:41PM +0900, Kyotaro Horiguchi wrote: > At Mon, 27 May 2024 11:31:24 -0300, Ranier Vilela wrote > in >> The function *plpgsql_inline_handler* can use uninitialized >> variable retval, if PG_TRY fails. >> Fix like function*plpgsql_call_handler* wich declare retval as >> volatile and initialize to (Datum 0). You forgot to read elog.h, explaining under which circumstances variables related to TRY/CATCH block should be marked as volatile. There is a big "Note:" paragraph. It is not the first time that this is mentioned on this list: but sending a report without looking at the reason why a change is justified makes everybody waste time. That's not productive. > If PG_TRY fails, retval is not actually accessed, so no real issue > exists. Commit 7292fd8f1c changed plpgsql_call_handler() to the > current form, but as stated in its commit message, it did not fix a > real issue and was solely to silence compiler. This complain was from lapwing, that uses a version of gcc which produces a lot of noise with incorrect issues. It is one of the only 32b buildfarm members, so it still has a lot of value. > I believe we do not need to modify plpgsql_inline_handler() unless > compiler actually issues a false warning for it. If we were to do something, that would be to remove the volatile from plpgsql_call_handler() at the end once we don't have in the buildfarm compilers that complain about it, because there is no reason to use a volatile in this case. :) -- Michael signature.asc Description: PGP signature
Re: In-placre persistance change of a relation
On Tue, Jun 04, 2024 at 03:50:51PM -0500, Nathan Bossart wrote: > Bharath, does the multi-INSERT stuff apply when changing a table to be > LOGGED? If so, I think it would be interesting to compare it with the FPI > approach being discussed here. The answer to this question is yes AFAIK. Look at patch 0002 in the latest series posted here, that touches ATRewriteTable() in tablecmds.c where the rewrite happens should a relation's relpersistence, AM, column or default requires a switch (particularly if more than one property is changed in a single command, grep for AT_REWRITE_*): https://www.postgresql.org/message-id/calj2acuz5+_ynea4zy-xg960_oxefm50mjd71vgscavdkf3...@mail.gmail.com I've just read through the patch set, and they are rather pleasant to the eye. I have comments about them, actually, but that's a topic for the other thread. -- Michael signature.asc Description: PGP signature
Re: Fix an incorrect assertion condition in mdwritev().
On Mon, Jun 03, 2024 at 03:24:07PM -0700, Andres Freund wrote:
> I'm confused - isn't using common/int.h entirely sufficient for that? Nearly
> all architectures have more efficient ways to check for 64bit overflows than
> doing actual 128 bit math.
One simple way to change the assertion would be something like that, I
assume. Andres, does it answer your concerns?
--
Michael
diff --git a/src/backend/storage/smgr/md.c b/src/backend/storage/smgr/md.c
index 6796756358..3849397b25 100644
--- a/src/backend/storage/smgr/md.c
+++ b/src/backend/storage/smgr/md.c
@@ -28,6 +28,7 @@
#include "access/xlogutils.h"
#include "commands/tablespace.h"
#include "common/file_utils.h"
+#include "common/int.h"
#include "miscadmin.h"
#include "pg_trace.h"
#include "pgstat.h"
@@ -929,8 +930,13 @@ mdwritev(SMgrRelation reln, ForkNumber forknum, BlockNumber blocknum,
const void **buffers, BlockNumber nblocks, bool skipFsync)
{
/* This assert is too expensive to have on normally ... */
-#ifdef CHECK_WRITE_VS_EXTEND
- Assert((uint64) blocknum + (uint64) nblocks <= (uint64) mdnblocks(reln, forknum));
+#if defined(USE_ASSERT_CHECKING) && defined(CHECK_WRITE_VS_EXTEND)
+ uint32 tot_blocks;
+
+ if (pg_add_u32_overflow(blocknum, nblocks, _blocks))
+ Assert(false);
+
+ Assert(tot_blocks <= mdnblocks(reln, forknum));
#endif
while (nblocks > 0)
signature.asc
Description: PGP signature
Re: Injection points: preloading and runtime arguments
On Tue, May 21, 2024 at 04:29:54PM +0500, Andrey M. Borodin wrote:
> Currently I'm working on the test using this
> $creator->query_until(qr/start/, q(
> \echo start
> select injection_points_wakeup('');
> select test_create_multixact();
> ));
>
> I'm fine if instead of injection_points_wakeup('') I'll have to use
> select injection_points_preload('point name');.
Based on our discussion of last week, please find attached the
promised patch set to allow your SLRU tests to work. I have reversed
the order of the patches, moving the loading part in 0001 and the
addition of the runtime arguments in 0002 as we have a use-case for
the loading, nothing yet for the runtime arguments.
I have also come back to the naming, feeling that "preload" was
overcomplicated. So I have used the word "load" instead across the
board for 0001.
Note that the SQL function injection_points_load() does now an
initialization of the shmem area when a process plugs into the module
for the first time, fixing the issue you have mentioned with your SLRU
test. Hence, you should be able to do a load(), then a wait in the
critical section as there would be no memory allocation done when the
point runs. Another thing you could do is to define a
INJECTION_POINT_LOAD() in the code path you're stressing outside the
critical section where the point is run. This should save from a call
to the SQL function. This choice is up to the one implementing the
test, both can be useful depending on what one is trying to achieve.
--
Michael
From fe58c08881c7fea3be94e6a166d621e8acc78a92 Mon Sep 17 00:00:00 2001
From: Michael Paquier
Date: Wed, 5 Jun 2024 07:35:29 +0900
Subject: [PATCH v2 1/2] Support loading of injection points
This can be used to load an injection point in the backend-level cache
before running it, to avoid issues if the point cannot be loaded due to
restrictions in the code path where it would be run, like a critical
section.
---
src/include/utils/injection_point.h | 3 +
src/backend/utils/misc/injection_point.c | 116 --
.../expected/injection_points.out | 32 +
.../injection_points--1.0.sql | 10 ++
.../injection_points/injection_points.c | 17 +++
.../injection_points/sql/injection_points.sql | 7 ++
doc/src/sgml/xfunc.sgml | 14 +++
7 files changed, 163 insertions(+), 36 deletions(-)
diff --git a/src/include/utils/injection_point.h b/src/include/utils/injection_point.h
index a61d5d4439..bd3a62425c 100644
--- a/src/include/utils/injection_point.h
+++ b/src/include/utils/injection_point.h
@@ -15,8 +15,10 @@
* Injections points require --enable-injection-points.
*/
#ifdef USE_INJECTION_POINTS
+#define INJECTION_POINT_LOAD(name) InjectionPointLoad(name)
#define INJECTION_POINT(name) InjectionPointRun(name)
#else
+#define INJECTION_POINT_LOAD(name) ((void) name)
#define INJECTION_POINT(name) ((void) name)
#endif
@@ -34,6 +36,7 @@ extern void InjectionPointAttach(const char *name,
const char *function,
const void *private_data,
int private_data_size);
+extern void InjectionPointLoad(const char *name);
extern void InjectionPointRun(const char *name);
extern bool InjectionPointDetach(const char *name);
diff --git a/src/backend/utils/misc/injection_point.c b/src/backend/utils/misc/injection_point.c
index 5c2a0d2297..f02ed6c08b 100644
--- a/src/backend/utils/misc/injection_point.c
+++ b/src/backend/utils/misc/injection_point.c
@@ -129,20 +129,47 @@ injection_point_cache_remove(const char *name)
(void) hash_search(InjectionPointCache, name, HASH_REMOVE, NULL);
}
+/*
+ * injection_point_cache_load
+ *
+ * Load an injection point into the local cache.
+ */
+static void
+injection_point_cache_load(InjectionPointEntry *entry_by_name)
+{
+ char path[MAXPGPATH];
+ void *injection_callback_local;
+
+ snprintf(path, MAXPGPATH, "%s/%s%s", pkglib_path,
+ entry_by_name->library, DLSUFFIX);
+
+ if (!pg_file_exists(path))
+ elog(ERROR, "could not find library \"%s\" for injection point \"%s\"",
+ path, entry_by_name->name);
+
+ injection_callback_local = (void *)
+ load_external_function(path, entry_by_name->function, false, NULL);
+
+ if (injection_callback_local == NULL)
+ elog(ERROR, "could not find function \"%s\" in library \"%s\" for injection point \"%s\"",
+ entry_by_name->function, path, entry_by_name->name);
+
+ /* add it to the local cache when found */
+ injection_point_cache_add(entry_by_name->name, injection_callback_local,
+ entry_by_name->private_data);
+}
+
/*
* injection_point_cache_get
*
* Retrieve an injection point from the local cache, if any.
*/
-static InjectionPointCallback
-injection_point_cache_get(const char *name, const void **private_data)
+static InjectionPointCache
Re: In-placre persistance change of a relation
On Tue, May 28, 2024 at 04:49:45PM -0700, Jelte Fennema-Nio wrote:
> Two notes after looking at this quickly during the advanced patch
> feedback session:
>
> 1. I would maybe split 0003 into two separate patches. One to make SET
> UNLOGGED fast, which seems quite easy to do because no WAL is needed.
> And then a follow up to make SET LOGGED fast, which does all the
> XLOG_FPI stuff.
Yeah, that would make sense. The LOGGED->UNLOGGED part is
straight-forward because we only care about the init fork. The
UNLOGGED->LOGGED case bugs me, though, a lot.
> 2. When wal_level = minitam, still some WAL logging is needed. The
> pages that were changed since the last still need to be made available
> for crash recovery.
More notes from me, as I was part of this session.
+ * : Some access methods don't support in-place persistence
+ * changes. GiST uses page LSNs to figure out whether a block has been
[...]
+if (r->rd_rel->relkind == RELKIND_INDEX &&
+/* GiST is excluded */
+r->rd_rel->relam != BTREE_AM_OID &&
+r->rd_rel->relam != HASH_AM_OID &&
+r->rd_rel->relam != GIN_AM_OID &&
+r->rd_rel->relam != SPGIST_AM_OID &&
+r->rd_rel->relam != BRIN_AM_OID)
This knowledge should not be encapsulated in the backend code. The
index AMs should be able to tell, instead, if they are able to support
this code path so as any out-of-core index AM can decide things on its
own. This ought to be split in its own patch, simple enough as of a
boolean or a routine telling how this backend path should behave.
+for (fork = 0; fork < INIT_FORKNUM; fork++)
+{
+if (smgrexists(RelationGetSmgr(r), fork))
+log_newpage_range(r, fork, 0,
+ smgrnblocks(RelationGetSmgr(r), fork),
+ false);
+}
A simple copy of the blocks means that we keep anything bloated in
them, while a rewrite in ALTER TABLE means that we would start afresh
by deforming the tuples from the origin before giving them to the
target, without any bloat. The compression of the FPWs and the
removal of the holes in the pages would surely limit the impact, but
this has not been discussed on this thread, and this is a nice
property of the existing implementation that would get silently
removed by this patch set.
Another point that Nathan has made is that it may be more appealling
to study how this is better than an integration with the multi-INSERT
APIs into AMs, so as it is possible to group the inserts in batches
rather than process them one-at-a-time, see [1]. I am ready to accept
that what this patch does is more efficient as long as everything is
block-based in some cases. Still there is a risk-vs-gain argument
here, and I am not sure whether what we have here is a good tradeoff
compared to the potential risk of breaking things. The amount of new
infrastructure is large for this code path. Grouping the inserts in
large batches may finish by being more efficient than a WAL stream
full of FPWs, as well, even if toast values are deformed? So perhaps
there is an argument for making that optional at query level, instead.
As a hole, I can say that grouping the INSERTs will be always more
efficient, while what we have here can be less efficient in some
cases. I'm OK to be outvoted, but the level of complications created
by this block-based copy and WAL-logging concerns me when it comes to
tweaking the relpersistence like that.
[1]: https://commitfest.postgresql.org/48/4777/
--
Michael
signature.asc
Description: PGP signature
Re: pg_ctl start may return 0 even if the postmaster has been already started on Windows
On Mon, Jan 15, 2024 at 01:34:46PM -0500, Robert Haas wrote: > This kind of change looks massively helpful to me. I don't know if it > is exactly right or not, but it would have been a big help to me when > writing my previous review, so +1 for some change of this general > type. During a live review of this patch last week, as part of the Advanced Patch Workshop of pgconf.dev, it has been mentioned by Tom that we may be able to simplify the check on pmstart if the detection of the postmaster PID started by pg_ctl is more stable using the WIN32 internals that this patch relies on. I am not sure that this suggestion is right, though, because we should still care about the clock skew case as written in the surrounding comments? Even if that's OK, I would assume that this should be an independent patch, written on top of the proposed v6-0001. Tom, could you comment about that? Perhaps my notes did not catch what you meant. -- Michael signature.asc Description: PGP signature
Re: Test to dump and restore objects left behind by regression
On Fri, Apr 26, 2024 at 06:38:22PM +0530, Ashutosh Bapat wrote: > Some points for discussion: > 1. The test still hardcodes the diffs between two dumps. Haven't found a > better way to do it. I did consider removing the problematic objects from > the regression database but thought against it since we would lose some > coverage. > > 2. The new code tests dump and restore of just the regression database and > does not use pg_dumpall like pg_upgrade. Should it instead perform > pg_dumpall? I decided against it since a. we are interested in dumping and > restoring objects left behind by regression, b. I didn't find a way to > provide the format option to pg_dumpall. The test could be enhanced to use > different dump formats. > > I have added it to the next commitfest. > https://commitfest.postgresql.org/48/4956/ Ashutosh and I have discussed this patch a bit last week. Here is a short summary of my input, after I understood what is going on. + # We could avoid this by dumping the database loaded from original dump. + # But that would change the state of the objects as left behind by the + # regression. + my $expected_diff = " -- + CREATE TABLE public.gtestxx_4 ( +-b integer, +-a integer NOT NULL ++a integer NOT NULL, ++b integer + ) [...] + my ($stdout, $stderr) = + run_command([ 'diff', '-u', $dump4_file, $dump5_file]); + # Clear file names, line numbers from the diffs; those are not going to + # remain the same always. Also clear empty lines and normalize new line + # characters across platforms. + $stdout =~ s/^\@\@.*$//mg; + $stdout =~ s/^.*$dump4_file.*$//mg; + $stdout =~ s/^.*$dump5_file.*$//mg; + $stdout =~ s/^\s*\n//mg; + $stdout =~ s/\r\n/\n/g; + $expected_diff =~ s/\r\n/\n/g; + is($stdout, $expected_diff, 'old and new dumps match after dump and restore'); +} I am not a fan of what this patch does, adding the knowledge related to the dump filtering within 002_pg_upgrade.pl. Please do not take me wrong, I am not against the idea of adding that within this pg_upgrade test to save from one full cycle of `make check` to check the consistency of the dump. My issue is that this logic should be externalized, and it should be in fewer lines of code. For the externalization part, Ashutosh and I considered a few ideas, but one that we found tempting is to create a small .pm, say named AdjustDump.pm. This would share some rules with the existing AdjustUpgrade.pm, which would be fine IMO even if there is a small overlap, documenting the dependency between each module. That makes the integration with the buildfarm much simpler by not creating more dependencies with the modules shared between core and the buildfarm code. For the "shorter" part, one idea that I had is to apply to the dump a regexp that wipes out the column definitions within the parenthesis, keeping around the CREATE TABLE and any other attributes not impacted by the reordering. All that should be documented in the module, of course. Another thing would be to improve the backend so as we are able to a better support for physical column ordering, which would, I assume (and correct me if I'm wrong!), prevent the reordering of the attributes like in this inheritance case. But that would not address the case of dumps taken from older versions with a new version of pg_dump, which is something that may be interesting to have more tests for in the long-term. Overall a module sounds like a better solution. -- Michael signature.asc Description: PGP signature
Re: Fix an incorrect assertion condition in mdwritev().
On Mon, Jun 03, 2024 at 03:24:07PM -0700, Andres Freund wrote: > I'm confused - isn't using common/int.h entirely sufficient for that? Nearly > all architectures have more efficient ways to check for 64bit overflows than > doing actual 128 bit math. Oh, right. We could just plug in pg_add_u32_overflow here. Funny thing is that I'm the one who committed these toys with __builtin_add_overflow(), still nobody has found a case where this one would be useful. At least until now. -- Michael signature.asc Description: PGP signature
Re: Fix an incorrect assertion condition in mdwritev().
On Sun, May 26, 2024 at 12:08:46AM -0400, Tom Lane wrote: > After a few minutes' thought, how about: > > Assert((uint64) blocknum + (uint64) nblocks <= (uint64) mdnblocks(reln, > forknum)); > > This'd stop being helpful if we ever widen BlockNumber to 64 bits, > but I think that's unlikely. (Partitioning seems like a better answer > for giant tables.) No idea if this will happen or not, but that's not the only area where we are going to need a native uint128 implementation to control the overflows with uint64. What you are suggesting is good enough for me, so I've applied on HEAD a version using that. -- Michael signature.asc Description: PGP signature
Re: Add memory context type to pg_backend_memory_contexts view
On Fri, May 31, 2024 at 12:35:58PM +1200, David Rowley wrote:
> This follows what we do in other places. If you look at explain.c,
> you'll see lots of "???"s.
>
> I think if you're worried about corrupted memory, then garbled output
> in pg_get_backend_memory_contexts wouldn't be that high on the list of
> concerns.
+ const char *type;
[...]
+ switch (context->type)
+ {
+ case T_AllocSetContext:
+ type = "AllocSet";
+ break;
+ case T_GenerationContext:
+ type = "Generation";
+ break;
+ case T_SlabContext:
+ type = "Slab";
+ break;
+ case T_BumpContext:
+ type = "Bump";
+ break;
+ default:
+ type = "???";
+ break;
+ }
Yeah, it's a common practice to use that as fallback. What you are
doing is OK, and it is not possible to remove the default case as
these are nodetags to generate warnings if a new value needs to be
added.
This patch looks like a good idea, so +1 from here. (PS: catversion
bump).
--
Michael
signature.asc
Description: PGP signature
Re: Comments on Custom RMGRs
On Fri, May 17, 2024 at 04:25:15PM -0400, Robert Haas wrote: > On Fri, May 17, 2024 at 4:20 PM Jeff Davis wrote: >> Regarding this particular change: the checkpointing hook seems more >> like a table AM feature, so I agree with you that we should have a good >> idea how a real table AM might use this, rather than only >> pg_stat_statements. > > I would even be OK with a pg_stat_statements example that is fully > working and fully explained. I just don't want to have no example at > all. The original proposal has been changed twice because of > complaints that the hook wasn't quite useful enough, but I think that > only proves that v3 is closer to being useful than v1. If v1 is 40% of > the way to useful and v3 is 120% of the way to useful, wonderful! But > if v1 is 20% of the way to being useful and v3 is 60% of the way to > being useful, it's not time to commit anything yet. I don't know which > is the case, and I think if someone wants this to be committed, they > need to explain clearly why it's the first and not the second. Please note that I've been studying ways to have pg_stat_statements being plugged in directly with the shared pgstat APIs to get it backed by a dshash to give more flexibility and scaling, giving a way for extensions to register their own stats kind. In this case, the flush of the stats would be controlled with a callback in the stats registered by the extensions, conflicting with what's proposed here. pg_stat_statements is all about stats, at the end. I don't want this argument to act as a barrier if a checkpoint hook is an accepted consensus here, but a checkpoint hook used for this code path is not the most intuitive solution I can think of in the long-term. -- Michael signature.asc Description: PGP signature
Re: Avoid possible dereference null pointer (src/backend/catalog/pg_depend.c)
On Fri, May 24, 2024 at 09:05:35AM -0300, Ranier Vilela wrote: > The function *get_attname* palloc the result name (pstrdup). > Isn't it necessary to free the memory here (pfree)? This is going to be freed with the current memory context, and all the callers of getIdentitySequence() are in query execution paths, so I don't see much the point. A second thing was a missing check on the attnum returned by get_attnum() with InvalidAttrNumber. I'd be tempted to introduce a missing_ok to this routine after looking at the callers in all the tree, as some of them want to fail still would not expect it, so that would reduce a bit the elog churn. That's a story for a different day, though. -- Michael signature.asc Description: PGP signature
Re: Fix an incorrect assertion condition in mdwritev().
On Sun, May 26, 2024 at 12:08:46AM -0400, Tom Lane wrote: > After a few minutes' thought, how about: > > Assert((uint64) blocknum + (uint64) nblocks <= (uint64) mdnblocks(reln, > forknum)); LGTM. Yeah that should be OK this way. -- Michael signature.asc Description: PGP signature
Re: Fix an incorrect assertion condition in mdwritev().
On Sat, May 25, 2024 at 11:52:22PM +0800, Xing Guo wrote: > In commit 4908c58[^1], a vectored variant of smgrwrite() is added and > the assertion condition in mdwritev() no longer matches the comment. > This patch helps fix it. > > /* This assert is too expensive to have on normally ... */ > #ifdef CHECK_WRITE_VS_EXTEND > - Assert(blocknum < mdnblocks(reln, forknum)); > + Assert(blocknum + nblocks <= mdnblocks(reln, forknum)); > #endif Yes, it looks like you're right that this can be made stricter, computing the number of blocks we're adding in the number calculated (aka adding one block to this number fails immediately at initdb). -- Michael signature.asc Description: PGP signature
Re: Avoid possible dereference null pointer (src/backend/catalog/pg_depend.c)
On Fri, May 24, 2024 at 11:58:51AM +0530, Ashutosh Bapat wrote: > If we are looking for avoiding a segfault and get a message which helps > debugging, using get_attname and get_attnum might be better options. > get_attname throws an error. get_attnum doesn't throw an error and returns > InvalidAttnum which won't return any valid identity sequence, and thus > return a NIL sequence list which is handled in that function already. Using > these two functions will avoid the clutter as well as segfault. If that's > acceptable, I will provide a patch. Yeah, you could do that with these two routines as well. The result would be the same in terms of runtime validity checks. -- Michael signature.asc Description: PGP signature
Re: Avoid possible dereference null pointer (src/backend/catalog/pg_depend.c)
On Thu, May 23, 2024 at 08:54:12AM -0300, Ranier Vilela wrote: > All calls to functions like SearchSysCacheAttName, in the whole codebase, > checks if returns are valid. > It must be for a very strong reason, such a style. Usually good practice, as I've outlined once upthread, because we do expect the attributes to exist in this case. Or if you want, an error is better than a crash if a concurrent path causes this area to lead to inconsistent lookups, which is something I've seen in the past while hacking on my own stuff, or just fix other things causing syscache lookup inconsistencies. You'd be surprised to hear that dropped attributes being mishandled is not that uncommon, especially in out-of-core code, as one example. FWIW, I don't see much a point in using ereport(), the two checks ought to be elog()s pointing to an internal error as these two errors should never happen. Still, it is a good idea to check that they never happen: aka an internal error state is better than a crash if a problem arises. > So, v3, implements it this way. I don't understand the point behind the open/close of attrelation, TBH. That's not needed. Except fot these two points, this is just moving the calls to make sure that we have valid tuples from the syscache, which is a better practice. 509199587df7 is recent enough that this should be fixed now rather than later. -- Michael signature.asc Description: PGP signature
Re: Cleaning up perl code
On Tue, May 21, 2024 at 02:33:16PM +0900, Michael Paquier wrote:
> Nice catches from both of you. The two ones in
> generate-wait_event_types.pl are caused by me, actually.
>
> Not sure about the changes in the errcodes scripts, though. The
> current state of thing can be also useful when it comes to debugging
> the parsing, and it does not hurt to keep the parsing rules the same
> across the board.
For now, I have staged for commit the attached, that handles most of
the changes from Alexander (msvc could go for more cleanup?). I'll
look at the changes from Dagfinn after that, including if perlcritic
could be changed. I'll handle the first part when v18 opens up, as
that's cosmetic.
The incorrect comment in 024_add_drop_pub.pl has been fixed as of
53785d2a2aaa, as that was a separate issue.
--
Michael
From 270e212e9f7524b96383387d62765185034fd59f Mon Sep 17 00:00:00 2001
From: Michael Paquier
Date: Fri, 24 May 2024 13:59:00 +0900
Subject: [PATCH] Cleanup perl code from unused variables and routines
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
This commit removes unused variables and routines from some perl code
that have accumulated across the years. This concerns various areas:
the script for wait events, AdjustUpgrade.pm and TAP code.
Author: Alexander Lakhin
Reviewed-by: Dagfinn Ilmari Mannsåker
Discussion: https://postgr.es/m/70b340bc-244a-589d-ef8b-d8aebb707...@gmail.com
---
.../utils/activity/generate-wait_event_types.pl | 2 --
src/bin/pg_dump/t/003_pg_dump_with_server.pl | 1 -
src/bin/pg_dump/t/005_pg_dump_filterfile.pl | 1 -
.../t/001_mutated_bindpasswd.pl | 4
.../ssl_passphrase_callback/t/001_testfunc.pl | 1 -
src/test/perl/PostgreSQL/Test/AdjustUpgrade.pm| 1 -
src/test/recovery/t/021_row_visibility.pl | 1 -
src/test/recovery/t/032_relfilenode_reuse.pl | 1 -
.../recovery/t/035_standby_logical_decoding.pl| 5 +
contrib/amcheck/t/001_verify_heapam.pl| 15 +--
contrib/amcheck/t/002_cic.pl | 2 +-
11 files changed, 3 insertions(+), 31 deletions(-)
diff --git a/src/backend/utils/activity/generate-wait_event_types.pl b/src/backend/utils/activity/generate-wait_event_types.pl
index 42f36f405b..b2d61bd8ba 100644
--- a/src/backend/utils/activity/generate-wait_event_types.pl
+++ b/src/backend/utils/activity/generate-wait_event_types.pl
@@ -42,8 +42,6 @@ my @abi_compatibility_lines;
my @lines;
my $abi_compatibility = 0;
my $section_name;
-my $note;
-my $note_name;
# Remove comments and empty lines and add waitclassname based on the section
while (<$wait_event_names>)
diff --git a/src/bin/pg_dump/t/003_pg_dump_with_server.pl b/src/bin/pg_dump/t/003_pg_dump_with_server.pl
index b5a1445550..3f549f44e7 100644
--- a/src/bin/pg_dump/t/003_pg_dump_with_server.pl
+++ b/src/bin/pg_dump/t/003_pg_dump_with_server.pl
@@ -26,7 +26,6 @@ $node->safe_psql('postgres', "CREATE SERVER s1 FOREIGN DATA WRAPPER dummy");
$node->safe_psql('postgres', "CREATE SERVER s2 FOREIGN DATA WRAPPER dummy");
$node->safe_psql('postgres', "CREATE FOREIGN TABLE t0 (a int) SERVER s0");
$node->safe_psql('postgres', "CREATE FOREIGN TABLE t1 (a int) SERVER s1");
-my ($cmd, $stdout, $stderr, $result);
command_fails_like(
[ "pg_dump", '-p', $port, '--include-foreign-data=s0', 'postgres' ],
diff --git a/src/bin/pg_dump/t/005_pg_dump_filterfile.pl b/src/bin/pg_dump/t/005_pg_dump_filterfile.pl
index a80e13a0d3..6025bb296c 100644
--- a/src/bin/pg_dump/t/005_pg_dump_filterfile.pl
+++ b/src/bin/pg_dump/t/005_pg_dump_filterfile.pl
@@ -81,7 +81,6 @@ $node->safe_psql('sourcedb',
#
# Test interaction of correctly specified filter file
#
-my ($cmd, $stdout, $stderr, $result);
# Empty filterfile
open $inputfile, '>', "$tempdir/inputfile.txt"
diff --git a/src/test/modules/ldap_password_func/t/001_mutated_bindpasswd.pl b/src/test/modules/ldap_password_func/t/001_mutated_bindpasswd.pl
index b990e7d101..82b1cb88e9 100644
--- a/src/test/modules/ldap_password_func/t/001_mutated_bindpasswd.pl
+++ b/src/test/modules/ldap_password_func/t/001_mutated_bindpasswd.pl
@@ -12,10 +12,6 @@ use Test::More;
use lib "$FindBin::RealBin/../../../ldap";
use LdapServer;
-my ($slapd, $ldap_bin_dir, $ldap_schema_dir);
-
-$ldap_bin_dir = undef;# usually in PATH
-
if ($ENV{with_ldap} ne 'yes')
{
plan skip_all => 'LDAP not supported by this build';
diff --git a/src/test/modules/ssl_passphrase_callback/t/001_testfunc.pl b/src/test/modules/ssl_passphrase_callback/t/001_testfunc.pl
index 7a63539f39..f71d0ff3e0 100644
--- a/src/test/modules/ssl_passphrase_callback/t/001_testfunc.pl
+++ b/src/test/modules/ssl_passphrase_callback/t/001_testfunc.pl
@@ -15,7 +15,6 @@ unless (($ENV{with_ssl} || "") eq 'openssl')
plan skip_all => 'OpenSSL not supported
Re: Fix src/test/subscription/t/029_on_error.pl test when wal_debug is enabled
On Thu, May 23, 2024 at 08:12:07AM +, Ilyasov Ian wrote: > > It seems to me that we should keep the 'for replication target relation > "public.tbl" in transaction \d+,', before the "finished at" so as it > is possible to make a difference with the context that has a column > name and the context where there is no target relation. > > I agree. Attached the updated patch. One issue that you have here is that the regexp detection would still fail when setting log_error_verbosity = verbose because of the extra error code added between the ERROR and the string. This caused the LSN to not be fetched properly. At the end, I've come up with a regexp that checks for a match of the error string after the ERROR to not confuse the last part getting the xdigits, and applied that down to 15. Perhaps I would have added the first "ERROR" part in the check, but could not come down to it for the readability of the thing. -- Michael signature.asc Description: PGP signature
Re: Fix src/test/subscription/t/029_on_error.pl test when wal_debug is enabled
On Wed, May 22, 2024 at 02:24:37PM +, Ilyasov Ian wrote: > I corrected my patch according to what I think > Michael wanted. I attached the new patch to the letter. Thanks for compiling this patch. Yes, that's the idea. - qr/processing remote data for replication origin \"pg_\d+\" during message type "INSERT" for replication target relation "public.tbl" in transaction \d+, finished at ([[:xdigit:]]+\/[[:xdigit:]]+)/ + qr/ERROR: duplicate key.*\n.*DETAIL:.*\n.*CONTEXT:.* finished at ([[:xdigit:]]+\/[[:xdigit:]]+)/m There are three CONTEXT strings that could map to this context. It seems to me that we should keep the 'for replication target relation "public.tbl" in transaction \d+,', before the "finished at" so as it is possible to make a difference with the context that has a column name and the context where there is no target relation. That makes the regexp longer, but it is not that bad. -- Michael signature.asc Description: PGP signature
Re: [PATCH] LockAcquireExtended improvement
On Sat, May 18, 2024 at 05:10:38PM +0800, Jingxian Li wrote: > Nice catch! The patch looks good to me. And fixed that as well. -- Michael signature.asc Description: PGP signature
Re: Avoid possible dereference null pointer (src/backend/catalog/pg_depend.c)
On Wed, May 22, 2024 at 03:28:48PM -0300, Ranier Vilela wrote: > 1. Another concern is the function *get_partition_ancestors*, > which may return NIL, which may affect *llast_oid*, which does not handle > NIL entries. Hm? We already know in the code path that the relation we are dealing with when calling get_partition_ancestors() *is* a partition thanks to the check on relispartition, no? In this case, calling get_partition_ancestors() is valid and there should be a top-most parent in any case all the time. So I don't get the point of checking get_partition_ancestors() for NIL-ness just for the sake of assuming that it would be possible. > 2. Is checking *relispartition* enough? > There a function *check_rel_can_be_partition* > (src/backend/utils/adt/partitionfuncs.c), > which performs a much more robust check, would it be worth using it? > > With the v2 attached, 1 is handled, but, in this case, > will it be the most correct? Saying that, your point about the result of SearchSysCacheAttName not checked if it is a valid tuple is right. We paint errors in these cases even if they should not happen as that's useful when it comes to debugging, at least. -- Michael signature.asc Description: PGP signature
Re: Cleaning up perl code
On Tue, May 21, 2024 at 06:00:00AM +0300, Alexander Lakhin wrote: > I reviewed my collection of unica I gathered for several months, but had > found some of them too minor/requiring more analysis. > Then I added more with perlcritic's policy UnusedVariables, and also > checked for unused subs with a script from blogs.perl.org (and it confirmed > my only previous find of that kind). Nice catches from both of you. The two ones in generate-wait_event_types.pl are caused by me, actually. Not sure about the changes in the errcodes scripts, though. The current state of thing can be also useful when it comes to debugging the parsing, and it does not hurt to keep the parsing rules the same across the board. >> The scripts parsing errcodes.txt really should be refactored into using >> a common module, but that's a patch for another day. > > Agree, and I would leave 005_negotiate_encryption.pl (with $node_conf, > $server_config unused since d39a49c1e) aside for another day too. I'm not sure about these ones as each one of these scripts have their own local tweaks. Now, if there is a cleaner picture with a .pm module I don't see while reading the whole, why not as long as it improves the code. -- Michael signature.asc Description: PGP signature
Re: Postgres and --config-file option
On Mon, May 20, 2024 at 12:20:02PM +0300, Aleksander Alekseev wrote: > Robert Haas writes: >> I agree that it's not necessary or particularly useful for this hint >> to be exhaustive. I could get behind your suggestion of >> s/You must specify/Specify/, but I also think it's fine to do nothing. > > Fair enough, let's leave the help message as is then. I closed the > corresponding CF entry. I'm OK to leave this be, as well. -- Michael signature.asc Description: PGP signature
Re: Injection points: preloading and runtime arguments
On Mon, May 20, 2024 at 05:01:15PM +0500, Andrey M. Borodin wrote:
> Both features look useful to me.
> I've tried to rebase my test of CV sleep during multixact generation[0]. I
> used it like this:
>
> INJECTION_POINT_PRELOAD("GetNewMultiXactId-done");
> multi = GetNewMultiXactId(nmembers, ); // starts critsection
> INJECTION_POINT("GetNewMultiXactId-done");
Thanks for the feedback.
> And it fails like this:
>
> 2024-05-20 16:50:40.430 +05 [21830] 001_multixact.pl LOG: statement: select
> test_create_multixact();
> TRAP: failed Assert("CritSectionCount == 0 ||
> (context)->allowInCritSection"), File: "mcxt.c", Line: 1185, PID: 21830
> 0 postgres0x000101452ed0
> ExceptionalCondition + 220
> 1 postgres0x0001014a6050 MemoryContextAlloc
> + 208
> 2 postgres0x0001011c3bf0
> dsm_create_descriptor + 72
> 3 postgres0x0001011c3ef4 dsm_attach + 400
> 4 postgres0x0001014990d8 dsa_attach + 24
> 5 postgres0x0001011c716c init_dsm_registry
> + 240
> 6 postgres0x0001011c6e60 GetNamedDSMSegment
> + 456
> 7 injection_points.dylib 0x000101c871f8
> injection_init_shmem + 60
> 8 injection_points.dylib 0x000101c86f1c injection_wait + 64
> 9 postgres0x00010148e228
> InjectionPointRunInternal + 376
> 10 postgres0x00010148e0a4 InjectionPointRun
> + 32
> 11 postgres0x000100cab798
> MultiXactIdCreateFromMembers + 344
> 12 postgres0x000100cab604 MultiXactIdCreate
> + 312
>
> Am I doing something wrong? Seems like extension have to know too that it is
> preloaded.
Your stack is pointing at the shared memory section initialized in the
module injection_points, which is a bit of a chicken-and-egg problem
because you'd want an extra preload to happen before even that, like a
pre-preload. From what I can see, you have a good point about the
shmem initialized in the module: injection_points_preload() should
call injection_init_shmem() so as this area would not trigger the
assertion.
However, there is a second thing here inherent to your test: shouldn't
the script call injection_points_preload() to make sure that the local
cache behind GetNewMultiXactId-done is fully allocated and prepared
for the moment where injection point will be run?
So I agree that 0002 ought to call injection_init_shmem() when calling
injection_points_preload(), but it also seems to me that the test is
missing the fact that it should heat the backend cache to avoid the
allocations in the critical sections.
Note that I disagree with taking a shortcut in the backend-side
injection point code where we would bypass CritSectionCount or
allowInCritSection. These states should stay consistent for the sake
of the callbacks registered so as these can rely on the same stack and
conditions as the code where they are called.
--
Michael
signature.asc
Description: PGP signature
Re: State of pg_createsubscriber
On Sun, May 19, 2024 at 02:49:22PM -0300, Euler Taveira wrote: > My bad. :( I'll post patches soon to address all of the points. Please note that I have added an open item pointing at this thread. -- Michael signature.asc Description: PGP signature
Injection points: preloading and runtime arguments
Hi all,
I have a couple of extra toys for injection points in my bucket that
I'd like to propose for integration in v18, based on some feedback I
have received:
1) Preload an injection point into the backend-level cache without
running it. This has come up because an injection point run for the
first time needs to be loaded with load_external_function that
internally does some allocations, and this would not work if the
injection point is in a critical section. Being able to preload an
injection point requires an extra macro, called
INJECTION_POINT_PRELOAD. Perhaps "load" makes more sense as keyword,
here.
2) Grab values at runtime from the code path where an injection point
is run and give them to the callback. The case here is to be able to
do some dynamic manipulation or a stack, reads of some runtime data or
even decide of a different set of actions in a callback based on what
the input has provided. One case that I've been playing with here is
the dynamic manipulation of pages in specific code paths to stress
internal checks, as one example. This introduces a 1-argument
version, as multiple args could always be passed down to the callback
within a structure.
The in-core module injection_points is extended to provide a SQL
interface to be able to do the preloading or define a callback with
arguments. The two changes are split into patches of their own.
These new facilities could be backpatched if there is a need for them
in the future in stable branches, as these are aimed for tests and the
changes do not introduce any ABI breakages with the existing APIs or
the in-core module.
Thoughts and comments are welcome.
--
Michael
From ed617725d1e6a156363384ed5fcbf4e79b5f8ab4 Mon Sep 17 00:00:00 2001
From: Michael Paquier
Date: Mon, 20 May 2024 11:50:42 +0900
Subject: [PATCH 1/2] Extend injection points with optional runtime arguments
This extends injections points with a 1-argument flavor, so as it
becomes possible for callbacks to pass down values coming from the code
paths where an injection point is defined. The primary use case that
can be covered in this function is runtime manipulation of a given
stack, where it would be possible to corrupt a running state, based on a
runtime set of conditions.
For example, imagine a class of failures in the solar flare category
causing bits to flip on a page.
---
src/include/utils/injection_point.h | 9 +-
src/backend/utils/misc/injection_point.c | 92 +--
.../expected/injection_points.out | 31 +++
.../injection_points--1.0.sql | 10 ++
.../injection_points/injection_points.c | 39 +++-
.../injection_points/sql/injection_points.sql | 9 ++
doc/src/sgml/xfunc.sgml | 9 +-
7 files changed, 169 insertions(+), 30 deletions(-)
diff --git a/src/include/utils/injection_point.h b/src/include/utils/injection_point.h
index a61d5d4439..c2c0840706 100644
--- a/src/include/utils/injection_point.h
+++ b/src/include/utils/injection_point.h
@@ -16,8 +16,10 @@
*/
#ifdef USE_INJECTION_POINTS
#define INJECTION_POINT(name) InjectionPointRun(name)
+#define INJECTION_POINT_1ARG(name, arg1) InjectionPointRun1Arg(name, arg1)
#else
#define INJECTION_POINT(name) ((void) name)
+#define INJECTION_POINT_1ARG(name) ((void) name, (void) arg1)
#endif
/*
@@ -25,6 +27,9 @@
*/
typedef void (*InjectionPointCallback) (const char *name,
const void *private_data);
+typedef void (*InjectionPointCallback1Arg) (const char *name,
+ const void *private_data,
+ const void *arg1);
extern Size InjectionPointShmemSize(void);
extern void InjectionPointShmemInit(void);
@@ -33,8 +38,10 @@ extern void InjectionPointAttach(const char *name,
const char *library,
const char *function,
const void *private_data,
- int private_data_size);
+ int private_data_size,
+ int num_args);
extern void InjectionPointRun(const char *name);
+extern void InjectionPointRun1Arg(const char *name, void *arg1);
extern bool InjectionPointDetach(const char *name);
#endif /* INJECTION_POINT_H */
diff --git a/src/backend/utils/misc/injection_point.c b/src/backend/utils/misc/injection_point.c
index 5c2a0d2297..2bcdb2708c 100644
--- a/src/backend/utils/misc/injection_point.c
+++ b/src/backend/utils/misc/injection_point.c
@@ -56,6 +56,9 @@ typedef struct InjectionPointEntry
* callbacks, registered when attached.
*/
char private_data[INJ_PRIVATE_MAXLEN];
+
+ /* Number of arguments used by the callback */
+ int num_args;
} InjectionPointEntry;
#define INJECTION_POINT_HASH_INIT_SIZE 16
@@ -69,7 +72,12 @@ typedef struct InjectionPointCacheEntry
{
char name[INJ_NAME_MAXLEN];
char private_data[INJ_PRIVATE_MAXLEN];
- InjectionPointCallback callback;
+ int num_args;
+ union
+ {
+ InjectionPointCallback callback;
+ InjectionPointCallback1Arg callback_1arg;
+ } data;
} InjectionPointCacheEntry;
Re: [PATCH] LockAcquireExtended improvement
On Fri, May 17, 2024 at 11:38:35PM -0700, Will Mortensen wrote: > On Tue, Mar 26, 2024 at 7:14 PM Will Mortensen wrote: >> This comment on ProcSleep() seems to have the values of dontWait >> backward (double negatives are tricky): >> >> * Result: PROC_WAIT_STATUS_OK if we acquired the lock, >> PROC_WAIT_STATUS_ERROR >> * if not (if dontWait = true, this is a deadlock; if dontWait = false, we >> * would have had to wait). >> >> Also there's a minor typo in a comment in LockAcquireExtended(): >> >> * Check the proclock entry status. If dontWait = true, this is an >> * expected case; otherwise, it will open happen if something in the >> * ipc communication doesn't work correctly. >> >> "open" should be "only". > > Here's a patch fixing those typos. Perhaps, this, err.. Should not have been named "dontWait" but "doWait" ;) Anyway, this goes way back in time and it is deep in the stack (LockAcquireExtended, etc.) so it is too late to change: the patch should be OK as it is. -- Michael signature.asc Description: PGP signature
Re: Propagate sanity checks of ProcessUtility() to standard_ProcessUtility()?
On Fri, May 17, 2024 at 03:53:58PM -0400, Robert Haas wrote: > The usual pattern for using hooks like this is to call the next > implementation, or the standard implementation, and pass down the > arguments that you got. If you try to do that and mess it up, you're > going to get a crash really, really quickly and it shouldn't be very > difficult at all to figure out what you did wrong. Honestly, that > doesn't seem like it would be hard even without the assertions: for > the most part, a quick glance at the stack backtrace ought to be > enough. If you're trying to do something more sophisticated, like > mutating the node tree before passing it down, the chances of your > mistakes getting caught by these assertions are pretty darn low, since > they're very superficial checks. Perhaps, still that would be something. Hmm. We've had in the past bugs where DDL paths were playing with the manipulation of Querys in core, corrupting their contents. It seems like what you would mean is to have a check at the *end* of ProcessUtility() that does an equalFuncs() on the Query, comparing it with a copy of it taken at its beginning, all that hidden within two USE_ASSERT_CHECKING blocks, when we'd expect the tree to not change. With readOnlyTree, that would be just changing from one policy to another, which is not really interesting at this stage based on how ProcessUtility() is shaped. -- Michael signature.asc Description: PGP signature
Re: Internal error codes triggered by tests
On Fri, May 17, 2024 at 01:41:29PM -0400, Robert Haas wrote: > On Tue, Apr 23, 2024 at 12:55 AM Michael Paquier wrote: >> Thoughts? > > The patch as proposed seems fine. Marking RfC. Thanks. I'll look again at that once v18 opens up for business. -- Michael signature.asc Description: PGP signature
Re: Why does pgindent's README say to download typedefs.list from the buildfarm?
On Fri, May 17, 2024 at 10:24:57AM +0900, Michael Paquier wrote: > Yep. I can handle that in 2~3 hours. And done with 110eb4aefbad. If there's anything else, feel free to let me know. -- Michael signature.asc Description: PGP signature
Re: Why does pgindent's README say to download typedefs.list from the buildfarm?
On Thu, May 16, 2024 at 09:09:36PM -0400, Tom Lane wrote: > WFM, and this is probably a place where we don't want to change the > API in v17 and again in v18, so I agree with pushing now. > > Reminder though: beta1 release freeze begins Saturday. > Not many hours left. Yep. I can handle that in 2~3 hours. -- Michael signature.asc Description: PGP signature
Re: WAL record CRC calculated incorrectly because of underlying buffer modification
On Thu, May 16, 2024 at 03:54:52PM -0700, Jeff Davis wrote: > On Mon, 2024-05-13 at 11:15 +1200, Thomas Munro wrote: >> Perhaps a no-image, no-change registered buffer should not be >> including an image, even for XLR_CHECK_CONSISTENCY? It's >> actually >> useless for consistency checking too I guess, this issue >> aside, >> because it doesn't change anything so there is nothing to >> check. > > I'm not convinced by that reasoning. Can't it check that nothing has > changed? That's something I've done four weeks ago in the hash replay code path, and having the image with XLR_CHECK_CONSISTENCY even if REGBUF_NO_CHANGE was necessary because replay was setting up a LSN on a REGBUF_NO_CHANGE page it should not have touched. > I'd prefer that we find a way to get rid of REGBUF_NO_CHANGE and make > all callers follow the rules than to find new special cases that depend > on REGBUF_NO_CHANGE. See these messages here: > > https://www.postgresql.org/message-id/b1f2d0f230d60fa8df33bb0b2af3236fde9d750d.camel%40j-davis.com > > https://www.postgresql.org/message-id/CA%2BTgmoY%2BdagCyrMKau7UQeQU6w4LuVEu%2ByjsmJBoXKAo6XbUUA%40mail.gmail.com > > In other words, we added REGBUF_NO_CHANGE for the call sites (only hash > indexes) that don't follow the rules and where it wasn't easy to make > them follow the rules. Now that we know of a concrete problem with the > design, there's more upside to fixing it properly. Yeah, agreed that getting rid of REGBUF_NO_CHANGE would be nice in the final picture. It still strikes me as a weird concept that WAL replay for hash indexes logs full pages just to be able to lock them at replay based on what's in the records. :/ -- Michael signature.asc Description: PGP signature
Re: Why does pgindent's README say to download typedefs.list from the buildfarm?
On Thu, May 16, 2024 at 10:45:18AM -0400, Tom Lane wrote:
> I am also quite confused by that. It seems to be kind of an enum
> that is supposed to be extended at runtime, meaning that neither
> of the existing enum member values ought to be used as such, although
> either autoprewarm.c didn't get the memo or I misunderstand the
> intended usage. NUM_BUILTIN_WAIT_EVENT_EXTENSION is possibly the
> most bizarre idea I've ever seen: what would a "built-in extension"
> event be exactly? I think the enum should be nuked altogether, but
> it's a bit late to be redesigning that for v17 perhaps.
You're right, WaitEventExtension is better gone. The only thing that
matters is that we want to start computing the IDs assigned to the
custom wait events for extensions with a number higher than the
existing WAIT_EXTENSION to avoid interferences in pg_stat_activity, so
this could be cleaned up as the attached.
The reason why autoprewarm.c does not have a custom wait event
assigned is that it does not make sense there: this would not show up
in pg_stat_activity. I think that we should just switch back to
PG_WAIT_EXTENSION there and call it a day.
I can still clean up that in time for beta1, as in today's time. But
that can wait, as well. Thoughts?
--
Michael
diff --git a/src/include/utils/wait_event.h b/src/include/utils/wait_event.h
index 080e92d1cf..1b735d4a0e 100644
--- a/src/include/utils/wait_event.h
+++ b/src/include/utils/wait_event.h
@@ -53,12 +53,6 @@ extern PGDLLIMPORT uint32 *my_wait_event_info;
*
* The ID retrieved can be used with pgstat_report_wait_start() or equivalent.
*/
-typedef enum
-{
- WAIT_EVENT_EXTENSION = PG_WAIT_EXTENSION,
- WAIT_EVENT_EXTENSION_FIRST_USER_DEFINED,
-} WaitEventExtension;
-
extern void WaitEventExtensionShmemInit(void);
extern Size WaitEventExtensionShmemSize(void);
diff --git a/src/backend/utils/activity/wait_event.c b/src/backend/utils/activity/wait_event.c
index 4ffcb10c8b..084a9dfdc2 100644
--- a/src/backend/utils/activity/wait_event.c
+++ b/src/backend/utils/activity/wait_event.c
@@ -89,8 +89,7 @@ typedef struct WaitEventExtensionCounterData
static WaitEventExtensionCounterData *WaitEventExtensionCounter;
/* first event ID of custom wait events for extensions */
-#define NUM_BUILTIN_WAIT_EVENT_EXTENSION \
- (WAIT_EVENT_EXTENSION_FIRST_USER_DEFINED - WAIT_EVENT_EXTENSION)
+#define WAIT_EVENT_EXTENSION_INITIAL_ID 1
/* wait event info for extensions */
#define WAIT_EVENT_EXTENSION_INFO(eventId) (PG_WAIT_EXTENSION | eventId)
@@ -129,7 +128,7 @@ WaitEventExtensionShmemInit(void)
if (!found)
{
/* initialize the allocation counter and its spinlock. */
- WaitEventExtensionCounter->nextId = NUM_BUILTIN_WAIT_EVENT_EXTENSION;
+ WaitEventExtensionCounter->nextId = WAIT_EVENT_EXTENSION_INITIAL_ID;
SpinLockInit(>mutex);
}
@@ -244,7 +243,7 @@ GetWaitEventExtensionIdentifier(uint16 eventId)
WaitEventExtensionEntryById *entry;
/* Built-in event? */
- if (eventId < NUM_BUILTIN_WAIT_EVENT_EXTENSION)
+ if (eventId < WAIT_EVENT_EXTENSION_INITIAL_ID)
return "Extension";
/* It is a user-defined wait event, so lookup hash table. */
diff --git a/contrib/pg_prewarm/autoprewarm.c b/contrib/pg_prewarm/autoprewarm.c
index 248b9914a3..1c8804dc43 100644
--- a/contrib/pg_prewarm/autoprewarm.c
+++ b/contrib/pg_prewarm/autoprewarm.c
@@ -226,7 +226,7 @@ autoprewarm_main(Datum main_arg)
(void) WaitLatch(MyLatch,
WL_LATCH_SET | WL_EXIT_ON_PM_DEATH,
-1L,
- WAIT_EVENT_EXTENSION);
+ PG_WAIT_EXTENSION);
}
else
{
@@ -253,7 +253,7 @@ autoprewarm_main(Datum main_arg)
(void) WaitLatch(MyLatch,
WL_LATCH_SET | WL_TIMEOUT | WL_EXIT_ON_PM_DEATH,
delay_in_ms,
- WAIT_EVENT_EXTENSION);
+ PG_WAIT_EXTENSION);
}
/* Reset the latch, loop. */
signature.asc
Description: PGP signature
Re: open items
On Tue, May 14, 2024 at 09:52:35AM -0400, Robert Haas wrote: > We are down to three open items, all of which have proposed fixes. > That is great, but we need to keep things moving along, because > according to https://wiki.postgresql.org/wiki/PostgreSQL_17_Open_Items > we are due to release beta1 on May 23. That means that a release > freeze will be in effect from Saturday, May 18, which is four days > from now. Since committing patches sometimes leads to unexpected > surprises, it would be best if the proposed fixes were put into place > sooner rather than later, to allow time for any adjustments that may > be required. As of this minute, the open item list is empty @-@. Thanks all for the various resolutions, updates and commits! -- Michael signature.asc Description: PGP signature
Re: Underscore in positional parameters?
On Thu, May 16, 2024 at 08:41:11AM +0200, Peter Eisentraut wrote: > On this specific patch, maybe reword "parameter too large" to "parameter > number too large". WFM here. > Also, I was bemused by the use of atol(), which is notoriously unportable > (sizeof(long)). So I poked around and found more places that might need > fixing. I'm attaching a patch here with annotations too look at later. Yeah atoXX calls have been funky in the tree for some time. This reminds this thread, somewhat: https://www.postgresql.org/message-id/CALAY4q8be6Qw_2J%3DzOp_v1X-zfMBzvVMkAfmMYv%3DUkr%3D2hPcFQ%40mail.gmail.com The issue is also that there is no "safe" parsing alternative for 64b integers in the frontend (as you know long is 32b in Windows, which is why I'd encourage ripping it out as much as we can). This may be better as a complementary of strtoint() in src/common/string.c. Note as well strtoint64() in pgbench.c. I think I have a patch lying around, actually.. -- Michael signature.asc Description: PGP signature
Re: Fix src/test/subscription/t/029_on_error.pl test when wal_debug is enabled
On Thu, May 16, 2024 at 09:00:47AM +0530, Amit Kapila wrote: > This can only be a problem if the apply worker generates more LOG > entries with the required context but it won't do that unless there is > an operation on the publisher to replicate. If we see any such danger > then I agree this can break on some slow machines but as of now, I > don't see any such race condition. Perhaps, still I'm not completely sure if this assumption is going to always stand for all the possible configurations we support. So, rather than coming back to this test again, I would choose to make the test as stable as possible from the start. That's what mapping with the error message would offer when grabbing the LSN from the CONTEXT part of it, because there's a one-one mapping between the expected ERROR and its CONTEXT from which the information used by the test is retrieved. -- Michael signature.asc Description: PGP signature
Re: Postgres and --config-file option
On Thu, May 16, 2024 at 11:57:10AM +0300, Aleksander Alekseev wrote:
> I propose my original v1 patch for correcting the --help output of
> 'postgres' too. I agree with the above comments that corresponding
> changes in v4 became somewhat unwieldy.
Thanks for compiling the rest.
-printf(_(" --NAME=VALUE set run-time parameter\n"));
+printf(_(" --NAME=VALUE set run-time parameter, a shorter form of
-c\n"));
This part with cross-references in the output is still meh to me, for
same reason as for the doc changes I've argued to discard upthread.
write_stderr("%s does not know where to find the server configuration
file.\n"
- "You must specify the --config-file or -D invocation "
+ "You must specify the --config-file (or equivalent -c) or
-D invocation "
I can fall behind changing this one, still I'm not sure if this
proposal is the optimal choice. Adding this option to --help makes
sense when applied to this error message, but that's incomplete in
regard with the other GUCs where this concept applies. A different
approach would be to do nothing in --help and change the reference of
--config-file to -c config_name=VALUE, which would be in line with
--help.
--
Michael
signature.asc
Description: PGP signature
Re: Simplify documentation related to Windows builds
On Wed, May 15, 2024 at 11:25:34AM -0400, Robert Haas wrote: > I think that we need to get a more definitive answer to the question > of whether command-line editing works or not. I have the impression > that it never has. If it's started working, we should establish that > for certain and probably also establish what made it start working; if > it works provided you do X, Y, or Z, we should establish what those > things are. Right. > I'm cool with adding diff to the list of dependencies. This makes sense also to me, still the patch is not completely right because it has been adding diff in the list for build dependencies. Perhaps this should just be a new list. > I'd prefer to see us update the other links rather than delete them. Okay. I'm not sure where this patch is going, so I am going to withdraw it for now. The state of Windows is going to be a topic at the next pgconf.dev, anyway. -- Michael signature.asc Description: PGP signature
Re: query_id, pg_stat_activity, extended query protocol
On Wed, May 15, 2024 at 06:36:23PM +, Imseih (AWS), Sami wrote: >> Okay, that's what I precisely wanted to understand: queryId doesn't have >> semantics to show the job that consumes resources right now—it is mostly >> about convenience to know that the backend processes nothing except >> (probably) this query. > > It may be a good idea to expose in pg_stat_activity or a > supplemental activity view information about the current state of the > query processing. i.e. Is it parsing, planning or executing a query or > is it processing a nested query. pg_stat_activity is already quite bloated with attributes, and I'd suspect that there are more properties in a query that would be interesting to track down at a thinner level as long as it mirrors a dynamic activity of the query. Perhaps a separate catalog like a pg_stat_query would make sense, moving query_start there as well? Catalog breakages are never fun, still always happen because the reasons behind a backward-incompatible change make the picture better in the long-term for users. -- Michael signature.asc Description: PGP signature
Re: Postgres and --config-file option
On Wed, May 15, 2024 at 04:47:27PM +0200, Jelte Fennema-Nio wrote: > I definitely think it would be useful to list this --config variant in > more places, imho it's nicer than the -c variant. Especially in the > PGOPTIONS docs it would be useful. People are already using it in the > wild and I regressed on support for that in PgBouncer by accident: > https://github.com/pgbouncer/pgbouncer/pull/1064 Agreed that mentioning the --name variant is useful. I'm not really on board with having one option refer to the other on the pages where both are described, like on --help or the doc page for "postgres". For now, I've applied a patch for the libpq.sgml and config.sgml bits which are improvements of their own. -- Michael signature.asc Description: PGP signature
Re: Log details for stats dropped more than once
On Wed, May 15, 2024 at 08:04:48AM +, Bertrand Drouvot wrote: > Thanks! BTW, I just realized that adding more details for this error has > already > been mentioned in [1] (and Andres did propose a slightly different version). > > [1]: > https://www.postgresql.org/message-id/20240505160915.6boysum4f34siqct%40awork3.anarazel.de Ah, it is not surprising. I'd agree with doing what is posted there for simplicity's sake. Rather than putting that in a errdetail, let's keep all the information in an errmsg() as that makes deparsing easier, and let's keep the elog(). -- Michael signature.asc Description: PGP signature
Re: Underscore in positional parameters?
On Wed, May 15, 2024 at 01:59:36PM +0200, Peter Eisentraut wrote: > On 14.05.24 18:07, Erik Wienhold wrote: >> Patch 0002 replaces atol with pg_strtoint32_safe in the backend parser >> and strtoint in ECPG. This fixes overflows like: > > Seems like a good idea, but as was said, this is an older issue, so let's > look at that separately. Hmm, yeah. I would be really tempted to fix that now. Now, it has been this way for ages, and with my RMT hat on (aka I need to show the example), I'd suggest to wait for when the v18 branch opens as there is no urgency. I'm OK to apply it myself at the end, the patch is a good idea. -- Michael signature.asc Description: PGP signature
Re: Introduce new multi insert Table AM and improve performance of various SQL commands with it for Heap AM
On Wed, May 15, 2024 at 11:14:14AM +0200, Alvaro Herrera wrote: > We don't use IAM anywhere, for example (it's always "index AM"), and I > don't think we'd turn "sequence AM" into SAM either, would we? SAM is not a term I've seen used for sequence AMs in the past, I don't intend to use it. TAM is similar strange to me, but perhaps it's just because I am used to table AMs as a whole. -- Michael signature.asc Description: PGP signature
Re: Fix src/test/subscription/t/029_on_error.pl test when wal_debug is enabled
On Wed, May 15, 2024 at 05:58:18PM +0530, Amit Kapila wrote:
> I guess it could be more work if we want to enhance the test for
> ERRORs other than the primary key violation.
And? You could pass the ERROR string expected as argument of the
function if more flexibility is wanted at some point, no? It happens
that you don't require that now, which is fine for the three scenarios
calling test_skip_lsn.
> One simple fix is to
> update the log_offset to the location of the LOG after successful
> replication of un-conflicted data. For example, the Log location after
> we execute the below line in the test:
>
> # Check replicated data
>my $res =
> $node_subscriber->safe_psql('postgres', "SELECT
> count(*) FROM tbl");
>is($res, $expected, $msg);
That still looks like a shortcut to me, weak to race conditions on
slow machines where more log entries could be generated in-between.
So it seems to me that you'd still want to make sure that the CONTEXT
from which the LSN is retrieved maps to the sole expected error. This
is not going to be stable unless there are stronger checks to avoid
log entries that can parasite the output, and a stronger matching
ensures that.
--
Michael
signature.asc
Description: PGP signature
Re: SQL:2011 application time
On Tue, May 14, 2024 at 01:33:46PM +0800, jian he wrote: > thanks for the idea, I roughly played around with it, seems doable. > but the timing seems not good, reverting is a good idea. Please note that this is still an open item, and that time is running short until beta1. A revert seems to be the consensus reached, so, Peter, are you planning to do so? -- Michael signature.asc Description: PGP signature
Re: Log details for stats dropped more than once
On Tue, May 14, 2024 at 10:07:14AM +, Bertrand Drouvot wrote: > While resuming the work on refilenode stats (mentioned in [1] but did not > share > the patch yet), I realized that my current POC patch is buggy enough to > produce > things like: > > 024-05-14 09:51:14.783 UTC [1788714] FATAL: can only drop stats once > > While the CONTEXT provides the list of dropped stats: > > 2024-05-14 09:51:14.783 UTC [1788714] CONTEXT: WAL redo at 0/D75F478 for > Transaction/ABORT: 2024-05-14 09:51:14.782223+00; dropped stats: > 2/16384/27512/0 2/16384/27515/0 2/16384/27516/0 Can refcount be useful to know in this errcontext? > Attached a tiny patch to report the stat that generates the error. The patch > uses > errdetail_internal() as the extra details don't seem to be useful to average > users. I think that's fine. Overall that looks like useful information for debugging, so no objections from here. -- Michael signature.asc Description: PGP signature
Re: Fixup a few 2023 copyright years
On Wed, May 15, 2024 at 03:03:00PM +1200, David Rowley wrote: > On Tue, 9 Apr 2024 at 15:26, Michael Paquier wrote: >> I would suggest to also wait until we're clearer with the situation >> for all these mechanical changes, which I suspect is going to take 1~2 >> weeks at least. > > Since the Oid resequencing and pgindent run is now done, I've pushed this > patch. Thanks, that looks correct. While running src/tools/copyright.pl, I have noticed that that a newline was missing at the end of index_including.sql, as an effect of the test added by you in a63224be49b8. I've cleaned up that while on it, as it was getting added automatically, and we tend to clean these like in 3f1197191685 or more recently c2df2ed90a82. -- Michael signature.asc Description: PGP signature
Re: query_id, pg_stat_activity, extended query protocol
On Wed, May 15, 2024 at 03:24:05AM +, Imseih (AWS), Sami wrote: >> I think we should generally report it when the backend executes a job >> related to the query with that queryId. This means it would reset the >> queryId at the end of the query execution. > > When the query completes execution and the session goes into a state > other than "active", both the query text and the queryId should be of the > last executed statement. This is the documented behavior, and I believe > it's the correct behavior. > > If we reset queryId at the end of execution, this behavior breaks. Right? Idle sessions keep track of the last query string run, hence being consistent in pg_stat_activity and report its query ID is user friendly. Resetting it while keeping the string is less consistent. It's been this way for years, so I'd rather let it be this way. >> This seems logical, but >> what about the planning process? If an extension plans a query without >> the intention to execute it for speculative reasons, should we still >> show the queryId? Perhaps we should reset the state right after planning >> to accurately reflect the current queryId. > > I think you are suggesting that during planning, the queryId > of the current statement being planned should not be reported. > > If my understanding is correct, I don't think that is a good idea. Tools that > snasphot pg_stat_activity will not be able to account for the queryId during > planning. This could mean that certain load on the database cannot be tied > back to a specific queryId. I'm -1 with the point of resetting the query ID based on what the patch does, even if it remains available in the hooks. pg_stat_activity is one thing, but you would also reduce the coverage of log_line_prefix with %Q. And that can provide really useful debugging information in the code paths where the query ID would be reset as an effect of the proposed patch. The patch to report the query ID of a planned query when running a query through a PortalRunSelect() feels more intuitive in the information it reports. -- Michael signature.asc Description: PGP signature
Re: Underscore in positional parameters?
On Tue, May 14, 2024 at 06:07:51PM +0200, Erik Wienhold wrote:
> I split the change in two independent patches:
The split makes sense to me.
> Patch 0001 changes rules param and param_junk to only accept digits 0-9.
-param \${decinteger}
-param_junk \${decinteger}{ident_start}
+/* Positional parameters don't accept underscores. */
+param \${decdigit}+
+param_junk \${decdigit}+{ident_start}
scan.l, psqlscan.l and pgc.l are the three files impacted, so that's
good to me.
> Patch 0002 replaces atol with pg_strtoint32_safe in the backend parser
> and strtoint in ECPG. This fixes overflows like:
>
> => PREPARE p1 AS SELECT $4294967297; -- same as $1
> PREPARE
>
> It now returns this error:
>
> => PREPARE p1 AS SELECT $4294967297;
> ERROR: parameter too large at or near $4294967297
This one is a much older problem, though. What you are doing is an
improvement, still I don't see a huge point in backpatching that based
on the lack of complaints with these overflows in the yyac paths.
+if (errno == ERANGE)
+mmfatal(PARSE_ERROR, "parameter too large");
Knowong that this is working on decdigits, an ERANGE check should be
enough, indeed.
--
Michael
signature.asc
Description: PGP signature
Re: Fix src/test/subscription/t/029_on_error.pl test when wal_debug is enabled
On Tue, May 14, 2024 at 10:22:29AM +, Ilyasov Ian wrote:
> Hello, hackers!
>
> Recently I've been building postgres with different cflags and cppflags.
> And suddenly on REL_15_STABLE, REL_16_STABLE and master
> I faced a failure of a src/test/subscription/t/029_on_error.pl test when
> CPPFLAGS="-DWAL_DEBUG"
> and
> printf "wal_debug = on\n" >> "${TEMP_CONFIG}"
> (or when both publisher and subscriber or only subscriber are run with
> wal_debug=on)
>
> So I propose a little fix to the test.
Rather than assuming that the last line is the one to check, wouldn't
it be better to grab the data from the CONTEXT line located just after
the ERROR reporting the primary key violation?
A multi-line regexp, grabbing the LSN with more matching context based
on the ERROR and the DETAIL strings generating the CONTEXT we want
seems like a more stable alternative to me than grabbing the last line
of the logs.
--
Michael
signature.asc
Description: PGP signature
Re: Postgres and --config-file option
On Tue, May 14, 2024 at 03:03:58PM +0300, Aleksander Alekseev wrote:
> Here is the patch v4 with fixed typo ("geoq"). Per off-list feedback
> from Alvaro - thanks!
+ -c name=value command-line parameter, or its equivalent
+ --name=value variation. For example,
-postgres -c log_connections=yes -c log_destination='syslog'
+postgres -c log_connections=yes --log-destination='syslog'
Wow. I've used -c many times, and never noticed that this was a
supported option switch. There's always something to learn around
here..
-printf(_(" -c NAME=VALUE set run-time parameter\n"));
+printf(_(" -c NAME=VALUE set run-time parameter (see also
--NAME)\n"));
[...]
-printf(_(" --NAME=VALUE set run-time parameter\n"));
+printf(_(" --NAME=VALUE set run-time parameter, a shorter form of
-c\n"));
[...]
-to set multiple parameters.
+to set multiple parameters. See the --name
+option below for an alternate syntax.
[...]
-Sets a named run-time parameter; a shorter form of
--c.
+Sets the named run-time parameter; a shorter form of
+-c. See
+for a listing of parameters.
Not sure that these additions in --help or the docs are necessary.
The rest looks OK.
-"You must specify the --config-file or -D invocation "
+"You must specify the --config-file (or equivalent -c) or -D invocation "
How about "You must specify the --config-file, -c
\"config_file=VALUE\" or -D invocation"? There is some practice for
--opt=VALUE in .po files.
--
Michael
signature.asc
Description: PGP signature
Re: Avoid orphaned objects dependencies, take 3
On Thu, May 09, 2024 at 12:20:51PM +, Bertrand Drouvot wrote:
> Oh I see, your test updates an existing dependency. v4 took care about brand
> new
> dependencies creation (recordMultipleDependencies()) but forgot to take care
> about changing an existing dependency (which is done in another code path:
> changeDependencyFor()).
>
> Please find attached v5 that adds:
>
> - a call to the new depLockAndCheckObject() function in changeDependencyFor().
> - a test when altering an existing dependency.
>
> With v5 applied, I don't see the issue anymore.
+if (object_description)
+ereport(ERROR, errmsg("%s does not exist",
object_description));
+else
+ereport(ERROR, errmsg("a dependent object does not ex
This generates an internal error code. Is that intended?
--- /dev/null
+++
b/src/test/modules/test_dependencies_locks/specs/test_dependencies_locks.spec
This introduces a module with only one single spec. I could get
behind an extra module if splitting the tests into more specs makes
sense or if there is a restriction on installcheck. However, for
one spec file filed with a bunch of objects, and note that I'm OK to
let this single spec grow more for this range of tests, it seems to me
that this would be better under src/test/isolation/.
+ if (use_dirty_snapshot)
+ {
+ InitDirtySnapshot(DirtySnapshot);
+ snapshot =
+ }
+ else
+ snapshot = NULL;
I'm wondering if Robert has a comment about that. It looks backwards
in a world where we try to encourage MVCC snapshots for catalog
scans (aka 568d4138c646), especially for the part related to
dependency.c and ObjectAddresses.
--
Michael
signature.asc
Description: PGP signature
Re: explain format json, unit for serialize and memory are different.
On Wed, May 15, 2024 at 02:01:21AM +1200, David Rowley wrote: > Yeah, I missed that. Here's another patch. > > Thanks for looking. Thanks for bringing in a patch that makes the whole picture more consistent across the board. When it comes to MEMORY, I can get behind your suggestion to use kB and call it a day, while SERIALIZE would apply the same conversion at 1023b. It would be nice to document the units implied in the non-text formats, rather than have the users guess what these are. Perhaps Alvaro and Tom would like to chime in, as committers of respectively 5de890e3610d and 06286709ee06? -- Michael signature.asc Description: PGP signature
Re: Underscore in positional parameters?
On Tue, May 14, 2024 at 10:51:41AM +0100, Dean Rasheed wrote: > I've moved this to "Older bugs affecting stable branches", since it > came in with v16. Oops, thanks for fixing. I've somewhat missed that b2d47928908d was in REL_16_STABLE. -- Michael signature.asc Description: PGP signature
Re: pgsql: Fix overread in JSON parsing errors for incomplete byte sequence
On Tue, May 14, 2024 at 10:39:36AM +0200, Peter Eisentraut wrote: > I saw the same thing. The problem is that there is incomplete dependency > information in the makefiles (not meson) between src/common/ and what is > using it. So whenever anything changes in src/common/, you pretty much have > to do a forced rebuild of everything. Is that a recent regression? I have some blurry memories from working on these areas that changing src/common/ reflected on the compiled pieces effectively at some point. -- Michael signature.asc Description: PGP signature
Re: Underscore in positional parameters?
On Tue, May 14, 2024 at 05:18:24AM +0200, Erik Wienhold wrote:
> Parameter $1_2 is taken as $1 because in rule {param} in scan.l we get
> the parameter number with atol which stops at the underscore. That's a
> regression in faff8f8e47f. Before that commit, $1_2 resulted in
> "ERROR: trailing junk after parameter".
Indeed, the behavior of HEAD is confusing. "1_2" means 12 as a
constant in a query, not 1, but HEAD implies 1 in the context of
PREPARE here.
> I can't tell which fix is the way to go: (1) accept underscores without
> using atol, or (2) just forbid underscores. Any ideas?
Does the SQL specification tell anything about the way parameters
should be marked? Not everything out there uses dollar-marked
parameters, so I guess that the answer to my question is no. My take
is all these cases should be rejected for params, only apply to
numeric and integer constants in the queries.
> atol can be replaced with pg_strtoint32_safe to handle the underscores.
> This also avoids atol's undefined behavior on overflows. AFAICT,
> positional parameters are not part of the SQL standard, so nothing
> prevents us from accepting underscores here as well. The attached patch
> does that and also adds a test case.
>
> But reverting {param} to its old form to forbid underscores also makes
> sense. That is:
>
> param \${decdigit}+
> param_junk\${decdigit}+{ident_start}
>
> It seems very unlikely that anybody uses that many parameters and still
> cares about readability to use underscores. But maybe users simply
> expect that underscores are valid here as well.
Adding Dean in CC as the committer of faff8f8e47f, Peter E for the SQL
specification part, and an open item.
--
Michael
signature.asc
Description: PGP signature
Re: BitmapHeapScan streaming read user and prelim refactoring
On Mon, May 13, 2024 at 10:05:03AM -0400, Melanie Plageman wrote: > Remove the assert and reset the field on which it previously asserted to > avoid incorrectly emitting NULL-filled tuples from a previous scan on > rescan. > - Assert(scan->rs_empty_tuples_pending == 0); > + scan->rs_empty_tuples_pending = 0; Perhaps this should document the reason why the reset is done in these two paths rather than let the reader guess it? And this is about avoiding emitting some tuples from a previous scan. > +SET enable_indexonlyscan = off; > +set enable_indexscan = off; > +SET enable_seqscan = off; Nit: adjusting the casing of the second SET here. -- Michael signature.asc Description: PGP signature
Re: Fix resource leak (src/backend/libpq/be-secure-common.c)
On Mon, May 13, 2024 at 08:06:57PM +0200, Daniel Gustafsson wrote: >> Any chance we'll have these fixes in v17? > > Nice timing, I was actually rebasing them today to get them committed. Looks sensible seen from here, as these paths could use a LOG or rely on a memory context permanent to the backend causing junk to be accumulated. It's not that much, still that would accumulate. -- Michael signature.asc Description: PGP signature
Re: explain format json, unit for serialize and memory are different.
On Mon, May 13, 2024 at 11:22:08AM +0200, Daniel Gustafsson wrote:
> Since json (and yaml/xml) is intended to be machine-readable I think we use a
> single unit for all values, and document this fact.
Agreed with the documentation gap. Another thing that could be worth
considering is to add the units aside with the numerical values, say:
"Memory Used": {"value": 23432, "units": "bytes"}
That would require changing ExplainProperty() so as the units are
showed in some shape, while still being readable when parsed. I
wouldn't recommend doing that in v17, but perhaps v18 could do better?
Units are also ignored for the XML and yaml outputs.
--
Michael
signature.asc
Description: PGP signature
Re: [PATCH] Fix bug when calling strncmp in check_authmethod_valid
On Mon, May 13, 2024 at 01:01:21PM +0300, Aleksander Alekseev wrote: >> Any objections to fixing this in 17 by removing it? (cc:ing Michael from the >> RMT) > > +1 Something that is not documented or used by anyone (apparently) and > is broken should just be removed. 8a02339e9ba3 sounds like an argument good enough to prove there is no demand in the field for being able to support options through initdb --auth, and this does not concern only pam. If somebody is interested in that, that could always be done later. My take is that this would be simpler if implemented through a separate option, leaving the checks between the options and the auth method up to the postmaster when loading pg_hba.conf at startup. Hence, no objections to clean up that now. Thanks for asking. -- Michael signature.asc Description: PGP signature
Re: GUC-ify walsender MAX_SEND_SIZE constant
On Thu, Apr 25, 2024 at 02:53:33PM +0200, Majid Garoosi wrote: > Unfortunately, I quit that company a month ago (I wish we could > discuss this earlier) and don't have access to the environment > anymore. > I'll try to ask my teammates and see if they can find anything about > the exact values of latency, bw, etc. > > Anyway, here is some description of the environment. Sadly, there > are no numbers in this description, but I'll try to provide as much details > as possible. > There is a k8s cluster running over some VMs. Each postgres > instance runs as a pod inside the k8s cluster. So, both the > primary and standby servers are in the same DC, but might be on > different baremetal nodes. There is an overlay network for the pods to > see each other, and there's also another overlay network for the VMs > to see each other. > The storage servers are in the same DC, but we're sure they're on some > racks other than the postgres pods. They run Ceph [1] project and provide > Rados Block Devices (RBD) [2] interface. In order for k8s to use ceph, a > Ceph-CSI [3] controller is running inside the k8s cluster. > BTW, the FS type is ext4. Okay, seeing the feedback for this patch and Andres disagreeing with it as being a good idea, I have marked the patch as rejected as it was still in the CF app. -- Michael signature.asc Description: PGP signature
Re: race condition in pg_class
On Sun, May 12, 2024 at 04:29:23PM -0700, Noah Misch wrote:
> I'm attaching patches implementing the LockTuple() design. It turns out we
> don't just lose inplace updates. We also overwrite unrelated tuples,
> reproduced at inplace.spec. Good starting points are README.tuplock and the
> heap_inplace_update_scan() header comment.
About inplace050-tests-inj-v1.patch.
+ /* Check if blocked_pid is in injection_wait(). */
+ proc = BackendPidGetProc(blocked_pid);
+ if (proc == NULL)
+ PG_RETURN_BOOL(false); /* session gone: definitely unblocked */
+ wait_event =
+
pgstat_get_wait_event(UINT32_ACCESS_ONCE(proc->wait_event_info));
+ if (wait_event && strncmp("INJECTION_POINT(",
+ wait_event,
+
strlen("INJECTION_POINT(")) == 0)
+ PG_RETURN_BOOL(true);
Hmm. I am not sure that this is the right interface for the job
because this is not only related to injection points but to the
monitoring of a one or more wait events when running a permutation
step. Perhaps this is something that should be linked to the spec
files with some property area listing the wait events we're expected
to wait on instead when running a step that we know will wait?
--
Michael
signature.asc
Description: PGP signature
Re: pgsql: Fix overread in JSON parsing errors for incomplete byte sequence
On Fri, May 10, 2024 at 02:23:09PM +0200, Alvaro Herrera wrote: > Ah, I ran 'git clean -dfx' and now it works correctly. I must have had > an incomplete rebuild. I am going to assume that this is an incorrect build. It seems to me that src/common/ was compiled with a past version not sufficient to make the new test pass as more bytes got pushed to the error output as the pre-855517307db8 code could point to some random junk. -- Michael signature.asc Description: PGP signature
Re: Weird test mixup
On Sat, May 11, 2024 at 11:45:33AM +0500, Andrey M. Borodin wrote: > I see that you store condition in private_data. So "private" means > that this is a data specific to extension, do I understand it right? Yes, it is possible to pass down some custom data to the callbacks registered, generated in a module. One example would be more complex condition grammar, like a JSON-based thing. I don't really see the need for this amount of complexity in the tree yet, but one could do that outside the tree easily. > As long as I started anyway, I also want to ask some more stupid > questions: > 1. Where is the border between responsibility of an extension and > the core part? I mean can we define in simple words what > functionality must be in extension? Rule 0 I've been using here: keep the footprint on the backend as simple as possible. These have as absolute minimum requirement: - A function name. - A library name. - A point name. The private area contents and size are added to address the concurrency cases with runtime checks. I didn't see a strong use for that first, but Noah has been convincing enough with his use cases and the fact that the race between detach and run was not completely closed because we lacked consistency with the shmem hash table lookup. > 2. If we have some concurrency issues, why can't we just protect > everything with one giant LWLock\SpinLock. We have some locking > model instead of serializing access from enter until exit. This reduces the test infrastructure flexibility, because one may want to attach or detach injection points while a point is running. So it is by design that the LWLock protecting the shmem hash table is not hold when a point is running. This has been covered a bit upthread, and I want to be able to do that as well. -- Michael signature.asc Description: PGP signature
Re: Weird test mixup
On Sun, May 12, 2024 at 10:48:51AM -0700, Noah Misch wrote: > This looks correct, and it works well in my tests. Thanks. Thanks for looking. While looking at it yesterday I've decided to split the change into two commits, one for the infra and one for the module. While doing so, I've noticed that the case of a private area passed as NULL was not completely correct as memcpy would be undefined. The open item has been marked as fixed. -- Michael signature.asc Description: PGP signature
Re: Use pgBufferUsage for block reporting in analyze
On Fri, May 10, 2024 at 10:54:07AM +0200, Anthonin Bonnefoy wrote:
> This patch replaces those Vacuum specific variables by pgBufferUsage
> in analyze. This makes VacuumPage{Hit,Miss,Dirty} unused and removable.
> This commit removes both their calls in bufmgr and their declarations.
Hmm, yeah, it looks like you're right. I can track all the blocks
read, hit and dirtied for VACUUM and ANALYZE in all the code path
where these removed variables were incremented. This needs some
runtime check to make sure that the calculations are consistent before
and after the fact (cannot do that now).
appendStringInfo(, _("buffer usage: %lld hits, %lld misses,
%lld dirtied\n"),
- (long long) AnalyzePageHit,
- (long long) AnalyzePageMiss,
- (long long) AnalyzePageDirty);
+ (long long) (bufferusage.shared_blks_hit +
bufferusage.local_blks_hit),
+ (long long) (bufferusage.shared_blks_read +
bufferusage.local_blks_read),
+ (long long) (bufferusage.shared_blks_dirtied +
bufferusage.local_blks_dirtied));
Perhaps this should say "read" rather than "miss" in the logs as the
two read variables for the shared and local blocks are used? For
consistency, at least.
That's not material for v17, only for v18.
--
Michael
signature.asc
Description: PGP signature
Re: Weird test mixup
On Thu, May 09, 2024 at 04:39:00PM -0700, Noah Misch wrote:
Thanks for the feedback.
> The return-bool approach sounds fine. Up to you whether to do in this patch,
> else I'll do it when I add the test.
I see no reason to not change the signature of the routine now if we
know that we're going to do it anyway in the future. I was shortly
wondering if doing the same for InjectionpointAttach() would make
sense, but it has more error states, so I'm not really tempted without
an actual reason (cannot think of a case where I'd want to put more
control into a module after a failed attach).
>> It could
>> always be possible that a concurrent backend does a detach followed by
>> an attach with the same name, causing the shmem exit callback to drop
>> a point it should not, but that's not really a plausible case IMO :)
>
> Agreed. It's reasonable to expect test cases to serialize backend exits,
> attach calls, and detach calls. If we need to fix that later, we can use
> attachment serial numbers.
Okay by me.
> I'd name this INJ_CONDITION_UNCONDITIONAL or INJ_CONDITION_ALWAYS. INVALID
> sounds like a can't-happen event or an injection point that never runs.
> Otherwise, the patch looks good and makes src/test/modules/gin safe for
> installcheck. Thanks.
INJ_CONDITION_ALWAYS sounds like a good compromise here.
Attached is an updated patch for now, indented with a happy CI. I am
still planning to look at that a second time on Monday with a fresher
mind, in case I'm missing something now.
--
Michael
From 233e710c8fc2242bdd4e40d5a20f8de2828765b7 Mon Sep 17 00:00:00 2001
From: Michael Paquier
Date: Fri, 10 May 2024 09:40:42 +0900
Subject: [PATCH v4] Add chunk area for injection points
---
src/include/utils/injection_point.h | 9 +-
src/backend/utils/misc/injection_point.c | 53 -
.../expected/injection_points.out | 2 +-
.../injection_points/injection_points.c | 191 +++---
doc/src/sgml/xfunc.sgml | 14 +-
src/tools/pgindent/typedefs.list | 1 +
6 files changed, 131 insertions(+), 139 deletions(-)
diff --git a/src/include/utils/injection_point.h b/src/include/utils/injection_point.h
index 55524b568f..a61d5d4439 100644
--- a/src/include/utils/injection_point.h
+++ b/src/include/utils/injection_point.h
@@ -23,15 +23,18 @@
/*
* Typedef for callback function launched by an injection point.
*/
-typedef void (*InjectionPointCallback) (const char *name);
+typedef void (*InjectionPointCallback) (const char *name,
+ const void *private_data);
extern Size InjectionPointShmemSize(void);
extern void InjectionPointShmemInit(void);
extern void InjectionPointAttach(const char *name,
const char *library,
- const char *function);
+ const char *function,
+ const void *private_data,
+ int private_data_size);
extern void InjectionPointRun(const char *name);
-extern void InjectionPointDetach(const char *name);
+extern bool InjectionPointDetach(const char *name);
#endif /* INJECTION_POINT_H */
diff --git a/src/backend/utils/misc/injection_point.c b/src/backend/utils/misc/injection_point.c
index 0cf4d51cac..d5a8726644 100644
--- a/src/backend/utils/misc/injection_point.c
+++ b/src/backend/utils/misc/injection_point.c
@@ -42,6 +42,7 @@ static HTAB *InjectionPointHash; /* find points from names */
#define INJ_NAME_MAXLEN 64
#define INJ_LIB_MAXLEN 128
#define INJ_FUNC_MAXLEN 128
+#define INJ_PRIVATE_MAXLEN 1024
/* Single injection point stored in InjectionPointHash */
typedef struct InjectionPointEntry
@@ -49,6 +50,12 @@ typedef struct InjectionPointEntry
char name[INJ_NAME_MAXLEN]; /* hash key */
char library[INJ_LIB_MAXLEN]; /* library */
char function[INJ_FUNC_MAXLEN]; /* function */
+
+ /*
+ * Opaque data area that modules can use to pass some custom data to
+ * callbacks, registered when attached.
+ */
+ char private_data[INJ_PRIVATE_MAXLEN];
} InjectionPointEntry;
#define INJECTION_POINT_HASH_INIT_SIZE 16
@@ -61,6 +68,7 @@ typedef struct InjectionPointEntry
typedef struct InjectionPointCacheEntry
{
char name[INJ_NAME_MAXLEN];
+ char private_data[INJ_PRIVATE_MAXLEN];
InjectionPointCallback callback;
} InjectionPointCacheEntry;
@@ -73,7 +81,8 @@ static HTAB *InjectionPointCache = NULL;
*/
static void
injection_point_cache_add(const char *name,
- InjectionPointCallback callback)
+ InjectionPointCallback callback,
+ const void *private_data)
{
InjectionPointCacheEntry *entry;
bool found;
@@ -99,6 +108,7 @@ injection_point_cache_add(const char *name,
Assert(!found);
strlcpy(entry->name, name, sizeof(entry->name));
entry->callback = callback;
+ memcpy(entry->private_data, private_data, INJ_PRIVATE_MAXLEN);
}
/*
@@ -124,11 +134,14 @@ injection_point_cache_remove(const char *name)
* Retrieve an injection point
Re: open items
On Thu, May 09, 2024 at 03:28:13PM -0400, Robert Haas wrote: > Just a few reminders about the open items list at > https://wiki.postgresql.org/wiki/PostgreSQL_17_Open_Items -- Thanks for summarizing the situation. > * Race condition with local injection point detach. Discussion is ongoing. I have sent a patch for that yesterday, which I assume is going in the right direction to close entirely the loop: https://www.postgresql.org/message-id/Zjx9-2swyNg6E1y1%40paquier.xyz There is still one point of detail related to the amount of flexibility we'd want for detachs (concurrent detach happening in parallel of an automated one in the shmem callback) that I'm not entirely sure about yet but I've proposed an idea to solve that as well. I'm hopeful in getting that wrapped at the beginning of next week. -- Michael signature.asc Description: PGP signature
Re: Weird test mixup
On Thu, May 09, 2024 at 01:47:45PM +0900, Michael Paquier wrote:
> That sounds fine to do that at the end.. I'm not sure how large this
> chunk area added to each InjectionPointEntry should be, though. 128B
> stored in each InjectionPointEntry would be more than enough I guess?
> Or more like 1024? The in-core module does not need much currently,
> but larger is likely better for pluggability.
Actually, this is leading to simplifications in the module, giving me
the attached:
4 files changed, 117 insertions(+), 134 deletions(-)
So I like your suggestion. This version closes the race window
between the shmem exit detach in backend A and a concurrent backend B
running a point local to A so as B will never run the local point of
A. However, it can cause a failure in the shmem exit callback of
backend A if backend B does a detach of a point local to A because A
tracks its local points with a static list in its TopMemoryContext, at
least in the attached. The second case could be solved by tracking
the list of local points in the module's InjectionPointSharedState,
but is this case really worth the complications it adds in the module
knowing that the first case would be solid enough? Perhaps not.
Another idea I have for the second case is to make
InjectionPointDetach() a bit "softer", by returning a boolean status
rather than fail if the detach cannot be done, so as the shmem exit
callback can still loop through the entries it has in store. It could
always be possible that a concurrent backend does a detach followed by
an attach with the same name, causing the shmem exit callback to drop
a point it should not, but that's not really a plausible case IMO :)
This stuff can be adjusted in subtle ways depending on the cases you
are most interested in. What do you think?
--
Michael
From 27b0ba88d3530c50cb87d1495439372885f0773b Mon Sep 17 00:00:00 2001
From: Michael Paquier
Date: Thu, 9 May 2024 16:29:16 +0900
Subject: [PATCH v3] Add chunk area for injection points
---
src/include/utils/injection_point.h | 7 +-
src/backend/utils/misc/injection_point.c | 45 -
.../injection_points/injection_points.c | 189 +++---
doc/src/sgml/xfunc.sgml | 10 +-
4 files changed, 117 insertions(+), 134 deletions(-)
diff --git a/src/include/utils/injection_point.h b/src/include/utils/injection_point.h
index 55524b568f..a8a11de5f2 100644
--- a/src/include/utils/injection_point.h
+++ b/src/include/utils/injection_point.h
@@ -23,14 +23,17 @@
/*
* Typedef for callback function launched by an injection point.
*/
-typedef void (*InjectionPointCallback) (const char *name);
+typedef void (*InjectionPointCallback) (const char *name,
+ const void *private_data);
extern Size InjectionPointShmemSize(void);
extern void InjectionPointShmemInit(void);
extern void InjectionPointAttach(const char *name,
const char *library,
- const char *function);
+ const char *function,
+ const void *private_data,
+ int private_data_size);
extern void InjectionPointRun(const char *name);
extern void InjectionPointDetach(const char *name);
diff --git a/src/backend/utils/misc/injection_point.c b/src/backend/utils/misc/injection_point.c
index 0cf4d51cac..d46ad8b48f 100644
--- a/src/backend/utils/misc/injection_point.c
+++ b/src/backend/utils/misc/injection_point.c
@@ -42,6 +42,7 @@ static HTAB *InjectionPointHash; /* find points from names */
#define INJ_NAME_MAXLEN 64
#define INJ_LIB_MAXLEN 128
#define INJ_FUNC_MAXLEN 128
+#define INJ_PRIVATE_MAXLEN 1024
/* Single injection point stored in InjectionPointHash */
typedef struct InjectionPointEntry
@@ -49,6 +50,12 @@ typedef struct InjectionPointEntry
char name[INJ_NAME_MAXLEN]; /* hash key */
char library[INJ_LIB_MAXLEN]; /* library */
char function[INJ_FUNC_MAXLEN]; /* function */
+
+ /*
+ * Opaque data area that modules can use to pass some custom data to
+ * callbacks, registered when attached.
+ */
+ char private_data[INJ_PRIVATE_MAXLEN];
} InjectionPointEntry;
#define INJECTION_POINT_HASH_INIT_SIZE 16
@@ -61,6 +68,7 @@ typedef struct InjectionPointEntry
typedef struct InjectionPointCacheEntry
{
char name[INJ_NAME_MAXLEN];
+ char private_data[INJ_PRIVATE_MAXLEN];
InjectionPointCallback callback;
} InjectionPointCacheEntry;
@@ -73,7 +81,8 @@ static HTAB *InjectionPointCache = NULL;
*/
static void
injection_point_cache_add(const char *name,
- InjectionPointCallback callback)
+ InjectionPointCallback callback,
+ const void *private_data)
{
InjectionPointCacheEntry *entry;
bool found;
@@ -99,6 +108,7 @@ injection_point_cache_add(const char *name,
Assert(!found);
strlcpy(entry->name, name, sizeof(entry->name));
entry->callback = callback;
+ memcpy(entry->private_data, private_data, INJ_PRIVATE_MAXLEN);
}
/*
@@ -124,11 +134,14 @@ injection_point_cache_
Re: Weird test mixup
On Wed, May 08, 2024 at 08:15:53PM -0700, Noah Misch wrote: > Yes, that would be a loss for test readability. Also, I wouldn't be surprised > if some test will want to attach POINT-B while POINT-A is in injection_wait(). Not impossible, still annoying with more complex scenarios. > Various options avoiding those limitations: > > 1. The data area formerly called a "status" area is immutable after attach. >The core code copies it into a stack buffer to pass in a const callback >argument. > > 3. Move the PID check into core code. The PID checks are specific to the module, and there could be much more conditions like running only in specific backend types (first example coming into mind), so I want to avoid that kind of knowledge in the backend. > (1) is, I think, simple and sufficient. How about that? That sounds fine to do that at the end.. I'm not sure how large this chunk area added to each InjectionPointEntry should be, though. 128B stored in each InjectionPointEntry would be more than enough I guess? Or more like 1024? The in-core module does not need much currently, but larger is likely better for pluggability. -- Michael signature.asc Description: PGP signature
Re: [PATCH] json_lex_string: don't overread on bad UTF8
On Wed, May 08, 2024 at 07:01:08AM -0700, Jacob Champion wrote: > On Tue, May 7, 2024 at 10:31 PM Michael Paquier wrote: >> But looking closer, I can see that in the JSON_INVALID_TOKEN case, >> when !tok_done, we set token_terminator to point to the end of the >> token, and that would include an incomplete byte sequence like in your >> case. :/ > > Ah, I see what you're saying. Yeah, that approach would need some more > invasive changes. My first feeling was actually to do that, and report the location in the input string where we are seeing issues. All code paths playing with token_terminator would need to track that. > Agreed. Fortunately (or unfortunately?) I think the JSON > client-encoding work is now a prerequisite for OAuth in libpq, so > hopefully some improvements can fall out of that work too. I'm afraid so. I don't quite see how this would be OK to tweak on stable branches, but all areas that could report error states with partial byte sequence contents would benefit from such a change. >> Thoughts and/or objections? > > None here. This is a bit mitigated by the fact that d6607016c738 is recent, but this is incorrect since v13 so backpatched down to that. -- Michael signature.asc Description: PGP signature
Re: Weird test mixup
On Tue, May 07, 2024 at 11:53:10AM -0700, Noah Misch wrote: > On Tue, May 07, 2024 at 10:17:49AM +0900, Michael Paquier wrote: >> Always resetting >> condition->name when detaching a point is a simpler flow and saner >> IMO. >> >> Overall, this switches from one detach behavior to a different one, > > Can you say more about that? The only behavior change known to me is that a > given injection point workload uses more of INJ_MAX_CONDITION. If there's > another behavior change, it was likely unintended. As far as I read the previous patch, the conditions stored in InjectionPointSharedState would never be really gone, even if the points are removed from InjectionPointHash. > The problem I'm trying to tackle in this thread is to make > src/test/modules/gin installcheck-safe. $SUBJECT's commit 5105c90 started > that work, having seen the intarray test suite break when run concurrently > with the injection_points test suite. That combination still does break at > the exit-time race condition. To reproduce, apply this attachment to add > sleeps, and run: > > make -C src/test/modules/gin installcheck USE_MODULE_DB=1 & sleep 2; make -C > contrib/intarray installcheck USE_MODULE_DB=1 Thanks for that. I am not really sure how to protect that without a small cost in flexibility for the cases of detach vs run paths. This comes down to the fact that a custom routine could be run while it could be detached concurrently, removing any stuff a callback could depend on in the module. It was mentioned upthread to add to InjectionPointCacheEntry a fixed area of memory that modules could use to store some "status" data, but it would not close the run/detach race because a backend could still hold a pointer to a callback, with concurrent backends playing with the contents of InjectionPointCacheEntry (concurrent detaches and attaches that would cause the same entries to be reused). One way to close entirely the window would be to hold InjectionPointLock longer in InjectionPointRun() until the callback finishes or until it triggers an ERROR. This would mean that the case you've mentioned in [1] would change, by blocking the detach() of s3 until the callback of s2 finishes. We don't have tests in the tree that do any of that, so holding InjectionPointLock longer would not break anything on HEAD. A detach being possible while the callback is run is something I've considered as valid in d86d20f0ba79, but perhaps that was too cute of me, even more with the use case of local points. > Separately, I see injection_points_attach() populates InjectionPointCondition > after InjectionPointAttach(). Shouldn't InjectionPointAttach() come last, to > avoid the same sort of race? I've not tried to reproduce that one. Good point. You could run into the case of a concurrent backend running an injection point that should be local if waiting between InjectionPointAttach() and the condition getting registered in injection_points_attach(). That should be reversed. [1] https://www.postgresql.org/message-id/20240501231214...@rfd.leadboat.com -- Michael signature.asc Description: PGP signature
Re: add --no-sync to pg_upgrade's calls to pg_dump and pg_dumpall
On Wed, May 08, 2024 at 02:49:58PM -0400, Tom Lane wrote: > Nathan Bossart writes: >> Thanks for looking. I noticed that the version check is unnecessary since >> we always use the new binary's pg_dump[all], so I removed that in v2. > > +1 +1. Could there be an argument in favor of a backpatch? This is a performance improvement, but one could also side that the addition of sync support in pg_dump[all] has made that a regression that we'd better fix because the flushes don't matter in this context. They also bring costs for no gain. -- Michael signature.asc Description: PGP signature
Re: bug: copy progress reporting of backends which run multiple COPYs
On Wed, May 08, 2024 at 10:07:15AM -0400, Robert Haas wrote: > I think you're hoping for too much. The progress reporting > infrastructure is fundamentally designed around the idea that there > can only be one progress-reporting operation in progress at a time. > For COPY, that is, I believe, true, but for file_fdw, it's false. If > we want to do any kind of progress reporting from within plannable > queries, we need some totally different and much more complex > infrastructure that can report progress for, probably, each plan node > individually. I think it's likely a mistake to try to shoehorn cases > like this into the infrastructure > that we have today. It will just encourage people to try to use the > current infrastructure in ways that are less and less like what it was > actually designed to do; whereas what we should be doing if we want > this kind of functionality, at least IMHO, is building infrastructure > that's actually fit for purpose. Hmm. OK. I have been looking around for cases out there where BeginCopyFrom() could be called with a pstate where the reporting could matter, and could not find anything worth worrying about. It still makes me a bit uneasy to not have a separate argument in the function to control that. Now, if you, Justin and Matthias agree with this approach, I won't stand in the way either. -- Michael signature.asc Description: PGP signature
Re: [PATCH] json_lex_string: don't overread on bad UTF8
On Tue, May 07, 2024 at 02:06:10PM -0700, Jacob Champion wrote:
> Maybe I've misunderstood, but isn't that what's being done in v2?
Something a bit different.. I was wondering if it could be possible
to tweak this code to truncate the data in the generated error string
so as the incomplete multi-byte sequence is entirely cut out, which
would come to setting token_terminator to "s" (last byte before the
incomplete byte sequence) rather than "term" (last byte available,
even if incomplete):
#define FAIL_AT_CHAR_END(code) \
do { \
char *term = s + pg_encoding_mblen(lex->input_encoding, s); \
lex->token_terminator = (term <= end) ? term : s; \
return code; \
} while (0)
But looking closer, I can see that in the JSON_INVALID_TOKEN case,
when !tok_done, we set token_terminator to point to the end of the
token, and that would include an incomplete byte sequence like in your
case. :/
At the end of the day, I think that I'm OK with your patch and avoid
the overread for now in the back-branches. This situation makes me
uncomfortable and we should put more effort in printing error messages
in a readable format, but that could always be tackled later as a
separate problem.. And I don't see something backpatchable at short
sight for v16.
Thoughts and/or objections?
--
Michael
signature.asc
Description: PGP signature
Re: SQL function which allows to distinguish a server being in point in time recovery mode and an ordinary replica
On Mon, May 06, 2024 at 06:55:46PM +0300, m.litsa...@postgrespro.ru wrote:
> as a first step I have introduced the `SharedRecoveryDataFlags` bitmask
> instead of three boolean SharedHotStandbyActive, SharedPromoteIsTriggered
> and SharedStandbyModeRequested flags (the last one from my previous patch)
> and made minimal updates in corresponding code based on that change.
Thanks for the patch.
/*
- * Local copy of SharedHotStandbyActive variable. False actually means "not
+ * Local copy of XLR_HOT_STANDBY_ACTIVE flag. False actually means "not
* known, need to check the shared state".
*/
static bool LocalHotStandbyActive = false;
/*
- * Local copy of SharedPromoteIsTriggered variable. False actually means "not
+ * Local copy of XLR_PROMOTE_IS_TRIGGERED flag. False actually means "not
* known, need to check the shared state".
*/
static bool LocalPromoteIsTriggered = false;
It's a bit strange to have a bitwise set of flags in shmem while we
keep these local copies as booleans. Perhaps it would be cleaner to
merge both local variables into a single bits32 store?
+ uint32 SharedRecoveryDataFlags;
I'd switch to bits32 for flags here.
+bool
+StandbyModeIsRequested(void)
+{
+ /*
+* Spinlock is not needed here because XLR_STANDBY_MODE_REQUESTED flag
+* can only be read after startup process is done.
+*/
+ return (XLogRecoveryCtl->SharedRecoveryDataFlags &
XLR_STANDBY_MODE_REQUESTED) != 0;
+}
How about introducing a single wrapper function that returns the whole
value SharedRecoveryDataFlags, with the flags published in a header?
Sure, XLR_HOT_STANDBY_ACTIVE is not really exciting because being able
to query a standby implies it, but XLR_PROMOTE_IS_TRIGGERED could be
interesting? Then this could be used with a function that returns a
text[] array with all the states retrieved?
The refactoring pieces and the function pieces should be split, for
clarity.
--
Michael
signature.asc
Description: PGP signature
Re: pg_sequence_last_value() for unlogged sequences on standbys
On Tue, May 07, 2024 at 02:39:42PM -0500, Nathan Bossart wrote: > Okay, phew. We can still do something like v3-0002 for v18. I'll give > Michael a chance to comment on 0001 before committing/back-patching that > one. What you are doing in 0001, and 0002 for v18 sounds fine to me. -- Michael signature.asc Description: PGP signature
Re: Use pgstat_kind_infos to read fixed shared stats structs
On Tue, May 07, 2024 at 02:07:42PM -0500, Tristan Partin wrote: > On Tue May 7, 2024 at 1:29 PM CDT, Andres Freund wrote: >> I think it's a good idea. I'd really like to allow extensions to register new >> types of stats eventually. Stuff like pg_stat_statements having its own, >> fairly ... mediocre, stats storage shouldn't be necessary. > > Could be useful for Neon in the future too. Interesting thing to consider. If you do that, I'm wondering if you could, actually, lift the restriction on pg_stat_statements.max and make it a SIGHUP so as it could be increased on-the-fly.. Hmm, just a thought in passing. >> Do we need to increase the stats version, I didn't check if the order we >> currently store things in and the numerical order of the stats IDs are the >> same. > > I checked the orders, and they looked the same. > > 1. Archiver > 2. BgWriter > 3. Checkpointer > 4. IO > 5. SLRU > 6. WAL Yup, I've looked at that yesterday and the read order remains the same so I don't see a need for a version bump. -- Michael signature.asc Description: PGP signature
Re: bug: copy progress reporting of backends which run multiple COPYs
On Tue, May 07, 2024 at 07:27:54AM -0500, Justin Pryzby wrote: > This didn't get fixed for v16, and it seems unlikely that it'll be > addressed in back branches. > > But while I was reviewing forgotten threads, it occurred to me to raise > the issue in time to fix it for v17. Thanks for the reminder. FWIW, I'm rather annoyed by the fact that we rely on the ParseState to decide if reporting should happen or not. file_fdw tells, even if that's accidental, that status reporting can be useful if working on a single table. So, shutting down the whole reporting if a caller if BeginCopyFrom() does not give a ParseState is too heavy-handed, IMO. The addition of report_progress in the COPY FROM state data is a good idea, though isn't that something we should give as an argument of BeginCopyFrom() instead if sticking this knowledge in COPY FROM? Different idea: could it be something worth controlling with a query-level option? It would then be possible to provide the same level of control for COPY TO which has reporting paths, given the application control over the reporting even with file_fdw, and store the value controlling the reporting in CopyFormatOptions. I am wondering if there would be a case where someone wants to do a COPY but hide entirely the reporting done. The query-level option has some appeal. -- Michael signature.asc Description: PGP signature
Re: Cutting support for OpenSSL 1.0.1 and 1.0.2 in 17~?
On Tue, May 07, 2024 at 12:36:24PM +0200, Daniel Gustafsson wrote: > Fair enough. I've taken a stab at documenting that the functions are > deprecated, while at the same time documenting when and how they can be used > (and be useful). The attached also removes one additional comment in the > testcode which is now obsolete (since removing 1.0.1 support really), and > fixes > the spurious whitespace you detected upthread. + This function is deprecated and only present for backwards compatibility, + it does nothing. [...] +and + are maintained for backwards compatibility, but are no longer required + since PostgreSQL 18. LGTM, thanks for doing this! -- Michael signature.asc Description: PGP signature
