Re: Disallow changing slot's failover option in transaction block
On Tue, Apr 16, 2024 at 5:06 PM shveta malik wrote: > > On Tue, Apr 16, 2024 at 1:45 PM Hayato Kuroda (Fujitsu) > wrote: > > > > Dear Hou, > > > > > Kuroda-San reported an issue off-list that: > > > > > > If user execute ALTER SUBSCRIPTION SET (failover) command inside a txn > > > block > > > and rollback, only the subscription option change can be rolled back, > > > while the > > > replication slot's failover change is preserved. > > > > > > This is because ALTER SUBSCRIPTION SET (failover) command internally > > > executes > > > the replication command ALTER_REPLICATION_SLOT to change the replication > > > slot's > > > failover property, but this replication command execution cannot be > > > rollback. > > > > > > To fix it, I think we can prevent user from executing ALTER SUBSCRIPTION > > > set > > > (failover) inside a txn block, which is also consistent to the ALTER > > > SUBSCRIPTION REFRESH/DROP SUBSCRIPTION command. Attach a small > > > patch to address this. > > > > Thanks for posting the patch, the fix is same as my expectation. > > Also, should we add the restriction to the doc? I feel [1] can be updated. > > +1. > > Similar to ALTER SUB, CREATE SUB also needs to be fixed. This is > because we call alter_replication_slot in CREATE SUB as well, for the > case when slot_name is provided and create_slot=false. But the tricky > part is we call alter_replication_slot() when creating subscription > for both failover=true and false. That means if we want to fix it on > the similar line of ALTER SUB, we have to disallow user from executing > the CREATE SUBSCRIPTION (slot_name = xx) in a txn block, which seems > to break some existing use cases. (previously, user can execute such a > command inside a txn block). > > So, we need to think if there are better ways to fix it. After > discussion with Hou-San offlist, here are some ideas: > > 1. do not alter replication slot's failover option when CREATE > SUBSCRIPTION WITH failover=false. This means we alter replication > slot only when failover is set to true. And thus we can disallow > CREATE SUB WITH (slot_name =xx, failover=true, create_slot=false) > inside a txn block. > > This option allows user to run CREATE-SUB(create_slot=false) with > failover=false in txn block like earlier. But on the downside, it > makes the behavior inconsistent for otherwise simpler option like > failover, i.e. with failover=true, CREATE SUB is not allowed in txn > block while with failover=false, it is allowed. It makes it slightly > complex to be understood by user. > > 2. let's not disallow CREATE SUB in txn block as earlier, just don't > perform internal alter-failover during CREATE SUB for existing > slots(create_slot=false, slot_name=xx) i.e. when create_slot is > false, we will ignore failover parameter of CREATE SUB and it is > user's responsibility to set it appropriately using ALTER SUB cmd. For > create_slot=true, the behaviour of CREATE-SUB remains same as earlier. > > This option does not add new restriction for CREATE SUB wrt txn block. > In context of failover with create_slot=false, we already have a > similar restriction (documented one) for ALTER SUB, i.e. with 'ALTER > SUBSCRIPTION SET(slot_name = new)', user needs to alter the new slot's > failover by himself. CREAT SUB can also be documented in similar way. > This seems simpler to be understood considering existing ALTER SUB's > behavior as well. Plus, this will make CREATE-SUB code slightly > simpler and thus easily manageable. > +1 for option 2 as it sounds logical to me and consistent with ALTER SUBSCRIPTION. BTW, IIUC, you are referring to: "When altering the slot_name, the failover and two_phase property values of the named slot may differ from the counterpart failover and two_phase parameters specified in the subscription. When creating the slot, ensure the slot properties failover and two_phase match their counterpart parameters of the subscription." in docs [1], right? -- With Regards, Amit Kapila.
Re: WIP Incremental JSON Parser
On Tue, Apr 09, 2024 at 09:26:49AM -0700, Jacob Champion wrote: > On Tue, Apr 9, 2024 at 7:30 AM Andrew Dunstan wrote: >> I think Michael's point was that if we carry the code we should test we >> can run it. The other possibility would be just to remove it. I can see >> arguments for both. > > Hm. If it's not acceptable to carry this (as a worse-is-better smoke > test) without also running it during tests, then my personal vote > would be to tear it out and just have people write/contribute targeted > benchmarks when they end up working on performance. I don't think the > cost/benefit makes sense at that point. And you may catch up a couple of bugs while on it. In my experience, things with custom makefile and/or meson rules tend to rot easily because everybody forgets about them. There are a few of them in the tree that could be ripped off, as well.. -- Michael signature.asc Description: PGP signature
Re: allow changing autovacuum_max_workers without restarting
> Here is a first attempt at a proper patch set based on the discussion thus > far. I've split it up into several small patches for ease of review, which > is probably a bit excessive. If this ever makes it to commit, they could > likely be combined. I looked at the patch set. With the help of DEBUG2 output, I tested to ensure that the the autovacuum_cost_limit balance adjusts correctly when the autovacuum_max_workers value increases/decreases. I did not think the patch will break this behavior, but it's important to verify this. Some comments on the patch: 1. A nit. There should be a tab here. - dlist_head av_freeWorkers; + dclist_head av_freeWorkers; 2. autovacuum_max_worker_slots documentation: + +Note that the value of is +silently capped to this value. + This comment looks redundant in the docs, since the entry for autovacuum_max_workers that follows mentions the same. 3. The docs for autovacuum_max_workers should mention that when the value changes, consider adjusting the autovacuum_cost_limit/cost_delay. This is not something new. Even in the current state, users should think about these settings. However, it seems even important if this value is to be dynamically adjusted. Regards, Sami
Re: POC: GROUP BY optimization
On 4/12/24 06:44, Tom Lane wrote:
* It seems like root->processed_groupClause no longer has much to do
with the grouping behavior, which is scary given how much code still
believes that it does. I suspect there are bugs lurking there, and
1. Analysing the code, processed_groupClause is used in:
- adjust_foreign_grouping_path_cost - to decide, do we need to add cost
of sort to the foreign grouping.
- just for replacing relids during SJE process.
- create_groupingsets_plan(), preprocess_grouping_sets,
remove_useless_groupby_columns - we don't apply this feature in the case
of grouping sets.
- get_number_of_groups - estimation shouldn't depend on the order of
clauses.
- create_grouping_paths - to analyse hashability of grouping clause
- make_group_input_target, make_partial_grouping_target - doesn't depend
on the order of clauses
planner.c: add_paths_to_grouping_rel, create_partial_grouping_paths - in
the case of hash grouping.
So, the only case we can impact current behavior lies in the
postgres_fdw. But here we don't really know which plan will be chosen
during planning on foreign node and stay the same behavior is legal for me.
am not comforted by the fact that the patch changed exactly nothing
in the pathnodes.h documentation of that field. This comment looks
pretty silly now too:
/* Preprocess regular GROUP BY clause, if any */
root->processed_groupClause = list_copy(parse->groupClause);
What "preprocessing" is going on there? This comment was adequate
before, when the code line invoked preprocess_groupclause which had
a bunch of commentary; but now it has to stand on its own and it's
not doing a great job of that.
Thanks for picking this place. I fixed it.
More interesting here is that we move decisions on the order of group-by
clauses to the stage, where we already have underlying subtree and
incoming path keys. In principle, we could return the preprocessing
routine and arrange GROUP-BY with the ORDER-BY clause as it was before.
But looking into the code, I found only one reason to do this:
adjust_group_pathkeys_for_groupagg. Curious about how much profit we get
here, I think we can discover it later with no hurry. A good outcome
here will be a test case that can show the importance of arranging
GROUP-BY and ORDER-BY at an early stage.
--
regards,
Andrei Lepikhov
Postgres Professional
diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
index 5ea3705796..861656a192 100644
--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -1424,7 +1424,10 @@ grouping_planner(PlannerInfo *root, double tuple_fraction,
}
else if (parse->groupClause)
{
- /* Preprocess regular GROUP BY clause, if any */
+ /*
+ * Make a copy of origin groupClause because we are going to
+ * remove redundant clauses.
+ */
root->processed_groupClause = list_copy(parse->groupClause);
/* Remove any redundant GROUP BY columns */
remove_useless_groupby_columns(root);
improve performance of pg_dump --binary-upgrade
While examining pg_upgrade on a cluster with many tables (created with the
command in [0]), I noticed that a huge amount of pg_dump time goes towards
the binary_upgrade_set_pg_class_oids() function. This function executes a
rather expensive query for a single row, and this function appears to be
called for most of the rows in pg_class.
The attached work-in-progress patch speeds up 'pg_dump --binary-upgrade'
for this case. Instead of executing the query in every call to the
function, we can execute it once during the first call and store all the
required information in a sorted array that we can bsearch() in future
calls. For the aformentioned test, pg_dump on my machine goes from ~2
minutes to ~18 seconds, which is much closer to the ~14 seconds it takes
without --binary-upgrade.
One downside of this approach is the memory usage. This was more-or-less
the first approach that crossed my mind, so I wouldn't be surprised if
there's a better way. I tried to keep the pg_dump output the same, but if
that isn't important, maybe we could dump all the pg_class OIDs at once
instead of calling binary_upgrade_set_pg_class_oids() for each one.
[0] https://postgr.es/m/3612876.1689443232%40sss.pgh.pa.us
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
>From 27b4a3249dd97376f13a7c99505330ab7cd78e3f Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Wed, 17 Apr 2024 22:55:27 -0500
Subject: [PATCH v1 1/1] Improve performance of pg_dump --binary-upgrade.
---
src/bin/pg_dump/pg_dump.c| 113 +++
src/tools/pgindent/typedefs.list | 1 +
2 files changed, 70 insertions(+), 44 deletions(-)
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index c52e961b30..d93d974108 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -55,6 +55,7 @@
#include "catalog/pg_trigger_d.h"
#include "catalog/pg_type_d.h"
#include "common/connect.h"
+#include "common/int.h"
#include "common/relpath.h"
#include "compress_io.h"
#include "dumputils.h"
@@ -99,6 +100,17 @@ typedef enum OidOptions
zeroAsNone = 4,
} OidOptions;
+typedef struct
+{
+ Oid oid;
+ char relkind;
+ RelFileNumber relfilenode;
+ Oid reltoastrelid;
+ RelFileNumber toast_relfilenode;
+ Oid indexrelid;
+ RelFileNumber toast_index_relfilenode;
+} BinaryUpgradeClassOids;
+
/* global decls */
static bool dosync = true; /* Issue fsync() to make dump durable on disk. */
@@ -5392,19 +5404,56 @@ binary_upgrade_set_type_oids_by_rel(Archive *fout,
pg_type_oid, false, false);
}
+static int
+BinaryUpgradeClassOidsCmp(const void *p1, const void *p2)
+{
+ BinaryUpgradeClassOids v1 = *((const BinaryUpgradeClassOids *) p1);
+ BinaryUpgradeClassOids v2 = *((const BinaryUpgradeClassOids *) p2);
+
+ return pg_cmp_u32(v1.oid, v2.oid);
+}
+
static void
binary_upgrade_set_pg_class_oids(Archive *fout,
PQExpBuffer upgrade_buffer, Oid pg_class_oid,
bool is_index)
{
- PQExpBuffer upgrade_query = createPQExpBuffer();
- PGresult *upgrade_res;
- RelFileNumber relfilenumber;
- Oid toast_oid;
- RelFileNumber toast_relfilenumber;
- char relkind;
- Oid toast_index_oid;
- RelFileNumber toast_index_relfilenumber;
+ static BinaryUpgradeClassOids *oids = NULL;
+ static int oids_len = 0;
+ BinaryUpgradeClassOids key = {0, '?', 0, 0, 0, 0, 0};
+ BinaryUpgradeClassOids *entry;
+
+ if (oids == NULL)
+ {
+ PGresult *res;
+
+ res = ExecuteSqlQuery(fout,
+ "SELECT c.oid, c.relkind, c.relfilenode, c.reltoastrelid, ct.relfilenode AS toast_relfilenode, "
+ "i.indexrelid, cti.relfilenode AS toast_index_relfilenode "
+ "FROM pg_catalog.pg_class c LEFT JOIN "
+ "pg_catalog.pg_index i ON (c.reltoastrelid = i.indrelid AND i.indisvalid) "
+ "LEFT JOIN pg_catalog.pg_class ct ON (c.reltoastrelid = ct.oid) "
+ "LEFT JOIN pg_catalog.pg_class AS cti ON (i.indexrelid = cti.oid) "
+ "ORDER BY c.oid;",
+ PGRES_TUPLES_OK);
+
+ oids_len = PQntuples(res);
+ oids = (BinaryUpgradeClassOids *)
+ pg_malloc(oids_len * sizeof(BinaryUpgradeClassOids));
+
+ for (int i = 0; i < oids_len; i++)
+ {
+ oids[i].oid = atooid(PQgetvalue(res, i, 0));
+ oids[i].relkind = *PQgetvalue(res, i, 1);
+ oids[i].relfilenode = atooid(PQgetvalue(res, i, 2));
+ oids[i].reltoastrelid = atooid(PQgetvalue(res, i, 3));
+ oids[i].toast_relfilenode = atooid(PQgetvalue(res, i, 4));
+ oids[i].indexrelid = atooid(PQgetvalue(res, i, 5));
+ oids[i].toast_index_relfilenode = atooid(PQgetvalue(res, i, 6));
+ }
+
+ PQclear(res);
+ }
/*
* Preserve the OID and relfilenumber of the table, table's index, table's
@@ -5417,29 +5466,9 @@ binary_upgrade_set_pg_class_oids(Archive *fout,
* by the new backend, so we can copy the files during binary upgrade
* without worrying about this case.
*/
- appendPQExpBuffer(upgrade_query,
- "SELECT c.relkind, c.relfilenode, c.reltoastrelid, ct.relfilenode AS toast_relfilenode,
Re: Speed up clean meson builds by ~25%
Jelte Fennema-Nio writes: > As I expected this problem was indeed fairly easy to address by still > building "backend/parser" before "interfaces". See attached patch. I think we should hold off on this. I found a simpler way to address ecpg's problem than what I sketched upthread. I have a not-ready-to- show-yet patch that allows the vast majority of ecpg's grammar productions to use the default semantic action. Testing on my M1 Macbook with clang 16.0.6 from MacPorts, I see the compile time for preproc.o in HEAD as about 1m44 sec; but with this patch, 0.6 sec. The core idea of the patch is to get rid of results from grammar nonterminals and instead pass the strings back as yylloc results, which we can do by redefining YYLTYPE as "char *"; since ecpg isn't using Bison's location logic for anything, this is free. Then we can implement a one-size-fits-most token concatenation rule in YYLLOC_DEFAULT, and only the various handmade rules that don't want to just concatenate their inputs need to do something different. The patch presently passes regression tests, but its memory management is shoddy as can be (basically "leak like there's no tomorrow"), and I want to fix that before presenting it. One could almost argue that we don't care about memory consumption of the ecpg preprocessor; but I think it's possible to do better. regards, tom lane
Re: Popcount optimization using AVX512
It was brought to my attention [0] that we probably should be checking for the OSXSAVE bit instead of the XSAVE bit when determining whether there's support for the XGETBV instruction. IIUC that should indicate that both the OS and the processor have XGETBV support (not just the processor). I've attached a one-line patch to fix this. [0] https://github.com/pgvector/pgvector/pull/519#issuecomment-2062804463 -- Nathan Bossart Amazon Web Services: https://aws.amazon.com diff --git a/src/port/pg_popcount_avx512_choose.c b/src/port/pg_popcount_avx512_choose.c index ae3fa3d306..cc3e89e096 100644 --- a/src/port/pg_popcount_avx512_choose.c +++ b/src/port/pg_popcount_avx512_choose.c @@ -74,7 +74,7 @@ pg_popcount_avx512_available(void) #else #error cpuid instruction not available #endif - if ((exx[2] & (1 << 26)) == 0) /* xsave */ + if ((exx[2] & (1 << 27)) == 0) /* osxsave */ return false; /* Does XGETBV say the ZMM registers are enabled? */
Re: ALTER TABLE SET ACCESS METHOD on partitioned tables
On Wed, Apr 17, 2024 at 10:31:52AM +0200, Alvaro Herrera wrote:
> On 2024-Apr-17, Michael Paquier wrote:
>> Yeah, that would be easy enough to track but I was wondering about
>> adding the relkind instead. Still, one thing that I found confusing
>> is the dump generated in this case, as it would mix the SET and the
>> ALTER TABLE commands so one reading the dumps may wonder why the SET
>> has no effect for a CREATE TABLE PARTITION OF without USING. Perhaps
>> that's fine and I just worry too much ;)
>
> Hmm, maybe we should do a RESET of default_table_access_method before
> printing the CREATE TABLE to avoid the confusion.
A hard reset would make the business around currTableAM that decides
when to generate the SET default_table_access_method queries slightly
more complicated, while increasing the number of queries run on the
server.
>> The extra ALTER commands need to be generated after the object
>> definitions, so we'd need a new subroutine similar to
>> _selectTableAccessMethod() like a _selectTableAccessMethodNoStorage().
>> Or grouping both together is just simpler?
>
> I think there should be two routines, since _select* routines just print
> a SET command; maybe the new one would be _printAlterTableAM() or
> something like that. Having _select() print an ALTER TABLE command
> depending on relkind (or the boolean flag) would be confusing, I think.
Fine by me to use two routines to generate the two different commands.
I am finishing with the attached for now, making dumps, restores and
upgrades work happily as far as I've tested.
I was also worrying about a need to dump the protocol version to be
able to track the relkind in the toc entries, but a45c78e3284b has
already done one. The difference in AM handling between relations
without storage and relations with storage pushes the relkind logic
more within the internals of pg_backup_archiver.c.
What do you think?
--
Michael
From ca3d0c1828336275d86a5c51ed975c0a43ca6af9 Mon Sep 17 00:00:00 2001
From: Michael Paquier
Date: Thu, 18 Apr 2024 09:37:20 +0900
Subject: [PATCH v2] Set properly table AMs of partitioned tables in pg_dump
---
src/bin/pg_dump/pg_backup_archiver.c | 70 +++-
src/bin/pg_dump/pg_backup_archiver.h | 5 +-
src/bin/pg_dump/pg_dump.c| 1 +
src/bin/pg_dump/t/002_pg_dump.pl | 6 +--
4 files changed, 75 insertions(+), 7 deletions(-)
diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
index c7a6c918a6..f69f72d731 100644
--- a/src/bin/pg_dump/pg_backup_archiver.c
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -30,6 +30,7 @@
#include
#endif
+#include "catalog/pg_class_d.h"
#include "common/string.h"
#include "compress_io.h"
#include "dumputils.h"
@@ -62,6 +63,8 @@ static void _becomeOwner(ArchiveHandle *AH, TocEntry *te);
static void _selectOutputSchema(ArchiveHandle *AH, const char *schemaName);
static void _selectTablespace(ArchiveHandle *AH, const char *tablespace);
static void _selectTableAccessMethod(ArchiveHandle *AH, const char *tableam);
+static void _printTableAccessMethodNoStorage(ArchiveHandle *AH,
+ TocEntry *te);
static void processEncodingEntry(ArchiveHandle *AH, TocEntry *te);
static void processStdStringsEntry(ArchiveHandle *AH, TocEntry *te);
static void processSearchPathEntry(ArchiveHandle *AH, TocEntry *te);
@@ -1222,6 +1225,7 @@ ArchiveEntry(Archive *AHX, CatalogId catalogId, DumpId dumpId,
newToc->namespace = opts->namespace ? pg_strdup(opts->namespace) : NULL;
newToc->tablespace = opts->tablespace ? pg_strdup(opts->tablespace) : NULL;
newToc->tableam = opts->tableam ? pg_strdup(opts->tableam) : NULL;
+ newToc->relkind = opts->relkind;
newToc->owner = opts->owner ? pg_strdup(opts->owner) : NULL;
newToc->desc = pg_strdup(opts->description);
newToc->defn = opts->createStmt ? pg_strdup(opts->createStmt) : NULL;
@@ -2602,6 +2606,7 @@ WriteToc(ArchiveHandle *AH)
WriteStr(AH, te->namespace);
WriteStr(AH, te->tablespace);
WriteStr(AH, te->tableam);
+ WriteInt(AH, te->relkind);
WriteStr(AH, te->owner);
WriteStr(AH, "false");
@@ -2707,6 +2712,9 @@ ReadToc(ArchiveHandle *AH)
if (AH->version >= K_VERS_1_14)
te->tableam = ReadStr(AH);
+ if (AH->version >= K_VERS_1_16)
+ te->relkind = ReadInt(AH);
+
te->owner = ReadStr(AH);
is_supported = true;
if (AH->version < K_VERS_1_9)
@@ -3567,6 +3575,51 @@ _selectTableAccessMethod(ArchiveHandle *AH, const char *tableam)
AH->currTableAm = pg_strdup(want);
}
+/*
+ * Set the proper default table access method for a table without storage,
+ * like a partitioned one where a table access method may be set.
+ */
+static void
+_printTableAccessMethodNoStorage(ArchiveHandle *AH, TocEntry *te)
+{
+ RestoreOptions *ropt = AH->public.ropt;
+ const char *tableam = te->tableam;
+ PQExpBuffer cmd;
+
+ /* do nothing in --no-table-access-method mode */
+ if (ropt->noTableAm)
+ return;
+
+ if (!tableam)
+ return;
+
+ Assert(te->relkind
Re: sql/json remaining issue
On Mon, Apr 15, 2024 at 9:46 PM Amit Langote wrote: > On Sat, Apr 13, 2024 at 11:12 PM jian he wrote: > > On Fri, Apr 12, 2024 at 5:44 PM Amit Langote > > wrote: > > > > > > > elog(ERROR, "unrecognized json wrapper %d", wrapper); > > > > should be > > > > elog(ERROR, "unrecognized json wrapper %d", (int) wrapper); > > > > > > Fixed in 0003. > > > > > the fix seems not in 0003? > > other than that, everything looks fine. Oops, really fixed now in 0002. > I've combined these patches into one -- attached 0001. Will push tomorrow. Decided to break the error message improvement patch into its own after all -- attached 0001. > Now studying the JsonBehavior DEFAULT expression issue and your patch. I found some more coercion-related expression nodes that must also be checked along with CoerceViaIO and CoerceToDomain. Also, after fixing the code to allow them, I found that we'd need to also check recursively whether their argument expression is also one of the supported expression nodes. Also, I decided that it's not necessary to include "cast" in the error message as one of the supported expressions. Will push all today. -- Thanks, Amit Langote v5-0002-SQL-JSON-Miscellaneous-fixes-and-improvements.patch Description: Binary data v5-0001-SQL-JSON-Improve-some-error-messages.patch Description: Binary data v5-0003-SQL-JSON-Fix-issues-with-DEFAULT-.-ON-ERROR-EMPTY.patch Description: Binary data
Re: Cannot find a working 64-bit integer type on Illumos
On Sat, Mar 23, 2024 at 3:23 PM Tom Lane wrote: > Thomas Munro writes: > > . o O ( int64_t, PRIdi64, etc were standardised a quarter of a century ago ) > > Yeah. Now that we require C99 it's probably reasonable to assume > that those things exist. I wouldn't be in favor of ripping out our > existing notations like UINT64CONST, because the code churn would be > substantial and the gain minimal. But we could imagine reimplementing > that stuff atop and then getting rid of the configure-time > probes. I played around with this a bit, but am not quite there yet. printf() is a little tricky. The standard wants us to use 's PRId64 etc, but that might confuse our snprintf.c (in theory, probably not in practice). "ll" should have the right size on all systems, but gets warnings from the printf format string checker on systems where "l" is the right type. So I think we still need to probe for INT64_MODIFIER at configure-time. Here's one way, but I can see it's not working on Clang/Linux... perhaps instead of that dubious incantation I should try compiling some actual printfs and check for warnings/errors. I think INT64CONST should just point to standard INT64_C(). For limits, why do we have this: - * stdint.h limits aren't guaranteed to have compatible types with our fixed - * width types. So just define our own. ? I mean, how could they not have compatible types? I noticed that configure.ac checks if int64 (no "_t") might be defined already by system header pollution, but meson.build doesn't. That's an inconsistency that should be fixed, but which way? Hmm, commit 15abc7788e6 said that was done for BeOS, which we de-supported. So maybe we should get rid of that? 0001-Use-int64_t-and-friends-from-stdint.h.patch Description: Binary data 0002-Remove-traces-of-BeOS.patch Description: Binary data
Re: pg_combinebackup fails on file named INCREMENTAL.*
On 4/18/24 00:14, Robert Haas wrote: On Tue, Apr 16, 2024 at 9:25 AM Robert Haas wrote: On Mon, Apr 15, 2024 at 10:12 PM David Steele wrote: Anyway, I think it should be fixed or documented as a caveat since it causes a hard failure on restore. Alright, I'll look into this. Here's a patch. Thanks! I've tested this and it works as advertised. Ideally I'd want an error on backup if there is a similar file in any data directories that would cause an error on combine, but I admit that it is vanishingly rare for users to put files anywhere but the root of PGDATA, so this approach seems OK to me. Regards, -David
Re: pg_combinebackup does not detect missing files
On 4/18/24 01:03, Robert Haas wrote: On Tue, Apr 16, 2024 at 7:25 PM David Steele wrote: But it will not go out of its way to perform checks that are unrelated to its documented purpose. And I don't think it should, especially if we have another tool that already does that. I'm not averse to having some kind of statement in the documentation along the lines of "Note that pg_combinebackup does not attempt to verify that the individual backups are intact; for that, use pg_verifybackup." I think we should do this at a minimum. Here is a patch to do that. I think here: + pg_basebackup only attempts to verify you mean: + pg_combinebackup only attempts to verify Otherwise this looks good to me. Especially given how pg_combinebackup works, backups are going to undergo a lot of user manipulation (pushing to and pull from storage, decompressing, untaring, etc.) and I think that means we should take extra care. We are in agreement on that point, if nothing else. I am terrified of users having problems with pg_combinebackup and me not being able to tell whether those problems are due to user error, Robert error, or something else. I put a lot of effort into detecting dumb things that I thought a user might do, and a lot of effort into debugging output so that when things do go wrong anyway, we have a reasonable chance of figuring out exactly where they went wrong. We do seem to have a philosophical difference about what the scope of those checks ought to be, and I don't really expect what I wrote above to convince you that my position is correct, but perhaps it will convince you that I have a thoughtful position, as opposed to just having done stuff at random. Fair enough. I accept that your reasoning is not random, but I'm still not very satisfied that the user needs to run a separate and rather expensive process to do the verification when pg_combinebackup already has the necessary information at hand. My guess is that most users will elect to skip verification. At least now they'll have the information they need to make an informed choice. Regards, -David
Re: pgsql: meson: Add initial version of meson based build system
Hi,
Uh, huh.
On 2024-04-17 15:42:28 +0200, Christoph Berg wrote:
> Re: Andres Freund
> > https://git.postgresql.org/pg/commitdiff/e6927270cd18d535b77cbe79c55c6584351524be
>
> This commit broke VPATH builds when the original source directory
> contains symlinks.
I.e. a symlink to the source directory, not a symlink inside the source
directory.
> Given there are no other changes I think this bit is at fault:
>
> > Modified Files
> > --
> > configure.ac |6 +
>
> +# Ensure that any meson build directories would reconfigure and see that
> +# there's a conflicting in-tree build and can error out.
> +if test "$vpath_build"="no"; then
> + touch meson.build
> +fi
Argh, this is missing spaces around the '=', leading to the branch always
being entered.
What I don't understand is how that possibly could affect the prep_buildtree
step, that happens earlier.
Hm.
Uh, I don't think it does? Afaict this failure is entirely unrelated to 'touch
meson.build'? From what I can tell the problem is that config/prep_buildtree
is invoked with the symlinked path, and that that doesn't seem to work:
bash -x /home/andres/src/postgresql-via-symlink/config/prep_buildtree
/home/andres/src/postgresql-via-symlink .
++ basename /home/andres/src/postgresql-via-symlink/config/prep_buildtree
+ me=prep_buildtree
+ help='Usage: prep_buildtree sourcetree [buildtree]'
+ test -z /home/andres/src/postgresql-via-symlink
+ test x/home/andres/src/postgresql-via-symlink = x--help
+ unset CDPATH
++ cd /home/andres/src/postgresql-via-symlink
++ pwd
+ sourcetree=/home/andres/src/postgresql-via-symlink
++ cd .
++ pwd
+ buildtree=/tmp/pgs
++ find /home/andres/src/postgresql-via-symlink -type d '(' '(' -name CVS
-prune ')' -o '(' -name .git -prune ')' -o -print ')'
++ grep -v '/home/andres/src/postgresql-via-symlink/doc/src/sgml/\+'
++ find /home/andres/src/postgresql-via-symlink -name Makefile -print -o -name
GNUmakefile -print
++ grep -v /home/andres/src/postgresql-via-symlink/doc/src/sgml/images/
+ exit 0
Note that the find does not return anything.
Greetings,
Andres Freund
Re: fix tablespace handling in pg_combinebackup
Hi, On 2024-04-17 16:16:55 -0400, Robert Haas wrote: > In the "Differential code coverage between 16 and HEAD" thread, Andres > pointed out that there wasn't test case coverage for > pg_combinebackup's code to handle files in tablespaces. I looked at > adding that, and as nobody could possibly have predicted, found a bug. Ha ;) > @@ -787,8 +787,13 @@ Does not start the node after initializing it. > > By default, the backup is assumed to be plain format. To restore from > a tar-format backup, pass the name of the tar program to use in the > -keyword parameter tar_program. Note that tablespace tar files aren't > -handled here. > +keyword parameter tar_program. > + > +If there are tablespace present in the backup, include tablespace_map as > +a keyword parameter whose values is a hash. When tar_program is used, the > +hash keys are tablespace OIDs; otherwise, they are the tablespace pathnames > +used in the backup. In either case, the values are the tablespace pathnames > +that should be used for the target cluster. Where would one get these oids? Could some of this be simplified by using allow_in_place_tablespaces instead? Looks like it'd simplify at least the extended test somewhat? Greetings, Andres Freund
plenty code is confused about function level static
Hi,
We have a fair amount of code that uses non-constant function level static
variables for read-only data. Which makes little sense - it prevents the
compiler from understanding
a) that the data is read only and can thus be put into a segment that's shared
between all invocations of the program
b) the data will be the same on every invocation, and thus from optimizing
based on that.
The most common example of this is that all our binaries use
static struct option long_options[] = { ... };
which prevents long_options from being put into read-only memory.
Is there some reason we went for this pattern in a fair number of places? I
assume it's mostly copy-pasta, but...
In practice it often is useful to use 'static const' instead of just
'const'. At least gcc otherwise soemtimes fills the data on the stack, instead
of having a read-only data member that's already initialized. I'm not sure
why, tbh.
Attached are fixes for struct option and a few more occurrences I've found
with a bit of grepping.
There are lots of places that could benefit from adding 'static
const'.
E.g. most, if not all, HASHCTL's should be that, but that's more verbose to
change, so I didn't do that.
Greetings,
Andres Freund
>From d43a10b5ba46b010c8e075b1062f9f30eb013498 Mon Sep 17 00:00:00 2001
From: Andres Freund
Date: Wed, 17 Apr 2024 14:27:03 -0700
Subject: [PATCH v1 1/3] static const: Convert struct option arrays
---
src/bin/initdb/initdb.c | 2 +-
src/bin/pg_amcheck/pg_amcheck.c | 2 +-
src/bin/pg_archivecleanup/pg_archivecleanup.c | 2 +-
src/bin/pg_basebackup/pg_basebackup.c | 2 +-
src/bin/pg_basebackup/pg_createsubscriber.c | 2 +-
src/bin/pg_basebackup/pg_receivewal.c | 2 +-
src/bin/pg_basebackup/pg_recvlogical.c| 2 +-
src/bin/pg_checksums/pg_checksums.c | 2 +-
src/bin/pg_combinebackup/pg_combinebackup.c | 2 +-
src/bin/pg_controldata/pg_controldata.c | 2 +-
src/bin/pg_ctl/pg_ctl.c | 2 +-
src/bin/pg_dump/pg_dump.c | 2 +-
src/bin/pg_dump/pg_dumpall.c | 2 +-
src/bin/pg_resetwal/pg_resetwal.c | 2 +-
src/bin/pg_rewind/pg_rewind.c | 2 +-
src/bin/pg_test_fsync/pg_test_fsync.c | 2 +-
src/bin/pg_test_timing/pg_test_timing.c | 2 +-
src/bin/pg_upgrade/option.c | 2 +-
src/bin/pg_verifybackup/pg_verifybackup.c | 2 +-
src/bin/pg_waldump/pg_waldump.c | 2 +-
src/bin/pg_walsummary/pg_walsummary.c | 2 +-
src/bin/pgbench/pgbench.c | 2 +-
src/bin/psql/startup.c| 2 +-
src/bin/scripts/clusterdb.c | 2 +-
src/bin/scripts/createdb.c| 2 +-
src/bin/scripts/createuser.c | 2 +-
src/bin/scripts/dropdb.c | 2 +-
src/bin/scripts/dropuser.c| 2 +-
src/bin/scripts/pg_isready.c | 2 +-
src/bin/scripts/reindexdb.c | 2 +-
src/bin/scripts/vacuumdb.c| 2 +-
contrib/btree_gist/btree_interval.c | 2 +-
contrib/oid2name/oid2name.c | 2 +-
contrib/vacuumlo/vacuumlo.c | 2 +-
src/interfaces/ecpg/preproc/ecpg.c| 2 +-
src/test/regress/pg_regress.c | 2 +-
36 files changed, 36 insertions(+), 36 deletions(-)
diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c
index 30e17bd1d1e..5d4044d5a90 100644
--- a/src/bin/initdb/initdb.c
+++ b/src/bin/initdb/initdb.c
@@ -3093,7 +3093,7 @@ initialize_data_directory(void)
int
main(int argc, char *argv[])
{
- static struct option long_options[] = {
+ static const struct option long_options[] = {
{"pgdata", required_argument, NULL, 'D'},
{"encoding", required_argument, NULL, 'E'},
{"locale", required_argument, NULL, 1},
diff --git a/src/bin/pg_amcheck/pg_amcheck.c b/src/bin/pg_amcheck/pg_amcheck.c
index 7e3101704d4..bb100022a0d 100644
--- a/src/bin/pg_amcheck/pg_amcheck.c
+++ b/src/bin/pg_amcheck/pg_amcheck.c
@@ -233,7 +233,7 @@ main(int argc, char *argv[])
uint64 relprogress = 0;
int pattern_id;
- static struct option long_options[] = {
+ static const struct option long_options[] = {
/* Connection options */
{"host", required_argument, NULL, 'h'},
{"port", required_argument, NULL, 'p'},
diff --git a/src/bin/pg_archivecleanup/pg_archivecleanup.c b/src/bin/pg_archivecleanup/pg_archivecleanup.c
index 07bf356b70c..6cdf471597e 100644
--- a/src/bin/pg_archivecleanup/pg_archivecleanup.c
+++ b/src/bin/pg_archivecleanup/pg_archivecleanup.c
@@ -285,7 +285,7 @@ usage(void)
int
main(int argc, char **argv)
{
- static struct option long_options[] = {
+ static const struct option long_options[] = {
{"clean-backup-history", no_argument, NULL, 'b'},
{"debug", no_argument, NULL, 'd'},
{"dry-run", no_argument, NULL, 'n'},
diff --git a/src/bin/pg_basebackup/pg_basebackup.c
fix tablespace handling in pg_combinebackup
In the "Differential code coverage between 16 and HEAD" thread, Andres pointed out that there wasn't test case coverage for pg_combinebackup's code to handle files in tablespaces. I looked at adding that, and as nobody could possibly have predicted, found a bug. Here's a 2-patch series to (1) enhance PostgreSQL::Test::Utils::init_from_backup to handle tablespaces and then (2) fix the bug in pg_combinebackup and add test coverage using the facilities from the first patch. -- Robert Haas EDB: http://www.enterprisedb.com 0001-Make-PostgreSQL-Test-Cluster-init_from_backup-handle.patch Description: Binary data 0002-pg_combinebackup-Fix-incorrect-tablespace-handling.patch Description: Binary data
Re: Removing GlobalVisTestNonRemovableHorizon
On 2024-04-16 07:32:55 +0900, Michael Paquier wrote: > On Mon, Apr 15, 2024 at 11:57:20AM -0700, Andres Freund wrote: > > GlobalVisTestNonRemovableHorizon()/GlobalVisTestNonRemovableFullHorizon() > > only > > existed for snapshot_too_old - but that was removed in f691f5b80a8. I'm > > inclined to think we should remove those functions for 17. No new code > > should > > use them. > > RMT hat on. Feel free to go ahead and clean up that now. No > objections from here as we don't want to take the risk of this stuff > getting more used in the wild. Cool. Pushed the removal..
Re: Removing GlobalVisTestNonRemovableHorizon
Hi, On 2024-04-15 15:13:51 -0400, Robert Haas wrote: > It would of course have been nice to have done this sooner, but I don't > think waiting for next release cycle will make anything better. I don't really know how it could have been discovered sooner. We don't have any infrastructure for finding code that's not used anymore. And even if we had something finding symbols that aren't referenced within the backend, we have lots of functions that are just used by extensions, which would thus appear unused. In my local build we have several hundred functions that are not used within the backend, according to -Wl,--gc-sections,--print-gc-sections. A lot of that is entirely expected stuff, like RegisterXactCallback(). But there are also long-unused things like TransactionIdIsActive(). Greetings, Andres Freund
Re: Solaris tar issues, or other reason why margay fails 010_pg_basebackup?
Thomas Munro writes: > This test suite is passing on pollock because it doesn't have IO::Pty > installed. Could you try uninstalling that perl package for now, so > we can see what breaks next? If that's inconvenient for some reason, you could also skip the tab-completion test by setting SKIP_READLINE_TESTS in the animal's build_env options. regards, tom lane
Re: documentation structure
Andres Freund writes: > Hi, > > On 2024-04-17 12:07:24 +0100, Dagfinn Ilmari Mannsåker wrote: >> Andres Freund writes: >> > I think the manual work for writing signatures in sgml is not >> > insignificant, >> > nor is the volume of sgml for them. Manually maintaining the signatures >> > makes >> > it impractical to significantly improve the presentation - which I don't >> > think >> > is all that great today. >> >> And it's very inconsistent. For example, some functions use >> tags for optional parameters, others use square brackets, and some use >> VARIADIC to indicate variadic parameters, others use >> ellipses (sometimes in tags or brackets). > > That seems almost inevitably the outcome of many people having to manually > infer the recommended semantics, for writing something boring but nontrivial, > from a 30k line file. As Corey mentioned elsethread, having a markup style guide (maybe a comment at the top of the file?) would be nice. >> > And the lack of argument names in the pg_proc entries is occasionally >> > fairly >> > annoying, because a \df+ doesn't provide enough information to use >> > functions. >> >> I was also annoyed by this the other day (specifically wrt. the boolean >> arguments to pg_ls_dir), > > My bane is regexp_match et al, I have given up on remembering the argument > order. There's a thread elsewhere about those specifically, but I can't be bothered to find the link right now. >> and started whipping up a Perl script to parse func.sgml and generate >> missing proargnames values for pg_proc.dat, which is how I discovered the >> above. > > Nice. > >> The script currently has a pile of hacky regexes to cope with that, >> so I'd be happy to submit a doc patch to turn it into actual markup to get >> rid of that, if people think that's a worhtwhile use of time and won't clash >> with any other plans for the documentation. > > I guess it's a bit hard to say without knowing how voluminious the changes > would be. If we end up rewriting the whole file the tradeoff is less clear > than if it's a dozen inconsistent entries. It turned out to not be that many that used [] for optional parameters, see the attached patch. I havent dealt with variadic yet, since the two styles are visually different, not just markup (... renders as [...]). The two styles for variadic are the what I call caller-style: concat ( val1 "any" [, val2 "any" [, ...] ] ) format(formatstr text [, formatarg "any" [, ...] ]) which shows more clearly how you'd call it, versus definition-style: num_nonnulls ( VARIADIC "any" ) jsonb_extract_path ( from_json jsonb, VARIADIC path_elems text[] ) which matches the CREATE FUNCTION statement. I don't have a strong opinion on which we should use, but we should be consistent. > Greetings, > > Andres Freund - ilmari >From f71e0669eb25b205bd5065f15657ba6d749261f3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= Date: Wed, 17 Apr 2024 16:00:52 +0100 Subject: [PATCH] func.sgml: Consistently use to indicate optional parameters Some functions were using square brackets instead. --- doc/src/sgml/func.sgml | 54 +- 1 file changed, 27 insertions(+), 27 deletions(-) diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml index 8dfb42ad4d..afaaf61d69 100644 --- a/doc/src/sgml/func.sgml +++ b/doc/src/sgml/func.sgml @@ -3036,7 +3036,7 @@ concat concat ( val1 "any" - [, val2 "any" [, ...] ] ) + , val2 "any" [, ...] ) text @@ -3056,7 +3056,7 @@ concat_ws ( sep text, val1 "any" -[, val2 "any" [, ...] ] ) +, val2 "any" [, ...] ) text @@ -3076,7 +3076,7 @@ format format ( formatstr text -[, formatarg "any" [, ...] ] ) +, formatarg "any" [, ...] ) text @@ -3170,7 +3170,7 @@ parse_ident parse_ident ( qualified_identifier text -[, strict_mode boolean DEFAULT true ] ) +, strict_mode boolean DEFAULT true ) text[] @@ -3309,8 +3309,8 @@ regexp_count regexp_count ( string text, pattern text - [, start integer - [, flags text ] ] ) + , start integer + , flags text ) integer @@ -3331,11 +3331,11 @@ regexp_instr regexp_instr ( string text, pattern text - [, start integer - [, N integer - [, endoption integer - [, flags text - [, subexpr integer ] ] ] ] ] ) + , start integer + , N integer + , endoption integer + , flags text + , subexpr integer ) integer @@ -3360,7 +3360,7 @@ regexp_like regexp_like ( string text, pattern text - [, flags text ] )
Idea Feedback: psql \h misses -> Offers Links?
Hackers,
I often use the ctrl-click on the link after getting help in psql. A
great feature.
Challenge, when there is no help, you don't get any link.
My thought process is to add a default response that would take them to
https://www.postgresql.org/search/?u=%2Fdocs%2F16%2F={TOKEN}
*Example:*
\h current_setting
No help available for "current_setting".
Try \h with no arguments to see available help.
https://www.postgresql.org/search/?u=%2Fdocs%2F16%2F=current_setting
To me, this is a huge step in helping me get to the docs.
This is Question 1: Do others see the potential value here?
Question 2: What if we allowed the users to set some extra link Templates
using \pset??
\pset help_assist_link_1 = https://www.google.com/search?q={token}'
\pset help_assist_link_2 = '
https://wiki.postgresql.org/index.php?title=Special%3ASearch={token}=Go
'
Such that the output, this time would be:
*Example:*
\h current_setting
No help available for "current_setting".
Try \h with no arguments to see available help.
https://www.postgresql.org/search/?u=%2Fdocs%2F16%2F=current_setting
https://www.google.com/search?q=current_setting
https://wiki.postgresql.org/index.php?title=Special%3ASearch=current_setting=Go
This Latter feature, I would consider applying to even successful searches?
[Based on Feedback here]
Thoughts?
Re: pg17 issues with not-null contraints
On 2024-Apr-16, Justin Pryzby wrote: > Yes, this fixes the issue I reported. Excellent, thanks for confirming. > BTW, that seems to be the same issue Andrew reported in January. > https://www.postgresql.org/message-id/CAJnzarwkfRu76_yi3dqVF_WL-MpvT54zMwAxFwJceXdHB76bOA%40mail.gmail.com That's really good news -- I was worried it would require much more invasive changes. I tested his case and noticed two additional issues, first that we fail to acquire locks down the hierarchy, so recursing down like ATPrepAddPrimaryKey does fails to pin down the children properly; and second, that the constraint left behind by restoring the dump preserves the "throaway" name. I made pg_dump use a different name when the table has a parent, just in case we end up not dropping the constraint. I'm going to push this early tomorrow. CI run: https://cirrus-ci.com/build/5754149453692928 -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ >From e1b48c62ab46f6aaed366daacdd0560374b1cf07 Mon Sep 17 00:00:00 2001 From: Alvaro Herrera Date: Wed, 17 Apr 2024 19:23:04 +0200 Subject: [PATCH v2] Fix restore of not-null constraints with inheritance In tables with primary keys, pg_dump creates tables with primary keys by initially dumping them with throw-away not-null constraints (marked "no inherit" so that they don't create problems elsewhere), to later drop them once the primary key is restored. Because of a unrelated consideration, on tables with children we add not-null constraints to all columns of the primary key when it is created. If both a table and its child have primary keys, and pg_dump happens to emit the child table first (and its throw-away not-null) and later its parent table, the creation of the parent's PK will fail because the throw-away not-null constraint collides with the permanent not-null constraint that the PK wants to add, so the dump fails to restore. We can work around this problem by letting the primary key "take over" the child's not-null. This requires no changes to pg_dump, just two changes to ALTER TABLE: first, the ability to convert a no-inherit not-null constraint into a regular inheritable one (including recursing down to children, if there are any); second, the ability to "drop" a constraint that is defined both directly in the table and inherited from a parent (which simply means to mark it as no longer having a local definition). Secondarily, change ATPrepAddPrimaryKey() to acquire locks all the way down the inheritance hierarchy, in case we need to recurse when propagating constraints. In addition, change pg_dump to avoid giving a constraint a throwaway name for tables that are inheritance children. This is out of fear that the column might require the above shenanigans and the constraint ends up persisting later. We don't want such constraints being created with unsightly names. These two changes allow pg_dump to reproduce more cases involving inheritance from versions 16 and older. Reported-by: Andrew Bille Reported-by: Justin Pryzby Discussion: https://postgr.es/m/cajnzarwkfru76_yi3dqvf_wl-mpvt54zmwaxfwjcexdhb76...@mail.gmail.com Discussion: https://postgr.es/m/Zh0aAH7tbZb-9HbC@pryzbyj2023 --- src/backend/catalog/heap.c| 36 +++-- src/backend/catalog/pg_constraint.c | 43 ++- src/backend/commands/tablecmds.c | 64 --- src/bin/pg_dump/pg_dump.c | 17 +- src/include/catalog/pg_constraint.h | 2 +- src/test/regress/expected/constraints.out | 56 src/test/regress/sql/constraints.sql | 22 7 files changed, 213 insertions(+), 27 deletions(-) diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c index cc31909012..f0278b9c01 100644 --- a/src/backend/catalog/heap.c +++ b/src/backend/catalog/heap.c @@ -2539,6 +2539,7 @@ AddRelationNewConstraints(Relation rel, CookedConstraint *nncooked; AttrNumber colnum; char *nnname; + int existing; /* Determine which column to modify */ colnum = get_attnum(RelationGetRelid(rel), strVal(linitial(cdef->keys))); @@ -2547,12 +2548,39 @@ AddRelationNewConstraints(Relation rel, strVal(linitial(cdef->keys)), RelationGetRelid(rel)); /* - * If the column already has a not-null constraint, we need only - * update its catalog status and we're done. + * If the column already has an inheritable not-null constraint, + * we need only adjust its inheritance status and we're done. If + * the constraint is there but marked NO INHERIT, then it is + * updated in the same way, but we also recurse to the children + * (if any) to add the constraint there as well. */ - if (AdjustNotNullInheritance1(RelationGetRelid(rel), colnum, - cdef->inhcount, cdef->is_no_inherit)) + existing = AdjustNotNullInheritance1(RelationGetRelid(rel), colnum, + cdef->inhcount, cdef->is_no_inherit); + if (existing == 1)
Re: Solaris tar issues, or other reason why margay fails 010_pg_basebackup?
On Thu, Apr 18, 2024 at 1:40 AM Marcel Hofstetter wrote: > Using gnu tar helps to make pg_basebackup work. Thanks! I guess that'll remain a mystery. > It fails now at a later step. Oh, this rings a bell: [14:54:58] t/010_tab_completion.pl .. Dubious, test returned 29 (wstat 7424, 0x1d00) We had another thread[1] where we figured out that Solaris's termios defaults include TABDLY=TAB3, meaning "expand tabs to spaces on output", and that was upsetting our tab-completion test. Other Unixes used to vary on this point too, but they all converged on not doing that, except Solaris, apparently. Perhaps IPC::Run could fix that by calling ->set_raw() on the pseudo-terminal, but I'm not very sure about that. This test suite is passing on pollock because it doesn't have IO::Pty installed. Could you try uninstalling that perl package for now, so we can see what breaks next? [06:34:40] t/010_tab_completion.pl .. skipped: IO::Pty is needed to run this test [1] https://www.postgresql.org/message-id/flat/MEYP282MB1669E2E11495A2DEAECE8736B6A7A%40MEYP282MB1669.AUSP282.PROD.OUTLOOK.COM
Re: Stack overflow issue
Hi,
On 2024-04-17 14:39:14 +0300, Alexander Korotkov wrote:
> On Wed, Apr 17, 2024 at 2:37 PM Alexander Korotkov
> wrote:
> > I've invested more time into polishing this. I'm intended to push
> > this. Could you, please, take a look before?
>
> Just after sending this I spotted a typo s/untill/until/. The updated
> version is attached.
Nice, I see you moved the code back to "where it was", the diff to 16 got
smaller this way.
> + /*
> + * Repeatedly call CommitTransactionCommandInternal() until all the work
> + * is done.
> + */
> + while (!CommitTransactionCommandInternal());
Personally I'd use
{
}
instead of just ; here. The above scans weirdly for me. But it's also not
important.
Greetings,
Andres Freund
Re: documentation structure
Hi, On 2024-04-17 12:07:24 +0100, Dagfinn Ilmari Mannsåker wrote: > Andres Freund writes: > > I think the manual work for writing signatures in sgml is not insignificant, > > nor is the volume of sgml for them. Manually maintaining the signatures > > makes > > it impractical to significantly improve the presentation - which I don't > > think > > is all that great today. > > And it's very inconsistent. For example, some functions use > tags for optional parameters, others use square brackets, and some use > VARIADIC to indicate variadic parameters, others use > ellipses (sometimes in tags or brackets). That seems almost inevitably the outcome of many people having to manually infer the recommended semantics, for writing something boring but nontrivial, from a 30k line file. > > And the lack of argument names in the pg_proc entries is occasionally fairly > > annoying, because a \df+ doesn't provide enough information to use > > functions. > > I was also annoyed by this the other day (specifically wrt. the boolean > arguments to pg_ls_dir), My bane is regexp_match et al, I have given up on remembering the argument order. > and started whipping up a Perl script to parse func.sgml and generate > missing proargnames values for pg_proc.dat, which is how I discovered the > above. Nice. > The script currently has a pile of hacky regexes to cope with that, > so I'd be happy to submit a doc patch to turn it into actual markup to get > rid of that, if people think that's a worhtwhile use of time and won't clash > with any other plans for the documentation. I guess it's a bit hard to say without knowing how voluminious the changes would be. If we end up rewriting the whole file the tradeoff is less clear than if it's a dozen inconsistent entries. > > It'd also be quite useful if clients could render more of the documentation > > for functions. People are used to language servers providing full > > documentation for functions etc... > > A more user-friendly version of \df+ (maybe spelled \hf, for symmetry > with \h for commands?) would certainly be nice. Indeed. Greetings, Andres Freund
Re: documentation structure
Hi, On 2024-04-17 02:46:53 -0400, Corey Huinker wrote: > > > This sounds to me like it would be a painful exercise with not a > > > lot of benefit in the end. > > > > Maybe we could _verify_ the contents of func.sgml against pg_proc. > > > > All of the functions redefined in catalog/system_functions.sql complicate > using pg_proc.dat as a doc generator or source of validation. We'd probably > do better to validate against a live instance, and even then the benefit > wouldn't be great. There are 80 'CREATE OR REPLACE's in system_functions.sql, 1016 occurrences of func_table_entry in funcs.sgml and 3.3k functions in pg_proc. I'm not saying that differences due to system_functions.sql wouldn't be annoying to deal with, but it'd also be far from the end of the world. Greetings, Andres Freund
Re: documentation structure
> > And it's very inconsistent. For example, some functions use > tags for optional parameters, others use square brackets, and some use > VARIADIC to indicate variadic parameters, others use > ellipses (sometimes in tags or brackets). Having just written a couple of those functions, I wasn't able to find any guidance on how to document them with regards to vs [], etc. Having such a thing would be helpful. While we're throwing out ideas, does it make sense to have function parameters and return values be things that can accept COMMENTs? Like so: COMMENT ON FUNCTION function_name [ ( [ [ argmode ] [ argname ] argtype [, ...] ] ) ] ARGUMENT argname IS ''; COMMENT ON FUNCTION function_name [ ( [ [ argmode ] [ argname ] argtype [, ...] ] ) ] RETURN VALUE IS ''; I don't think this is a great idea, but if we're going to auto-generate documentation then we've got to store the metadata somewhere, and pg_proc.dat is already lacking relevant details.
Re: Statistics Import and Export
On Thu, Apr 11, 2024 at 03:54:07PM -0400, Corey Huinker wrote: > At the request of a few people, attached is an attempt to move stats to > DATA/POST-DATA, and the TAP test failure that results from that. > > The relevant errors are confusing, in that they all concern GRANT/REVOKE, > and the fact that I made no changes to the TAP test itself. > > $ grep 'not ok' build/meson-logs/testlog.txt > not ok 9347 - section_data: should not dump GRANT INSERT(col1) ON TABLE > test_second_table It looks like the problem is that the ACLs are getting dumped in the data section when we are also dumping stats. I'm able to get the tests to pass by moving the call to dumpRelationStats() that's in dumpTableSchema() to dumpTableData(). I'm not entirely sure why that fixes it yet, but if we're treating stats as data, then it intuitively makes sense for us to dump it in dumpTableData(). However, that seems to prevent the stats from getting exported in the --schema-only/--binary-upgrade scenario, which presents a problem for pg_upgrade. ISTM we'll need some extra hacks to get this to work as desired. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: Parallel CREATE INDEX for BRIN indexes
On 4/15/24 20:35, Tomas Vondra wrote: > ... > > Attached is the cleanup I thought about doing earlier in this patch [1] > to make the code more like btree. The diff might make it seem like a big > change, but it really just moves the merge code into a separate function > and makes it use using the conditional variable. I still believe the old > code is correct, but this seems like an improvement so plan to push this > soon and resolve the open item. > I've now pushed this cleanup patch, after rewording the commit message a little bit, etc. I believe this resolves the open item tracking this, so I've moved it to the "resolved" part. regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: ecpg_config.h symbol missing with meson
Peter Eisentraut writes: > I checked the generated ecpg_config.h with make and meson, and the meson > one is missing > #define HAVE_LONG_LONG_INT 1 > This is obviously quite uninteresting, since that is required by C99. > But it would be more satisfactory if we didn't have discrepancies like > that. Note that we also kept ENABLE_THREAD_SAFETY in ecpg_config.h for > compatibility. > ... > Alternatively, we could remove the symbol from the make side. Think I'd vote for removing it, since we use it nowhere. The ENABLE_THREAD_SAFETY precedent feels a little bit different, since there's not the C99-requires-the-feature angle. regards, tom lane
Re: [PATCH] Improve amcheck to also check UNIQUE constraint in btree index.
I did notice (I meant to point out) that I have concerns about this > part of the new uniqueness check code: > " > if (P_IGNORE(topaque) || !P_ISLEAF(topaque)) > break; > " My concern here is with the !P_ISLEAF(topaque) test -- it shouldn't be > required I agree. But I didn't see the need to check uniqueness constraints violations in internal pages. Furthermore, it doesn't mean only a violation of constraint, but a major index corruption. I agree that checking and reporting this type of corruption separately is a possible thing. Separately, I dislike the way the target block changes within > bt_target_page_check(). The general idea behind verify_nbtree.c's > target block is that every block becomes the target exactly once, in a > clearly defined place. All corruption (in the index structure itself) > is formally considered to be a problem with that particular target > block. I want to be able to clearly distinguish between the target and > target's right sibling here, to explain my concerns, but they're kinda > both the target, so that's a lot harder than it should be. (Admittedly > directly blaming the target block has always been a little bit > arbitrary, at least in certain cases, but even there it provides > structure that makes things much easier to describe unambiguously.) > The possible way to load the target block only once is to get rid of the cross-page uniqueness violation check. I introduced it to catch more possible cases of uniqueness violations. Though they are expected to be extremely rare, and anyway the algorithm doesn't get any warranty, just does its best to catch what is possible. I don't object to this change. Regards, Pavel.
Re: pg_combinebackup does not detect missing files
On Tue, Apr 16, 2024 at 7:25 PM David Steele wrote: > Except that we are running pg_combinebackup on the incremental, which > the user might reasonably expect to check backup integrity. It actually > does a bunch of integrity checks -- but not this one. I think it's a bad idea to duplicate all of the checks from pg_verifybackup into pg_combinebackup. I did consider this exact issue during development. These are separate tools with separate purposes. I think that what a user should expect is that each tool has some job and tries to do that job well, while leaving other jobs to other tools. And I think if you think about it that way, it explains a lot about which checks pg_combinebackup does and which checks it does not do. pg_combinebackup tries to check that it is valid to combine backup A with backup B (and maybe C, D, E ...), and it checks a lot of stuff to try to make sure that such an operation appears to be sensible. Those are checks that pg_verifybackup cannot do, because pg_verifybackup only looks at one backup in isolation. If pg_combinebackup does not do those checks, nobody does. Aside from that, it will also report errors that it can't avoid noticing, even if those are things that pg_verifybackup would also have found, such as, say, the control file not existing. But it will not go out of its way to perform checks that are unrelated to its documented purpose. And I don't think it should, especially if we have another tool that already does that. > > I'm not averse to having some kind of statement in the documentation > > along the lines of "Note that pg_combinebackup does not attempt to > > verify that the individual backups are intact; for that, use > > pg_verifybackup." > > I think we should do this at a minimum. Here is a patch to do that. > Especially given how pg_combinebackup works, backups are going to > undergo a lot of user manipulation (pushing to and pull from storage, > decompressing, untaring, etc.) and I think that means we should take > extra care. We are in agreement on that point, if nothing else. I am terrified of users having problems with pg_combinebackup and me not being able to tell whether those problems are due to user error, Robert error, or something else. I put a lot of effort into detecting dumb things that I thought a user might do, and a lot of effort into debugging output so that when things do go wrong anyway, we have a reasonable chance of figuring out exactly where they went wrong. We do seem to have a philosophical difference about what the scope of those checks ought to be, and I don't really expect what I wrote above to convince you that my position is correct, but perhaps it will convince you that I have a thoughtful position, as opposed to just having done stuff at random. -- Robert Haas EDB: http://www.enterprisedb.com v1-0001-docs-Mention-that-pg_combinebackup-does-not-verif.patch Description: Binary data
Re: Speed up clean meson builds by ~25%
On Wed, 17 Apr 2024 at 16:00, Tom Lane wrote: > Break gram.y (say, misspell some token in a production) and > see what happens. The behavior's likely to be timing sensitive > though. Thanks for clarifying. It took me a little while to break gram.y in such a way that I was able to consistently reproduce, but I managed in the end using the attached small diff. And then running ninja in non-parallel mode: ninja -C build all -j1 As I expected this problem was indeed fairly easy to address by still building "backend/parser" before "interfaces". See attached patch. v1-0001-Speed-up-clean-parallel-meson-builds-a-lot.patch Description: Binary data trigger-confusing-build-error.diff Description: Binary data
ecpg_config.h symbol missing with meson
I checked the generated ecpg_config.h with make and meson, and the meson
one is missing
#define HAVE_LONG_LONG_INT 1
This is obviously quite uninteresting, since that is required by C99.
But it would be more satisfactory if we didn't have discrepancies like
that. Note that we also kept ENABLE_THREAD_SAFETY in ecpg_config.h for
compatibility.
Fixing this on the meson side would be like
diff --git a/src/interfaces/ecpg/include/meson.build
b/src/interfaces/ecpg/include/meson.build
index 31610fef589..b85486acbea 100644
--- a/src/interfaces/ecpg/include/meson.build
+++ b/src/interfaces/ecpg/include/meson.build
@@ -12,6 +12,7 @@ ecpg_conf_keys = [
ecpg_conf_data = configuration_data()
ecpg_conf_data.set('ENABLE_THREAD_SAFETY', 1)
+ecpg_conf_data.set('HAVE_LONG_LONG_INT', 1)
foreach key : ecpg_conf_keys
if cdata.has(key)
Alternatively, we could remove the symbol from the make side.
Re: Parallel CREATE INDEX for BRIN indexes
On 4/16/24 22:33, Tomas Vondra wrote: > On 4/15/24 20:35, Tomas Vondra wrote: >> On 4/15/24 10:18, Tomas Vondra wrote: > > ... > > That is, no parallel index builds on temporary tables. Which means the > test is not actually testing anything :-( Much more stable, but not very > useful for finding issues. > > I think the best way to stabilize the test is to just not delete the > rows. That means we won't have any "empty" ranges (runs of pages without > any tuples). > I just pushed a revert and a patch to stabilize the test in a different way - Matthias mentioned to me off-list that DELETE is not the only way to generate empty ranges in a BRIN index, because partial indexes have the same effect. After playing with that a bit, that seems to work fine (works with parallel builds, not affected by cleanup), so done that way. regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: pg_combinebackup fails on file named INCREMENTAL.*
On Tue, Apr 16, 2024 at 9:25 AM Robert Haas wrote: > On Mon, Apr 15, 2024 at 10:12 PM David Steele wrote: > > Anyway, I think it should be fixed or documented as a caveat since it > > causes a hard failure on restore. > > Alright, I'll look into this. Here's a patch. -- Robert Haas EDB: http://www.enterprisedb.com v1-0001-Restrict-where-INCREMENTAL.-NAME-files-are-recogn.patch Description: Binary data
Re: Speed up clean meson builds by ~25%
Jelte Fennema-Nio writes: > How can I test if this actually happens? Because it sounds like if > that indeed happens it should be fixable fairly easily. Break gram.y (say, misspell some token in a production) and see what happens. The behavior's likely to be timing sensitive though. regards, tom lane
Re: pgsql: meson: Add initial version of meson based build system
Re: Andres Freund > https://git.postgresql.org/pg/commitdiff/e6927270cd18d535b77cbe79c55c6584351524be This commit broke VPATH builds when the original source directory contains symlinks. The $PWD is /home/myon/postgresql/pg/master, but the actual directory is /home/myon/projects/postgresql/pg/postgresql. When I mkdir build; cd build && ../configure there, I get a build directory missing a lot of files/directories: $ ls build/ config.log config.status* GNUmakefile meson.build src/ $ ls build/src/ backend/ include/ interfaces/ Makefile.global Makefile.port@ $ ls build/src/backend/ port/ Given there are no other changes I think this bit is at fault: > Modified Files > -- > configure.ac |6 + +# Ensure that any meson build directories would reconfigure and see that +# there's a conflicting in-tree build and can error out. +if test "$vpath_build"="no"; then + touch meson.build +fi Christoph
Re: Solaris tar issues, or other reason why margay fails 010_pg_basebackup?
Hi Thomas Using gnu tar helps to make pg_basebackup work. It fails now at a later step. Best regards, Marcel Am 17.04.2024 um 10:52 schrieb Thomas Munro: On Wed, Apr 17, 2024 at 7:17 PM Marcel Hofstetter wrote: Is there a way to configure which tar to use? gnu tar would be available. -bash-5.1$ ls -l /usr/gnu/bin/tar -r-xr-xr-x 1 root bin 1226248 Jul 1 2022 /usr/gnu/bin/tar Cool. I guess you could fix the test either by setting TAR=/usr/gnu/bin/tar or PATH=/usr/gnu/bin:$PATH. If we want to understand *why* it doesn't work, someone would need to dig into that. It's possible that PostgreSQL is using some GNU extension (if so, apparently the BSDs' tar is OK with it too, and I guess AIX's and HP-UX's was too in the recent times before we dropped those OSes). I vaguely recall (maybe 20 years ago, time flies) that Solaris tar wasn't able to extract some tarballs but I can't remember why... I'm also happy to leave it at "Sun's tar doesn't work for us, we don't know why" if you are.
Re: some LLVM function checks missing in meson
On 13.04.24 10:25, Heikki Linnakangas wrote: Something like the below would appear to fix that: diff --git a/meson.build b/meson.build index 43fad5323c0..cdfd31377d1 100644 --- a/meson.build +++ b/meson.build @@ -2301,6 +2301,14 @@ decl_checks += [ ['pwritev', 'sys/uio.h'], ] +# Check presence of some optional LLVM functions. +if llvm.found() + decl_checks += [ + ['LLVMCreateGDBRegistrationListener', 'llvm-c/ExecutionEngine.h'], + ['LLVMCreatePerfJITEventListener', 'llvm-c/ExecutionEngine.h'], + ] +endif + foreach c : decl_checks func = c.get(0) header = c.get(1) I don't know what these functions do, but the symbols are used in the source code. Thoughts? +1. I also don't know what they do, but clearly the configure and meson checks should be in sync. Committed that, too.
Re: CREATE TABLE creates a composite type corresponding to the table row, which is and is not there
On 29.03.24 02:42, David G. Johnston wrote: We do use the term "stand-alone composite" in create type so I'm inclined to use it instead of "composite created with CREATE TYPE"; especially in the error messages; I'm a bit more willing to add the cross-reference to create type in the user docs. I'm not sure this would have helped. If you see this in the error message, then there is no additional guidance what a "stand-alone composite type" and a not-"stand-alone composite type" are. Maybe it's possible to catch the forbidden cases more explicitly and come up with more helpful error messages along the lines of "cannot create a typed table based on the row type of another table".
Re: Speed up clean meson builds by ~25%
On Wed, 17 Apr 2024 at 13:41, Peter Eisentraut wrote: > > On 10.04.24 17:33, Tom Lane wrote: > > The immediate question then is do we want to take Jelte's patch > > as a way to ameliorate the pain meanwhile. I'm kind of down on > > it, because AFAICS what would happen if you break the core > > grammar is that (most likely) the failure would be reported > > against ecpg first. That seems pretty confusing. > > Yeah that would be confusing. How can I test if this actually happens? Because it sounds like if that indeed happens it should be fixable fairly easily.
Re: Building with meson on NixOS/nixpkgs
Peter Eisentraut:
On 29.03.24 19:47, walt...@technowledgy.de wrote:
> - uuid = dependency('ossp-uuid', required: true)
> + # upstream is called "uuid", but many distros change this to
"ossp-uuid"
> + uuid = dependency('ossp-uuid', 'uuid', required: true)
How would this behave if you have only uuid.pc from e2fsprogs installed
but choose -Duuid=ossp? Then it would pick up uuid.pc here, but fail to
compile later?
It would still fail the meson setup step, because for e2fs we have:
uuidfunc = 'uuid_generate'
uuidheader = 'uuid/uuid.h'
while for ossp we have:
uuidfunc = 'uuid_export'
uuidheader = 'uuid.h'
and later we do:
if not cc.has_header_symbol(uuidheader, uuidfunc, args: test_c_args,
dependencies: uuid)
error('uuid library @0@ missing required function
@1@'.format(uuidopt, uuidfunc))
endif
Best,
Wolfgang
Re: Building with meson on NixOS/nixpkgs
On 29.03.24 19:47, walt...@technowledgy.de wrote:
> -uuid = dependency('ossp-uuid', required: true)
> +# upstream is called "uuid", but many distros change this to
"ossp-uuid"
> +uuid = dependency('ossp-uuid', 'uuid', required: true)
How would this behave if you have only uuid.pc from e2fsprogs installed
but choose -Duuid=ossp? Then it would pick up uuid.pc here, but fail to
compile later?
Re: Speed up clean meson builds by ~25%
On 10.04.24 17:33, Tom Lane wrote: The immediate question then is do we want to take Jelte's patch as a way to ameliorate the pain meanwhile. I'm kind of down on it, because AFAICS what would happen if you break the core grammar is that (most likely) the failure would be reported against ecpg first. That seems pretty confusing. Yeah that would be confusing. I suppose we could just take the part of the patch that moves up preproc among the subdirectories of ecpg, but I don't know if that by itself would really buy anything.
Re: Stack overflow issue
On Wed, Apr 17, 2024 at 2:37 PM Alexander Korotkov wrote: > On Tue, Apr 16, 2024 at 7:42 PM Alexander Korotkov > wrote: > > On Tue, Apr 16, 2024 at 6:35 PM Andres Freund wrote: > > > On 2024-04-16 15:45:42 +0300, Alexander Korotkov wrote: > > > > On Tue, Apr 16, 2024 at 1:48 AM Andres Freund > > > > wrote: > > > > > On 2024-03-06 14:17:23 +0200, Alexander Korotkov wrote: > > > > > > 0001 Turn tail recursion into iteration in > > > > > > CommitTransactionCommand() > > > > > > I did minor revision of comments and code blocks order to improve > > > > > > the > > > > > > readability. > > > > > > > > > > After sending > > > > > https://www.postgresql.org/message-id/20240414223305.m3i5eju6zylabvln%40awork3.anarazel.de > > > > > I looked some more at important areas where changes didn't have code > > > > > coverage. One thing I noticed was that the "non-internal" part of > > > > > AbortCurrentTransaction() is uncovered: > > > > > https://anarazel.de/postgres/cov/16-vs-HEAD-2024-04-14/src/backend/access/transam/xact.c.gcov.html#L3403 > > > > > > > > > > Which made me try to understand fefd9a3fed2. I'm a bit confused > > > > > about why > > > > > some parts are handled in > > > > > CommitCurrentTransaction()/AbortCurrentTransaction() > > > > > and others are in the *Internal functions. > > > > > > > > > > I understand that fefd9a3fed2 needed to remove the recursion in > > > > > CommitTransactionCommand()/AbortCurrentTransaction(). But I don't > > > > > understand > > > > > why that means having some code in in the non-internal and some in the > > > > > internal functions? Wouldn't it be easier to just have all the state > > > > > handling > > > > > code in the Internal() function and just break after the > > > > > CleanupSubTransaction() calls? > > > > > > > > I'm not sure I correctly get what you mean. Do you think the attached > > > > patch matches the direction you're pointing? The patch itself is not > > > > final, it requires cleanup and comments revision, just to check the > > > > direction. > > > > > > Something like that, yea. The split does seem less confusing that way to > > > me, > > > but also not 100% certain. > > > > Thank you for your feedback. I'm going to go ahead and polish this patch. > > I've invested more time into polishing this. I'm intended to push > this. Could you, please, take a look before? Just after sending this I spotted a typo s/untill/until/. The updated version is attached. -- Regards, Alexander Korotkov v3-0001-Refactoring-for-CommitTransactionCommand-AbortCur.patch Description: Binary data
Re: Stack overflow issue
On Tue, Apr 16, 2024 at 7:42 PM Alexander Korotkov wrote: > > On Tue, Apr 16, 2024 at 6:35 PM Andres Freund wrote: > > On 2024-04-16 15:45:42 +0300, Alexander Korotkov wrote: > > > On Tue, Apr 16, 2024 at 1:48 AM Andres Freund wrote: > > > > On 2024-03-06 14:17:23 +0200, Alexander Korotkov wrote: > > > > > 0001 Turn tail recursion into iteration in CommitTransactionCommand() > > > > > I did minor revision of comments and code blocks order to improve the > > > > > readability. > > > > > > > > After sending > > > > https://www.postgresql.org/message-id/20240414223305.m3i5eju6zylabvln%40awork3.anarazel.de > > > > I looked some more at important areas where changes didn't have code > > > > coverage. One thing I noticed was that the "non-internal" part of > > > > AbortCurrentTransaction() is uncovered: > > > > https://anarazel.de/postgres/cov/16-vs-HEAD-2024-04-14/src/backend/access/transam/xact.c.gcov.html#L3403 > > > > > > > > Which made me try to understand fefd9a3fed2. I'm a bit confused about > > > > why > > > > some parts are handled in > > > > CommitCurrentTransaction()/AbortCurrentTransaction() > > > > and others are in the *Internal functions. > > > > > > > > I understand that fefd9a3fed2 needed to remove the recursion in > > > > CommitTransactionCommand()/AbortCurrentTransaction(). But I don't > > > > understand > > > > why that means having some code in in the non-internal and some in the > > > > internal functions? Wouldn't it be easier to just have all the state > > > > handling > > > > code in the Internal() function and just break after the > > > > CleanupSubTransaction() calls? > > > > > > I'm not sure I correctly get what you mean. Do you think the attached > > > patch matches the direction you're pointing? The patch itself is not > > > final, it requires cleanup and comments revision, just to check the > > > direction. > > > > Something like that, yea. The split does seem less confusing that way to me, > > but also not 100% certain. > > Thank you for your feedback. I'm going to go ahead and polish this patch. I've invested more time into polishing this. I'm intended to push this. Could you, please, take a look before? -- Regards, Alexander Korotkov v2-0001-Refactoring-for-CommitTransactionCommand-AbortCur.patch Description: Binary data
Re: documentation structure
Andres Freund writes:
> Definitely shouldn't be the same in all cases, but I think there's a decent
> number of cases where they can be the same. The differences between the two is
> often minimal today.
>
> Entirely randomly chosen example:
>
> { oid => '2825',
> descr => 'slope of the least-squares-fit linear equation determined by the
> (X, Y) pairs',
> proname => 'regr_slope', prokind => 'a', proisstrict => 'f',
> prorettype => 'float8', proargtypes => 'float8 float8',
> prosrc => 'aggregate_dummy' },
>
> and
>
>
>
>
> regression slope
>
>
> regr_slope
>
> regr_slope ( Y
> double precision, X double
> precision )
> double precision
>
>
> Computes the slope of the least-squares-fit linear equation determined
> by the (X, Y)
> pairs.
>
>Yes
>
>
>
> The description is quite similar, the pg_proc entry lacks argument names.
>
>
>> This sounds to me like it would be a painful exercise with not a
>> lot of benefit in the end.
>
> I think the manual work for writing signatures in sgml is not insignificant,
> nor is the volume of sgml for them. Manually maintaining the signatures makes
> it impractical to significantly improve the presentation - which I don't think
> is all that great today.
And it's very inconsistent. For example, some functions use
tags for optional parameters, others use square brackets, and some use
VARIADIC to indicate variadic parameters, others use
ellipses (sometimes in tags or brackets).
> And the lack of argument names in the pg_proc entries is occasionally fairly
> annoying, because a \df+ doesn't provide enough information to use functions.
I was also annoyed by this the other day (specifically wrt. the boolean
arguments to pg_ls_dir), and started whipping up a Perl script to parse
func.sgml and generate missing proargnames values for pg_proc.dat, which
is how I discovered the above. The script currently has a pile of hacky
regexes to cope with that, so I'd be happy to submit a doc patch to turn
it into actual markup to get rid of that, if people think that's a
worhtwhile use of time and won't clash with any other plans for the
documentation.
> It'd also be quite useful if clients could render more of the documentation
> for functions. People are used to language servers providing full
> documentation for functions etc...
A more user-friendly version of \df+ (maybe spelled \hf, for symmetry
with \h for commands?) would certainly be nice.
> Greetings,
>
> Andres Freund
- ilmari
Re: ALTER TABLE SET ACCESS METHOD on partitioned tables
BTW if nothing else, this thread led me to discover a 18-month-old typo in the Spanish translation of pg_dump: -msgstr " --no-tablespaces no volcar métodos de acceso de tablas\n" +msgstr " --no-table-access-method no volcar métodos de acceso de tablas\n" Oops. -- Álvaro HerreraBreisgau, Deutschland — https://www.EnterpriseDB.com/ "Los dioses no protegen a los insensatos. Éstos reciben protección de otros insensatos mejor dotados" (Luis Wu, Mundo Anillo)
Re: SQL function which allows to distinguish a server being in point in time recovery mode and an ordinary replica
On 2024-Apr-16, Michael Paquier wrote: there are already too many of them. Perhaps we should begin tracking all that as a set of bitmasks, then plug in the tracked state in shmem for consumption in some SQL function. Yes, it sounds reasonable. Let me implement some initial draft and come back with it after a while. Respectfully, Mikhail Litsarev Postgres Professional: https://postgrespro.com
Re: some LLVM function checks missing in meson
On 13.04.24 10:25, Heikki Linnakangas wrote:
There's also this in llvmjit.c:
if (llvm_opt3_orc)
{
#if defined(HAVE_DECL_LLVMORCREGISTERPERF) &&
HAVE_DECL_LLVMORCREGISTERPERF
if (jit_profiling_support)
LLVMOrcUnregisterPerf(llvm_opt3_orc);
#endif
LLVMOrcDisposeInstance(llvm_opt3_orc);
llvm_opt3_orc = NULL;
}
if (llvm_opt0_orc)
{
#if defined(HAVE_DECL_LLVMORCREGISTERPERF) &&
HAVE_DECL_LLVMORCREGISTERPERF
if (jit_profiling_support)
LLVMOrcUnregisterPerf(llvm_opt0_orc);
#endif
LLVMOrcDisposeInstance(llvm_opt0_orc);
llvm_opt0_orc = NULL;
}
}
The autoconf test that set HAVE_DECL_LLVMORCREGISTERPERF was removed in
commit e9a9843e13. I believe that's a leftover that should also have
been removed.
Right, that was clearly forgotten. I have removed the dead code.
Re: Solaris tar issues, or other reason why margay fails 010_pg_basebackup?
On Wed, Apr 17, 2024 at 7:17 PM Marcel Hofstetter wrote: > Is there a way to configure which tar to use? > > gnu tar would be available. > > -bash-5.1$ ls -l /usr/gnu/bin/tar > -r-xr-xr-x 1 root bin 1226248 Jul 1 2022 /usr/gnu/bin/tar Cool. I guess you could fix the test either by setting TAR=/usr/gnu/bin/tar or PATH=/usr/gnu/bin:$PATH. If we want to understand *why* it doesn't work, someone would need to dig into that. It's possible that PostgreSQL is using some GNU extension (if so, apparently the BSDs' tar is OK with it too, and I guess AIX's and HP-UX's was too in the recent times before we dropped those OSes). I vaguely recall (maybe 20 years ago, time flies) that Solaris tar wasn't able to extract some tarballs but I can't remember why... I'm also happy to leave it at "Sun's tar doesn't work for us, we don't know why" if you are.
Re: hash_destroy on the hash table allocated with TopMemoryContext
On Wed, Apr 17, 2024 at 1:04 PM Sharique Muhammed wrote: > Hi, > > I was looking for a pattern to destroy a hashtable (dynahash).allocated > in TopMemoryContext > I found one pattern : create_seq_hashtable uses TopMemoryContext > memory context to create hash table. It calls hash_destroy in > ResetSequenceCaches. hash_destroy will destroy the memory > context(TopMemoryContext). Is it the right way to use hash_destroy ? > The context used to pass hash_create() is used to create a child memory context. The hash table is allocated in the child memory context and it's that context which is destoryed by hash_destory(). Isn't it? > > I have allocated a hash table in TopMemoryContext context and I want > to destroy it. It seems to me that there is no function to destroy hash > table allocated in TopMemoryContext context. > > How did you create hash table in TopMemoryContext? -- Best Wishes, Ashutosh Bapat
Re: ALTER TABLE SET ACCESS METHOD on partitioned tables
On 2024-Apr-17, Michael Paquier wrote: > Yeah, that would be easy enough to track but I was wondering about > adding the relkind instead. Still, one thing that I found confusing > is the dump generated in this case, as it would mix the SET and the > ALTER TABLE commands so one reading the dumps may wonder why the SET > has no effect for a CREATE TABLE PARTITION OF without USING. Perhaps > that's fine and I just worry too much ;) Hmm, maybe we should do a RESET of default_table_access_method before printing the CREATE TABLE to avoid the confusion. > The extra ALTER commands need to be generated after the object > definitions, so we'd need a new subroutine similar to > _selectTableAccessMethod() like a _selectTableAccessMethodNoStorage(). > Or grouping both together is just simpler? I think there should be two routines, since _select* routines just print a SET command; maybe the new one would be _printAlterTableAM() or something like that. Having _select() print an ALTER TABLE command depending on relkind (or the boolean flag) would be confusing, I think. -- Álvaro HerreraBreisgau, Deutschland — https://www.EnterpriseDB.com/
Re: Typo about the SetDatatabaseHasLoginEventTriggers?
On Wed, 17 Apr 2024 at 14:10, Michael Paquier wrote: > On Tue, Apr 16, 2024 at 03:31:49PM +0900, Michael Paquier wrote: >> Indeed, thanks! Will fix and double-check the surroundings. > > And fixed this one. Thanks for the pushing! -- Regards, Japin Li
Re: ALTER TABLE SET ACCESS METHOD on partitioned tables
On Wed, Apr 17, 2024 at 09:50:02AM +0200, Alvaro Herrera wrote: > I think it's easy enough to add a "bool ispartitioned" to TableInfo and > use an ALTER TABLE or rely on the GUC depending on that -- seems easy > enough. Yeah, that would be easy enough to track but I was wondering about adding the relkind instead. Still, one thing that I found confusing is the dump generated in this case, as it would mix the SET and the ALTER TABLE commands so one reading the dumps may wonder why the SET has no effect for a CREATE TABLE PARTITION OF without USING. Perhaps that's fine and I just worry too much ;) The extra ALTER commands need to be generated after the object definitions, so we'd need a new subroutine similar to _selectTableAccessMethod() like a _selectTableAccessMethodNoStorage(). Or grouping both together is just simpler? -- Michael signature.asc Description: PGP signature
Re: ALTER TABLE SET ACCESS METHOD on partitioned tables
On 2024-Apr-17, Alvaro Herrera wrote: > Hmm, cannot we simply add a USING clause to the CREATE TABLE command for > partitioned tables? That would override the > default_table_access_method, so it should give the correct result, no? Ah, upthread you noted that pg_restore-time --no-table-access-method needs to be able to elide it, so a dump-time USING clause doesn't work. I think it's easy enough to add a "bool ispartitioned" to TableInfo and use an ALTER TABLE or rely on the GUC depending on that -- seems easy enough. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
Re: ALTER TABLE SET ACCESS METHOD on partitioned tables
On 2024-Apr-17, Michael Paquier wrote: > 2) We could limit these extra ALTER TABLE commands to be generated for > partitioned tables. This is kind of confusing as resulting dumps > would mix SET commands for default_table_access_method that would > affect tables with physical storage, while partitioned tables would > have their own extra ALTER TABLE commands. Hmm, cannot we simply add a USING clause to the CREATE TABLE command for partitioned tables? That would override the default_table_access_method, so it should give the correct result, no? -- Álvaro HerreraBreisgau, Deutschland — https://www.EnterpriseDB.com/
hash_destroy on the hash table allocated with TopMemoryContext
Hi, I was looking for a pattern to destroy a hashtable (dynahash).allocated in TopMemoryContext I found one pattern : create_seq_hashtable uses TopMemoryContext memory context to create hash table. It calls hash_destroy in ResetSequenceCaches. hash_destroy will destroy the memory context(TopMemoryContext). Is it the right way to use hash_destroy ? I have allocated a hash table in TopMemoryContext context and I want to destroy it. It seems to me that there is no function to destroy hash table allocated in TopMemoryContext context. -- Sharique
Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features)
On Wed, Apr 17, 2024 at 4:28 PM torikoshia wrote:
>
> On 2024-04-16 13:16, Masahiko Sawada wrote:
> > On Tue, Apr 2, 2024 at 7:34 PM torikoshia
> > wrote:
> >>
> >> On 2024-04-01 11:31, Masahiko Sawada wrote:
> >> > On Fri, Mar 29, 2024 at 11:54 AM torikoshia
> >> > wrote:
> >> >>
> >> >> On 2024-03-28 21:54, Masahiko Sawada wrote:
> >> >> > On Thu, Mar 28, 2024 at 9:38 PM torikoshia
> >> >> >
> >> >> > wrote:
> >> >> >>
> >> >> >> On 2024-03-28 10:20, Masahiko Sawada wrote:
> >> >> >> > Hi,
> >> >> >> >
> >> >> >> > On Thu, Jan 18, 2024 at 5:33 PM Masahiko Sawada
> >> >> >> >
> >> >> >> > wrote:
> >> >> >> >>
> >> >> >> >> On Thu, Jan 18, 2024 at 4:59 PM Alexander Korotkov
> >> >> >> >> wrote:
> >> >> >> >> >
> >> >> >> >> > On Thu, Jan 18, 2024 at 4:16 AM torikoshia
> >> >> >> >> > wrote:
> >> >> >> >> > > On 2024-01-18 10:10, jian he wrote:
> >> >> >> >> > > > On Thu, Jan 18, 2024 at 8:57 AM Masahiko Sawada
> >> >> >> >> > > >
> >> >> >> >> > > > wrote:
> >> >> >> >> > > >> On Thu, Jan 18, 2024 at 6:38 AM Tom Lane
> >> >> >> >> > > >> wrote:
> >> >> >> >> > > >> > Kyotaro-san's suggestion isn't bad, though I might
> >> >> >> >> > > >> > shorten it to
> >> >> >> >> > > >> > error_action {error|ignore|log} (or perhaps "stop"
> >> >> >> >> > > >> > instead of "error")?
> >> >> >> >> > > >> > You will need a separate parameter anyway to specify the
> >> >> >> >> > > >> > destination
> >> >> >> >> > > >> > of "log", unless "none" became an illegal table name
> >> >> >> >> > > >> > when I wasn't
> >> >> >> >> > > >> > looking. I don't buy that one parameter that has some
> >> >> >> >> > > >> > special values
> >> >> >> >> > > >> > while other values could be names will be a good design.
> >> >> >> >> > > >> > Moreover,
> >> >> >> >> > > >> > what if we want to support (say) log-to-file along with
> >> >> >> >> > > >> > log-to-table?
> >> >> >> >> > > >> > Trying to distinguish a file name from a table name
> >> >> >> >> > > >> > without any other
> >> >> >> >> > > >> > context seems impossible.
> >> >> >> >> > > >>
> >> >> >> >> > > >> I've been thinking we can add more values to this option
> >> >> >> >> > > >> to log errors
> >> >> >> >> > > >> not only to the server logs but also to the error table
> >> >> >> >> > > >> (not sure
> >> >> >> >> > > >> details but I imagined an error table is created for each
> >> >> >> >> > > >> table on
> >> >> >> >> > > >> error), without an additional option for the destination
> >> >> >> >> > > >> name. The
> >> >> >> >> > > >> values would be like error_action
> >> >> >> >> > > >> {error|ignore|save-logs|save-table}.
> >> >> >> >> > > >>
> >> >> >> >> > > >
> >> >> >> >> > > > another idea:
> >> >> >> >> > > > on_error {error|ignore|other_future_option}
> >> >> >> >> > > > if not specified then by default ERROR.
> >> >> >> >> > > > You can also specify ERROR or IGNORE for now.
> >> >> >> >> > > >
> >> >> >> >> > > > I agree, the parameter "error_action" is better than
> >> >> >> >> > > > "location".
> >> >> >> >> > >
> >> >> >> >> > > I'm not sure whether error_action or on_error is better, but
> >> >> >> >> > > either way
> >> >> >> >> > > "error_action error" and "on_error error" seems a bit odd to
> >> >> >> >> > > me.
> >> >> >> >> > > I feel "stop" is better for both cases as Tom suggested.
> >> >> >> >> >
> >> >> >> >> > OK. What about this?
> >> >> >> >> > on_error {stop|ignore|other_future_option}
> >> >> >> >> > where other_future_option might be compound like "file
> >> >> >> >> > 'copy.log'" or
> >> >> >> >> > "table 'copy_log'".
> >> >> >> >>
> >> >> >> >> +1
> >> >> >> >>
> >> >> >> >
> >> >> >> > I realized that ON_ERROR syntax synoposis in the documentation is
> >> >> >> > not
> >> >> >> > correct. The option doesn't require the value to be quoted and the
> >> >> >> > value can be omitted. The attached patch fixes it.
> >> >> >> >
> >> >> >> > Regards,
> >> >> >>
> >> >> >> Thanks!
> >> >> >>
> >> >> >> Attached patch fixes the doc, but I'm wondering perhaps it might be
> >> >> >> better to modify the codes to prohibit abbreviation of the value.
> >> >> >>
> >> >> >> When seeing the query which abbreviates ON_ERROR value, I feel it's
> >> >> >> not
> >> >> >> obvious what happens compared to other options which tolerates
> >> >> >> abbreviation of the value such as FREEZE or HEADER.
> >> >> >>
> >> >> >>COPY t1 FROM stdin WITH (ON_ERROR);
> >> >> >>
> >> >> >> What do you think?
> >> >> >
> >> >> > Indeed. Looking at options of other commands such as VACUUM and
> >> >> > EXPLAIN, I can see that we can omit a boolean value, but non-boolean
> >> >> > parameters require its value. The HEADER option is not a pure boolean
> >> >> > parameter but we can omit the value. It seems to be for backward
> >> >> > compatibility; it used to be a boolean parameter. I agree that the
> >> >> > above example would confuse users.
> >> >> >
> >> >> > Regards,
> >> >>
> >> >> Thanks for your comment!
> >> >>
> >> >> Attached a patch
Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features)
On 2024-04-16 13:16, Masahiko Sawada wrote:
On Tue, Apr 2, 2024 at 7:34 PM torikoshia
wrote:
On 2024-04-01 11:31, Masahiko Sawada wrote:
> On Fri, Mar 29, 2024 at 11:54 AM torikoshia
> wrote:
>>
>> On 2024-03-28 21:54, Masahiko Sawada wrote:
>> > On Thu, Mar 28, 2024 at 9:38 PM torikoshia
>> > wrote:
>> >>
>> >> On 2024-03-28 10:20, Masahiko Sawada wrote:
>> >> > Hi,
>> >> >
>> >> > On Thu, Jan 18, 2024 at 5:33 PM Masahiko Sawada
>> >> > wrote:
>> >> >>
>> >> >> On Thu, Jan 18, 2024 at 4:59 PM Alexander Korotkov
>> >> >> wrote:
>> >> >> >
>> >> >> > On Thu, Jan 18, 2024 at 4:16 AM torikoshia
wrote:
>> >> >> > > On 2024-01-18 10:10, jian he wrote:
>> >> >> > > > On Thu, Jan 18, 2024 at 8:57 AM Masahiko Sawada
>> >> >> > > > wrote:
>> >> >> > > >> On Thu, Jan 18, 2024 at 6:38 AM Tom Lane
wrote:
>> >> >> > > >> > Kyotaro-san's suggestion isn't bad, though I might shorten it
to
>> >> >> > > >> > error_action {error|ignore|log} (or perhaps "stop" instead of
"error")?
>> >> >> > > >> > You will need a separate parameter anyway to specify the
destination
>> >> >> > > >> > of "log", unless "none" became an illegal table name when I
wasn't
>> >> >> > > >> > looking. I don't buy that one parameter that has some
special values
>> >> >> > > >> > while other values could be names will be a good design.
Moreover,
>> >> >> > > >> > what if we want to support (say) log-to-file along with
log-to-table?
>> >> >> > > >> > Trying to distinguish a file name from a table name without
any other
>> >> >> > > >> > context seems impossible.
>> >> >> > > >>
>> >> >> > > >> I've been thinking we can add more values to this option to log
errors
>> >> >> > > >> not only to the server logs but also to the error table (not
sure
>> >> >> > > >> details but I imagined an error table is created for each table
on
>> >> >> > > >> error), without an additional option for the destination name.
The
>> >> >> > > >> values would be like error_action
{error|ignore|save-logs|save-table}.
>> >> >> > > >>
>> >> >> > > >
>> >> >> > > > another idea:
>> >> >> > > > on_error {error|ignore|other_future_option}
>> >> >> > > > if not specified then by default ERROR.
>> >> >> > > > You can also specify ERROR or IGNORE for now.
>> >> >> > > >
>> >> >> > > > I agree, the parameter "error_action" is better than "location".
>> >> >> > >
>> >> >> > > I'm not sure whether error_action or on_error is better, but
either way
>> >> >> > > "error_action error" and "on_error error" seems a bit odd to me.
>> >> >> > > I feel "stop" is better for both cases as Tom suggested.
>> >> >> >
>> >> >> > OK. What about this?
>> >> >> > on_error {stop|ignore|other_future_option}
>> >> >> > where other_future_option might be compound like "file 'copy.log'" or
>> >> >> > "table 'copy_log'".
>> >> >>
>> >> >> +1
>> >> >>
>> >> >
>> >> > I realized that ON_ERROR syntax synoposis in the documentation is not
>> >> > correct. The option doesn't require the value to be quoted and the
>> >> > value can be omitted. The attached patch fixes it.
>> >> >
>> >> > Regards,
>> >>
>> >> Thanks!
>> >>
>> >> Attached patch fixes the doc, but I'm wondering perhaps it might be
>> >> better to modify the codes to prohibit abbreviation of the value.
>> >>
>> >> When seeing the query which abbreviates ON_ERROR value, I feel it's
>> >> not
>> >> obvious what happens compared to other options which tolerates
>> >> abbreviation of the value such as FREEZE or HEADER.
>> >>
>> >>COPY t1 FROM stdin WITH (ON_ERROR);
>> >>
>> >> What do you think?
>> >
>> > Indeed. Looking at options of other commands such as VACUUM and
>> > EXPLAIN, I can see that we can omit a boolean value, but non-boolean
>> > parameters require its value. The HEADER option is not a pure boolean
>> > parameter but we can omit the value. It seems to be for backward
>> > compatibility; it used to be a boolean parameter. I agree that the
>> > above example would confuse users.
>> >
>> > Regards,
>>
>> Thanks for your comment!
>>
>> Attached a patch which modifies the code to prohibit omission of its
>> value.
>>
>> I was a little unsure about adding a regression test for this, but I
>> have not added it since other COPY option doesn't test the omission of
>> its value.
>
> Probably should we change the doc as well since ON_ERROR value doesn't
> necessarily need to be single-quoted?
Agreed.
Since it seems this issue is independent from the omission of ON_ERROR
option value, attached a separate patch.
Thank you for the patches! These patches look good to me. I'll push
them, barring any objections.
Regards,
Thanks for your review and apply!
--
Regards,
--
Atsushi Torikoshi
NTT DATA Group Corporation
Re: Solaris tar issues, or other reason why margay fails 010_pg_basebackup?
Hi Is there a way to configure which tar to use? gnu tar would be available. -bash-5.1$ ls -l /usr/gnu/bin/tar -r-xr-xr-x 1 root bin 1226248 Jul 1 2022 /usr/gnu/bin/tar Which tar file is used? I could try to untar manually to see what happens. Best regards, Marcel Am 17.04.2024 um 06:21 schrieb Thomas Munro: Hi, I noticed that margay (Solaris) has started running more of the tests lately, but is failing in pg_basebaseup/010_pg_basebackup. It runs successfully on wrasse (in older branches, Solaris 11.3 is desupported in 17/master), and also on pollock (illumos, forked from common ancestor Solaris 10 while it was open source). Hmm, wrasse is using "/opt/csw/bin/gtar xf ..." and pollock is using "/usr/gnu/bin/tar xf ...", while margay is using "/usr/bin/tar xf ...". The tar command is indicating success (it's run by system_or_bail and it's not bailing), but the replica doesn't want to come up: pg_ctl: directory "/home/marcel/build-farm-15/buildroot/HEAD/pgsql.build/src/bin/pg_basebackup/tmp_check/t_010_pg_basebackup_replica_data/pgdata" is not a database cluster directory" So one idea would be that our tar format is incompatible with Sun tar in some way that corrupts the output, or there is some still difference in the nesting of the directory structure it creates, or something like that. I wonder if this is already common knowledge in the repressed memories of this list, but I couldn't find anything specific. I'd be curious to know why exactly, if so (in terms of POSIX conformance etc, who is doing something wrong).
Re: allow changing autovacuum_max_workers without restarting
Agree +1,From a dba perspective, I would prefer that this parameter can be dynamically modified, rather than adding a new parameter,What is more difficult is how to smoothly reach the target value when the setting is considered to be too large and needs to be lowered. Regards On Tue, 16 Apr 2024 at 01:41, Imseih (AWS), Sami wrote: > > Another option could be to just remove the restart-only GUC and hard-code > > the upper limit of autovacuum_max_workers to 64 or 128 or something. > While > > that would simplify matters, I suspect it would be hard to choose an > > appropriate limit that won't quickly become outdated. > > Hardcoded values are usually hard to deal with because they are hidden > either > In code or in docs. > > > When I thought about this, I considered proposing to add a new GUC for > > "autovacuum_policy_workers". > > > autovacuum_max_workers would be the same as before, requiring a restart > > to change. The policy GUC would be the soft limit, changable at runtime > > I think autovacuum_max_workers should still be the GUC that controls > the number of concurrent autovacuums. This parameter is already well > established and changing the meaning now will be confusing. > > I suspect most users will be glad it's now dynamic, but will probably > be annoyed if it's no longer doing what it's supposed to. > > Regards, > > Sami > >
Re: documentation structure
> > > This sounds to me like it would be a painful exercise with not a > > lot of benefit in the end. > > Maybe we could _verify_ the contents of func.sgml against pg_proc. > All of the functions redefined in catalog/system_functions.sql complicate using pg_proc.dat as a doc generator or source of validation. We'd probably do better to validate against a live instance, and even then the benefit wouldn't be great.
Re: [PATCH] Improve amcheck to also check UNIQUE constraint in btree index.
On 24.10.23 22:13, Alexander Korotkov wrote: On Wed, Sep 28, 2022 at 11:44 AM Aleksander Alekseev wrote: I think, this patch was marked as "Waiting on Author", probably, by mistake. Since recent changes were done without any significant code changes and CF bot how happy again. I'm going to move it to RfC, could I? If not, please tell why. I restored the "Ready for Committer" state. I don't think it's a good practice to change the state every time the patch has a slight conflict or something. This is not helpful at all. Such things happen quite regularly and typically are fixed in a couple of days. This patch seems useful to me. I went through the thread, it seems that all the critics are addressed. I've rebased this patch. Also, I've run perltidy for tests, split long errmsg() into errmsg(), errdetail() and errhint(), and do other minor enchantments. I think this patch is ready to go. I'm going to push it if there are no objections. I just found the new pg_amcheck option --checkunique in PG17-to-be. Could we rename this to --check-unique? Seems friendlier. Maybe also rename the bt_index_check function argument to check_unique.
Re: Typo about the SetDatatabaseHasLoginEventTriggers?
On Tue, Apr 16, 2024 at 03:31:49PM +0900, Michael Paquier wrote: > Indeed, thanks! Will fix and double-check the surroundings. And fixed this one. -- Michael signature.asc Description: PGP signature
