Re: Introduce XID age and inactive timeout based replication slot invalidation
Hi, here are some review comments for v45-0001 (excluding the test code)
==
doc/src/sgml/config.sgml
1.
+Note that the inactive timeout invalidation mechanism is not
+applicable for slots on the standby server that are being synced
+from primary server (i.e., standby slots having
nit - /from primary server/from the primary server/
==
src/backend/replication/slot.c
2. ReplicationSlotAcquire
+ errmsg("can no longer get changes from replication slot \"%s\"",
+ NameStr(s->data.name)),
+ errdetail("This slot has been invalidated because it was inactive
for longer than the amount of time specified by \"%s\".",
+"replication_slot_inactive_timeout.")));
nit - "replication_slot_inactive_timeout." - should be no period
inside that GUC name literal
~~~
3. ReportSlotInvalidation
I didn't understand why there was a hint for:
"You might need to increase \"%s\".", "max_slot_wal_keep_size"
But you don't have an equivalent hint for timeout invalidation:
"You might need to increase \"%s\".", "replication_slot_inactive_timeout"
Why aren't these similar cases consistent?
~~~
4. RestoreSlotFromDisk
+ /* Use the same inactive_since time for all the slots. */
+ if (now == 0)
+ now = GetCurrentTimestamp();
+
Is the deferred assignment really necessary? Why not just
unconditionally assign the 'now' just before the for-loop? Or even at
the declaration? e.g. The 'replication_slot_inactive_timeout' is
measured in seconds so I don't think 'inactive_since' being wrong by a
millisecond here will make any difference.
==
src/include/replication/slot.h
5. ReplicationSlotSetInactiveSince
+/*
+ * Set slot's inactive_since property unless it was previously invalidated due
+ * to inactive timeout.
+ */
+static inline void
+ReplicationSlotSetInactiveSince(ReplicationSlot *s, TimestampTz *now,
+ bool acquire_lock)
+{
+ if (acquire_lock)
+ SpinLockAcquire(&s->mutex);
+
+ if (s->data.invalidated != RS_INVAL_INACTIVE_TIMEOUT)
+ s->inactive_since = *now;
+
+ if (acquire_lock)
+ SpinLockRelease(&s->mutex);
+}
Is the logic correct? What if the slot was already invalid due to some
reason other than RS_INVAL_INACTIVE_TIMEOUT? Is an Assert needed?
==
Kind Regards,
Peter Smith.
Fujitsu Australia
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 27b2285..97b4fb5 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -4582,7 +4582,7 @@ restore_command = 'copy "C:\\server\\archivedir\\%f"
"%p"' # Windows
Note that the inactive timeout invalidation mechanism is not
applicable for slots on the standby server that are being synced
-from primary server (i.e., standby slots having
+from the primary server (i.e., standby slots having
pg_replication_slots.synced
value true).
Synced slots are always considered to be inactive because they don't
diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index d92b92b..8cc67b4 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -640,7 +640,7 @@ retry:
errmsg("can no longer get changes from
replication slot \"%s\"",
NameStr(s->data.name)),
errdetail("This slot has been invalidated
because it was inactive for longer than the amount of time specified by
\"%s\".",
-
"replication_slot_inactive_timeout.")));
+
"replication_slot_inactive_timeout")));
}
/*
Re: Invalid Assert while validating REPLICA IDENTITY?
On Mon, Sep 9, 2024 at 11:44 AM Dilip Kumar wrote: > > On Fri, Sep 6, 2024 at 4:48 PM vignesh C wrote: > > > > On Mon, 2 Sept 2024 at 18:22, Dilip Kumar wrote: > > > > > > On Mon, Sep 2, 2024 at 3:32 PM Amit Kapila > > > wrote: > > > > > > > > On Mon, Sep 2, 2024 at 11:21 AM Dilip Kumar > > > > wrote: > > > > > > > > > > While working on some other code I noticed that in > > > > > FindReplTupleInLocalRel() there is an assert [1] that seems to be > > > > > passing IndexRelation to GetRelationIdentityOrPK() whereas it should > > > > > be passing normal relation. > > > > > > > > > > > > > Agreed. But this should lead to assertion failure. Did you try testing > > > > it? > > > > > > No, I did not test this particular case, it impacted me with my other > > > addition of the code where I got Index Relation as input to the > > > RelationGetIndexList() function, and my local changes were impacted by > > > that. I will write a test for this stand-alone case so that it hits > > > the assert. Thanks for looking into this. > > > > The FindReplTupleInLocalRel function can be triggered by both update > > and delete operations, but this only occurs if the relation has been > > marked as updatable by the logicalrep_rel_mark_updatable function. If > > the relation is marked as non-updatable, an error will be thrown by > > check_relation_updatable. Given this, if a relation is updatable, the > > IsIndexUsableForReplicaIdentityFull condition might always evaluate to > > true due to the previous checks in logicalrep_rel_mark_updatable. > > Therefore, it’s possible that we might not encounter the Assert > > statement, as IsIndexUsableForReplicaIdentityFull may consistently be > > true. > > Thoughts? > > With that it seems that the first Assert condition is useless isn't it? > The second part of the assertion is incomplete. The IsIndexUsableForReplicaIdentityFull() should be used only when the remote relation has REPLICA_IDENTITY_FULL set. I haven't tested all possible cases yet but I think the attached should be a better way to write this assertion. -- With Regards, Amit Kapila. v1_improve_assert_worker.patch Description: Binary data
Re: ANALYZE ONLY
Thanks for the feedback David. On Mon, 9 Sept 2024 at 11:27, David Rowley wrote: > You've written "was" (past tense), but then the existing text uses > "will" (future tense). I guess if the point in time is after parse and > before work has been done, then that's correct, but I think using "is" > instead of "was" is better. > Maybe "are also vacuumed" instead of "are vacuumed" is more clear? Agreed. I have updated my patch with both of these suggestions. > 4. A very minor detail, but I think in vacuum.c the WARNING you've > added should use RelationGetRelationName(). We seem to be very > inconsistent with using that macro and I see it's not used just above > for the lock warning, which I imagine you copied. As far as I can tell RelationGetRelationName is for extracting the name from a Relation struct, but in this case we have a RangeVar so it doesn't appear to be applicable. I could not find an equivalent access macro for RangeVar. Thanks again. Cheers Mike v4-0001-Implementation-of-the-ONLY-feature.patch Description: Binary data
Re: not null constraints, again
On Mon, Sep 2, 2024 at 6:33 PM Tender Wang wrote: > > > > The attached patch adds List *nnconstraints, which store the not-null > definition, in struct CreateStmt. > This makes me a little confused about List *constraints in struct CreateStmt. > Actually, the List constraints > store ckeck constraint, and it will be better if the comments can reflect > that. Re-naming it to List *ckconstraints > seems more reasonable. But a lot of codes that use stmt->constraints will be > changed. > hi. seems you forgot to attach the patch? I also noticed this minor issue. I have no preference for Renaming it to List *ckconstraints. +1 for better comments. maybe reword to >>> List *constraints;/* CHECK constraints (list of Constraint nodes) */ >>> On Tue, Sep 3, 2024 at 3:17 PM Tender Wang wrote: > > The test case in constraints.sql, as below: > CREATE TABLE notnull_tbl1 (a INTEGER NOT NULL NOT NULL); > > ^^ > There are two not-null definitions, and is the second one redundant? > hi. i think this is ok. please see function transformColumnDefinition and variable saw_nullable. we need to make sure this: CREATE TABLE notnull_tbl3 (a INTEGER NULL NOT NULL); fails. of course, it's also OK do this: CREATE TABLE notnull_tbl3 (a INTEGER NULL NULL);
Re: Remove emode argument from XLogFileRead/XLogFileReadAnyTLI
On Mon, 9 Sep 2024 12:16:01 +0900
Michael Paquier wrote:
> On Fri, Sep 06, 2024 at 08:10:43PM +0900, Yugo Nagata wrote:
> > Since 1bb2558046c, XLogFileRead() doesn't use the emode argument.
> > Also, since abf5c5c9a4f, XLogFileReadAnyTLI() is called just once
> > and emode is always DEBUG2. So, I think we can remove the emode
> > argument from these functions. I've atached the patch.
>
> It's true that the last relevant caller of XLogFileReadAnyTLI() that
> required an emode is abf5c5c9a4f1, as you say, that's also what I am
> tracking down. Any objections to that?
Thank you for looking into this.
I mean to remove emode from XLogFileRead, too, but this fix is accidentally
missed in the previous patch. I attached the updated patch.
Regards,
Yugo Nagata
--
Yugo NAGATA
diff --git a/src/backend/access/transam/xlogrecovery.c b/src/backend/access/transam/xlogrecovery.c
index 178491f6f5..320b14add1 100644
--- a/src/backend/access/transam/xlogrecovery.c
+++ b/src/backend/access/transam/xlogrecovery.c
@@ -430,9 +430,9 @@ static int emode_for_corrupt_record(int emode, XLogRecPtr RecPtr);
static XLogRecord *ReadCheckpointRecord(XLogPrefetcher *xlogprefetcher,
XLogRecPtr RecPtr, TimeLineID replayTLI);
static bool rescanLatestTimeLine(TimeLineID replayTLI, XLogRecPtr replayLSN);
-static int XLogFileRead(XLogSegNo segno, int emode, TimeLineID tli,
+static int XLogFileRead(XLogSegNo segno, TimeLineID tli,
XLogSource source, bool notfoundOk);
-static int XLogFileReadAnyTLI(XLogSegNo segno, int emode, XLogSource source);
+static int XLogFileReadAnyTLI(XLogSegNo segno, XLogSource source);
static bool CheckForStandbyTrigger(void);
static void SetPromoteIsTriggered(void);
@@ -3780,7 +3780,7 @@ WaitForWALToBecomeAvailable(XLogRecPtr RecPtr, bool randAccess,
* Try to restore the file from archive, or read an existing
* file from pg_wal.
*/
-readFile = XLogFileReadAnyTLI(readSegNo, DEBUG2,
+readFile = XLogFileReadAnyTLI(readSegNo,
currentSource == XLOG_FROM_ARCHIVE ? XLOG_FROM_ANY :
currentSource);
if (readFile >= 0)
@@ -3929,8 +3929,7 @@ WaitForWALToBecomeAvailable(XLogRecPtr RecPtr, bool randAccess,
{
if (!expectedTLEs)
expectedTLEs = readTimeLineHistory(recoveryTargetTLI);
- readFile = XLogFileRead(readSegNo, PANIC,
- receiveTLI,
+ readFile = XLogFileRead(readSegNo, receiveTLI,
XLOG_FROM_STREAM, false);
Assert(readFile >= 0);
}
@@ -4201,7 +4200,7 @@ rescanLatestTimeLine(TimeLineID replayTLI, XLogRecPtr replayLSN)
* Otherwise, it's assumed to be already available in pg_wal.
*/
static int
-XLogFileRead(XLogSegNo segno, int emode, TimeLineID tli,
+XLogFileRead(XLogSegNo segno, TimeLineID tli,
XLogSource source, bool notfoundOk)
{
char xlogfname[MAXFNAMELEN];
@@ -4283,7 +4282,7 @@ XLogFileRead(XLogSegNo segno, int emode, TimeLineID tli,
* This version searches for the segment with any TLI listed in expectedTLEs.
*/
static int
-XLogFileReadAnyTLI(XLogSegNo segno, int emode, XLogSource source)
+XLogFileReadAnyTLI(XLogSegNo segno, XLogSource source)
{
char path[MAXPGPATH];
ListCell *cell;
@@ -4347,8 +4346,7 @@ XLogFileReadAnyTLI(XLogSegNo segno, int emode, XLogSource source)
if (source == XLOG_FROM_ANY || source == XLOG_FROM_ARCHIVE)
{
- fd = XLogFileRead(segno, emode, tli,
- XLOG_FROM_ARCHIVE, true);
+ fd = XLogFileRead(segno, tli, XLOG_FROM_ARCHIVE, true);
if (fd != -1)
{
elog(DEBUG1, "got WAL segment from archive");
@@ -4360,8 +4358,7 @@ XLogFileReadAnyTLI(XLogSegNo segno, int emode, XLogSource source)
if (source == XLOG_FROM_ANY || source == XLOG_FROM_PG_WAL)
{
- fd = XLogFileRead(segno, emode, tli,
- XLOG_FROM_PG_WAL, true);
+ fd = XLogFileRead(segno, tli, XLOG_FROM_PG_WAL, true);
if (fd != -1)
{
if (!expectedTLEs)
@@ -4374,7 +4371,7 @@ XLogFileReadAnyTLI(XLogSegNo segno, int emode, XLogSource source)
/* Couldn't find it. For simplicity, complain about front timeline */
XLogFilePath(path, recoveryTargetTLI, segno, wal_segment_size);
errno = ENOENT;
- ereport(emode,
+ ereport(DEBUG2,
(errcode_for_file_access(),
errmsg("could not open file \"%s\": %m", path)));
return -1;
Re: not null constraints, again
jian he 于2024年9月9日周一 16:31写道： > On Mon, Sep 2, 2024 at 6:33 PM Tender Wang wrote: > > > > > > > > The attached patch adds List *nnconstraints, which store the not-null > definition, in struct CreateStmt. > > This makes me a little confused about List *constraints in struct > CreateStmt. Actually, the List constraints > > store ckeck constraint, and it will be better if the comments can > reflect that. Re-naming it to List *ckconstraints > > seems more reasonable. But a lot of codes that use stmt->constraints > will be changed. > > > hi. > seems you forgot to attach the patch? > I also noticed this minor issue. > I have no preference for Renaming it to List *ckconstraints. > +1 for better comments. maybe reword to > >>> > List *constraints;/* CHECK constraints (list of Constraint > nodes) */ > >>> > I just gave advice; whether it is accepted or not, it's up to Alvaro. If Alvaro agrees with the advice, he will patch a new one. We can continue to review the new patch. If Alvaro disagrees, he doesn't need to change the current patch. I think this way will be more straightforward for others who will review this feature. > > On Tue, Sep 3, 2024 at 3:17 PM Tender Wang wrote: > > > > The test case in constraints.sql, as below: > > CREATE TABLE notnull_tbl1 (a INTEGER NOT NULL NOT NULL); > > > ^^ > > There are two not-null definitions, and is the second one redundant? > > > > hi. > i think this is ok. please see > function transformColumnDefinition and variable saw_nullable. > Yeah, it is ok. > we need to make sure this: > CREATE TABLE notnull_tbl3 (a INTEGER NULL NOT NULL); > fails. > > > of course, it's also OK do this: > CREATE TABLE notnull_tbl3 (a INTEGER NULL NULL); > -- Thanks, Tender Wang
Re: Conflict Detection and Resolution
On Fri, Sep 6, 2024 at 2:05 PM Ajin Cherian wrote: > > > Thank you for your feedback, Shveta. I've addressed both sets of comments you > provided. Thanks for the patches. I am reviewing v12-patch001, it is WIP. But please find first set of comments: 1) src/sgml/logical-replication.sgml: + Users have the option to configure a conflict_resolver Full stop for previous line is missing. 2) + For more information on the conflict_types detected and the supported conflict_resolvers, refer to section CONFLICT RESOLVERS. We may change to : For more information on the supported conflict_types and conflict_resolvers, refer to section CONFLICT RESOLVERS. 3) src/backend/commands/subscriptioncmds.c: Line removed. This change is not needed. static void CheckAlterSubOption(Subscription *sub, const char *option, bool slot_needs_update, bool isTopLevel); - 4) Let's stick to the same comments format as the rest of the file i.e. first letter in caps. + /* first initialise the resolvers with default values */ first --> First initialise --> initialize Same for below comments: + /* validate the conflict type and resolver */ + /* update the corresponding resolver for the given conflict type */ Please verify the rest of the file for the same. 5) Please add below in header of parse_subscription_conflict_resolvers (similar to parse_subscription_options): * This function will report an error if mutually exclusive options are specified. 6) + * Warn users if prerequisites are not met. + * Initialize with default values. + */ + if (stmt->resolvers) + conf_detection_check_prerequisites(); + Would it be better to move the above call inside parse_subscription_conflict_resolvers(), then we will have all resolver related stuff at one place? Irrespective of whether we move it or not, please remove 'Initialize with default values.' from above as that is now not done here. thanks Shveta
Re: Introduce XID age and inactive timeout based replication slot invalidation
On Mon, Sep 9, 2024 at 10:28 AM shveta malik wrote: > > On Mon, Sep 9, 2024 at 10:26 AM Bharath Rupireddy > wrote: > > > > Hi, > > > > On Mon, Sep 9, 2024 at 9:17 AM shveta malik wrote: > > > > > > > We should not allow the invalid replication slot to be altered > > > > irrespective of the reason unless there is any benefit. > > > > > > Okay, then I think we need to change the existing behaviour of the > > > other invalidation causes which still allow alter-slot. > > > > +1. Perhaps, track it in a separate thread? > > I think so. It does not come under the scope of this thread. > It makes sense to me as well. But let's go ahead and get that sorted out first. -- With Regards, Amit Kapila.
Why don't we consider explicit Incremental Sort?
While looking at label_sort_with_costsize() due to another issue, I noticed that we build explicit Sort nodes but not explicit Incremental Sort nodes. I wonder why this is the case. It seems to me that Incremental Sorts are preferable in some cases. For example: create table t (a int, b int); create index on t (a); set enable_seqscan to off; explain (costs off) select * from t t1 join t t2 on t1.a = t2.a and t1.b = t2.b; QUERY PLAN - Merge Join Merge Cond: ((t1.a = t2.a) AND (t1.b = t2.b)) -> Sort Sort Key: t1.a, t1.b -> Index Scan using t_a_idx on t t1 -> Sort Sort Key: t2.a, t2.b -> Index Scan using t_a_idx on t t2 (8 rows) For the outer path of a mergejoin, I think we can leverage Incremental Sort to save effort. For the inner path, though, we cannot do this because Incremental Sort does not support mark/restore. It could be argued that what if a mergejoin with an Incremental Sort as the outer path is selected as the inner path of another mergejoin? Well, I don't think this is a problem, because mergejoin itself does not support mark/restore either, and we will add a Material node on top of it anyway in this case (see final_cost_mergejoin). label_sort_with_costsize is also called in create_append_plan, create_merge_append_plan and create_unique_plan. In these cases, we may also consider using Incremental Sort. But I haven't looked into these cases. Any thoughts? Thanks Richard
Re: Conflict Detection and Resolution
On Fri, Sep 6, 2024 at 2:05 PM Ajin Cherian wrote: > > > > On Thu, Aug 29, 2024 at 2:50 PM shveta malik wrote: >> >> On Wed, Aug 28, 2024 at 4:07 PM shveta malik wrote: >> > >> > > On Wed, Aug 28, 2024 at 10:30 AM Ajin Cherian wrote: >> > > > >> > >> > The review is WIP. Please find a few comments on patch001. >> > >> >> More comments on ptach001 in continuation of previous comments: >> > > Thank you for your feedback, Shveta. I've addressed both sets of comments you > provided. Thanks for the patches. I tested the v12-0001 patch, and here are my comments: 1) An unexpected error occurs when attempting to alter the resolver for multiple conflict_type(s) in ALTER SUB...CONFLICT RESOLVER command. See below examples : postgres=# alter subscription sub2 CONFLICT RESOLVER (update_exists=keep_local, delete_missing=error, update_origin_differs=error); ERROR: unrecognized node type: 1633972341 postgres=# alter subscription sub2 CONFLICT RESOLVER ( update_origin_differs=error, update_exists=error); ERROR: unrecognized node type: 1633972341 postgres=# alter subscription sub2 CONFLICT RESOLVER ( delete_origin_differs=error, delete_missing=error); ERROR: unrecognized node type: 1701602660 postgres=# alter subscription sub2 CONFLICT RESOLVER (update_exists=keep_local, delete_missing=error); ALTER SUBSCRIPTION -- It appears that the error occurs only when at least two conflict types belong to the same category, either UPDATE or DELETE. 2) Given the above issue, it would be beneficial to add a test in subscription.sql to cover cases where all valid conflict types are set with appropriate resolvers in both the ALTER and CREATE commands. Thanks, Nisha
Re: Add RESPECT/IGNORE NULLS and FROM FIRST/LAST options
On Sun, Sep 8, 2024 at 2:22 PM Vik Fearing wrote: > > On 9/7/24 22:25, Oliver Ford wrote: > > On Sat, May 6, 2023 at 9:41 AM Oliver Ford wrote: > >> > >> > >> > >> On Sat, 6 May 2023, 04:57 Tatsuo Ishii, wrote: > >>> > >>> Attached is the patch to implement this (on top of your patch). > >>> > >>> test=# SELECT row_number() RESPECT NULLS OVER () FROM (SELECT 1) AS s; > >>> ERROR: window function row_number cannot have RESPECT NULLS or IGNORE > >>> NULLS > >> > >> > >> The last time this was discussed > >> (https://www.postgresql.org/message-id/1037735.1610402426%40sss.pgh.pa.us) > >> it was suggested to make the feature generalizable, beyond what the > >> standard says it should be limited to. > >> > >> With it generalizable, there would need to be extra checks for custom > >> functions, such as if they allow multiple column arguments (which I'll add > >> in v2 of the patch if the design's accepted). > >> > >> So I think we need a consensus on whether to stick to limiting it to > >> several specific functions, or making it generalized yet agreeing the > >> rules to limit it (such as no agg functions, and no functions with > >> multiple column arguments). > > > > Reviving this thread, I've attached a rebased patch with code, docs, > > and tests and added it to November commitfest. > > Excellent! One of these days we'll get this in. :-) > > I have a problem with this test, though: > > SELECT sum(orbit) RESPECT NULLS OVER () FROM planets; -- succeeds > > Why should that succeed? Especially since aggregates such as SUM() will > ignore nulls! The error message on its partner seems to confirm this: > > SELECT sum(orbit) IGNORE NULLS OVER () FROM planets; -- fails > ERROR: aggregate functions do not accept RESPECT/IGNORE NULLS > > I believe they should both fail. > -- > Vik Fearing Fair enough, here's version 2 where this fails. The ignore_nulls variable is now an int instead of a bool 0002-add-ignore_nulls.patch Description: Binary data
Re: Add memory/disk usage for WindowAgg nodes in EXPLAIN
On Fri, Sep 6, 2024 at 7:21 PM David Rowley wrote: > > On Fri, 6 Sept 2024 at 19:08, Ashutosh Bapat > wrote: > > The changes look better. A nitpick though. With their definitions > > changed, I think it's better to change the names of the functions > > since their purpose has changed. Right now they report the storage > > type and size used, respectively, at the time of calling the function. > > With this patch, they report maximum space ever used and the storage > > corresponding to the maximum space. tuplestore_space_used() may be > > changed to tuplestore_maxspace_used(). I am having difficulty with > > tuplestore_storage_type_name(); tuplestore_largest_storage_type_name() > > seems mouthful and yet not doing justice to the functionality. It > > might be better to just have one funciton tuplestore_maxspace_used() > > which returns both the maximum space used as well as the storage type > > when maximum space was used. > > How about just removing tuplestore_storage_type_name() and > tuplestore_space_used() and adding tuplestore_get_stats(). I did take > some inspiration from tuplesort.c for this, so maybe we can defer back > there for further guidance. I'm not so sure it's worth having a stats > struct type like tuplesort.c has. All we need is a char ** and an > int64 * output parameter to pass to the stats function. I don't think > we need to copy the tuplesort_method_name(). It seems fine just to > point the output parameter of the stats function at the statically > allocated constant. tuplestore_get_stats() similar to tuplesort_get_stats() looks fine. In future the stats reported by this function might expand e.g. maximum number of readers may be included in the stats. If it expands beyond two values, we could think of a separate structure, but for now it looks fine given its limited use. A comment explaining why we aren't using a stats structure and some guidance on when that would be appropriate will be better. -- Best Wishes, Ashutosh Bapat
Re: Test to dump and restore objects left behind by regression
On Fri, Jul 12, 2024 at 10:42 AM Ashutosh Bapat
wrote:
>
> I have merged the two patches now.
>
894be11adfa60ad1ce5f74534cf5f04e66d51c30 changed the schema in which
objects in test genereated_stored.sql are created. Because of this the
new test added by the patch was failing. Fixed the failure in the
attached.
--
Best Wishes,
Ashutosh Bapat
From 3b4573b0d3bb59fd21e01c3887a3d9cab8643238 Mon Sep 17 00:00:00 2001
From: Ashutosh Bapat
Date: Thu, 27 Jun 2024 10:03:53 +0530
Subject: [PATCH 2/2] Test pg_dump/restore of regression objects
002_pg_upgrade.pl tests pg_upgrade on the regression database left
behind by regression run. Modify it to test dump and restore of the
regression database as well.
Regression database created by regression run contains almost all the
database objects supported by PostgreSQL in various states. The test
thus covers wider dump and restore scenarios.
When PG_TEST_EXTRA has 'regress_dump_formats' in it, test dump and
restore in all supported formats. Otherwise test only "plain" format so
that the test finishes quickly.
Author: Ashutosh Bapat
Reviewed by: Michael Pacquire
Discussion: https://www.postgresql.org/message-id/CAExHW5uF5V=Cjecx3_Z=7xfh4rg2wf61pt+hfquzjbqourz...@mail.gmail.com
---
doc/src/sgml/regress.sgml | 13 ++
src/bin/pg_upgrade/t/002_pg_upgrade.pl | 173 ++--
src/test/perl/PostgreSQL/Test/AdjustDump.pm | 109
3 files changed, 277 insertions(+), 18 deletions(-)
create mode 100644 src/test/perl/PostgreSQL/Test/AdjustDump.pm
diff --git a/doc/src/sgml/regress.sgml b/doc/src/sgml/regress.sgml
index d1042e02228..8c1a9ddc403 100644
--- a/doc/src/sgml/regress.sgml
+++ b/doc/src/sgml/regress.sgml
@@ -336,6 +336,19 @@ make check-world PG_TEST_EXTRA='kerberos ldap ssl load_balance libpq_encryption'
+
+
+ regress_dump_formats
+
+
+ When enabled,
+ src/bin/pg_upgrade/t/002_pg_upgrade.pl tests dump
+ and restore of regression database using all dump formats. Otherwise
+ tests only plain format. Not enabled by default
+ because it is resource intensive.
+
+
+
Tests for features that are not supported by the current build
diff --git a/src/bin/pg_upgrade/t/002_pg_upgrade.pl b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
index 17af2ce61ef..613512ffe7d 100644
--- a/src/bin/pg_upgrade/t/002_pg_upgrade.pl
+++ b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
@@ -13,6 +13,7 @@ use File::Path qw(rmtree);
use PostgreSQL::Test::Cluster;
use PostgreSQL::Test::Utils;
use PostgreSQL::Test::AdjustUpgrade;
+use PostgreSQL::Test::AdjustDump;
use Test::More;
# Can be changed to test the other modes.
@@ -36,9 +37,9 @@ sub generate_db
"created database with ASCII characters from $from_char to $to_char");
}
-# Filter the contents of a dump before its use in a content comparison.
-# This returns the path to the filtered dump.
-sub filter_dump
+# Filter the contents of a dump before its use in a content comparison for
+# upgrade testing. This returns the path to the filtered dump.
+sub filter_dump_for_upgrade
{
my ($is_old, $old_version, $dump_file) = @_;
my $dump_contents = slurp_file($dump_file);
@@ -61,6 +62,44 @@ sub filter_dump
return $dump_file_filtered;
}
+# Test that the given two files match. The files usually contain pg_dump
+# output in "plain" format. Output the difference if any.
+sub compare_dumps
+{
+ my ($dump1, $dump2, $testname) = @_;
+
+ my $compare_res = compare($dump1, $dump2);
+ is($compare_res, 0, $testname);
+
+ # Provide more context if the dumps do not match.
+ if ($compare_res != 0)
+ {
+ my ($stdout, $stderr) =
+ run_command([ 'diff', '-u', $dump1, $dump2 ]);
+ print "=== diff of $dump1 and $dump2\n";
+ print "=== stdout ===\n";
+ print $stdout;
+ print "=== stderr ===\n";
+ print $stderr;
+ print "=== EOF ===\n";
+ }
+}
+
+# Adjust the contents of a dump before its use in a content comparison for dump
+# and restore testing. This returns the path to the adjusted dump.
+sub adjust_dump_for_restore
+{
+ my ($dump_file, $is_original) = @_;
+ my $dump_adjusted = "${dump_file}_adjusted";
+
+ open(my $dh, '>', $dump_adjusted)
+ || die "opening $dump_adjusted ";
+ print $dh adjust_regress_dumpfile(slurp_file($dump_file), $is_original);
+ close($dh);
+
+ return $dump_adjusted;
+}
+
# The test of pg_upgrade requires two clusters, an old one and a new one
# that gets upgraded. Before running the upgrade, a logical dump of the
# old cluster is taken, and a second logical dump of the new one is taken
@@ -258,6 +297,12 @@ else
}
}
is($rc, 0, 'regression tests pass');
+
+ # Test dump/restore of the objects left behind by regression. Ideally it
+ # should be done in a separate test, but doing it here saves us one full
+ # regression run. Do this while the old cluster remains usable before
+ # upgrading it.
+ test_regression_dump_restore($oldnode, %node_params);
}
# Initialize a new node for the upg
Re: Why don't we consider explicit Incremental Sort?
Hi, On 9/9/24 11:39, Richard Guo wrote: > While looking at label_sort_with_costsize() due to another issue, I > noticed that we build explicit Sort nodes but not explicit Incremental > Sort nodes. I wonder why this is the case. It seems to me that > Incremental Sorts are preferable in some cases. For example: > I think we intentionally added incremental sort ... incrementally ;-) I don't recall the reasoning exactly, but I think we realized the incremental sort costing can be a bit shaky (and AFAIK we saw a couple cases of that reported). So we added it to places where the reasoning was it wouldn't be a problem and the incremental sort would be a clear win, e.g. thanks to the "cheap startup" thing. > create table t (a int, b int); > create index on t (a); > > set enable_seqscan to off; > > explain (costs off) > select * from t t1 join t t2 on t1.a = t2.a and t1.b = t2.b; >QUERY PLAN > - > Merge Join >Merge Cond: ((t1.a = t2.a) AND (t1.b = t2.b)) >-> Sort > Sort Key: t1.a, t1.b > -> Index Scan using t_a_idx on t t1 >-> Sort > Sort Key: t2.a, t2.b > -> Index Scan using t_a_idx on t t2 > (8 rows) > > For the outer path of a mergejoin, I think we can leverage Incremental > Sort to save effort. For the inner path, though, we cannot do this > because Incremental Sort does not support mark/restore. > > It could be argued that what if a mergejoin with an Incremental Sort > as the outer path is selected as the inner path of another mergejoin? > Well, I don't think this is a problem, because mergejoin itself does > not support mark/restore either, and we will add a Material node on > top of it anyway in this case (see final_cost_mergejoin). > > label_sort_with_costsize is also called in create_append_plan, > create_merge_append_plan and create_unique_plan. In these cases, we > may also consider using Incremental Sort. But I haven't looked into > these cases. > > Any thoughts? > I think one challenge with this case is that create_mergejoin_plan creates these Sort plans explicitly - it's not "pathified" so it doesn't go through the usual cost comparison etc. And it certainly is not the case that incremental sort would win always, so we'd need to replicate the cost comparison logic too. I have not thought about this particular case too much, but how likely is it that mergejoin will win for this plan in practice? If we consider a query that only needs a fraction of rows (say, thanks to LIMIT), aren't we likely to pick a nested loop (which can do incremental sort for the outer plan)? For joins that need to run to completion it might be a win, but there's also the higher risk of a poor costing. I'm not saying it's not worth exploring, I'm just trying recall reasons why it might not work. Also I don't think it's fundamentally impossible to do mark/restore for incremental sort - I just haven't tried doing it because it wasn't necessary. In the worst case we could simply add a Materialize node on top, no? regards -- Tomas Vondra
Re: POC, WIP: OR-clause support for indexes
On Wed, Sep 4, 2024 at 6:42 PM Alena Rybakina wrote: > On 04.09.2024 18:31, Alena Rybakina wrote: > > I rewrote the tests with integer types. Thanks for your suggestion. If > > you don't mind, I've updated the diff file you attached earlier to > > include the tests. > Sorry, I've just noticed that one of your changes with the regression > test wasn't included. I fixed it here. Please, find the revised patchset attached. I've integrated the fixes by you and Andrei in the thread. Also, I've addressed the note from Andrei [1] about construction of RestrictInfos. I decided to use make_simple_restrictinfo() in match_orclause_to_indexcol(), because I've seen its usage in get_index_clause_from_support(). Also, I agree it get it's wrong to directly copy RestrictInfo struct in group_similar_or_args(). Instead, I've renamed make_restrictinfo_internal() to make_plain_restrictinfo(), which is intended to handle non-recursive cases when you've children already wrapped with RestrictInfos. make_plain_restrictinfo() now used in group_similar_or_args(). Hopefully, this item is resolved by now. Links. 1. https://www.postgresql.org/message-id/60760203-4917-4c6c-ac74-a5ee764735a4%40gmail.com -- Regards, Alexander Korotkov v39-0002-Teach-bitmap-path-generation-about-transforming-.patch Description: Binary data v39-0001-Transform-OR-clauses-to-SAOP-s-during-index-matc.patch Description: Binary data
Re: Add support for (Var op Var) clause in extended MCV statistics
Hi everyone,
I've taken a closer look at the patch and realized that we don't need
the new function 'mcv_clause_selectivity_var_op_var()' we can use
'mcv_clause_selectivity()' instead.
I'm attaching the updated patch and test generator.
--
Regards,
Ilia Evdokimov,
Tantor Labs LCC.
#!/usr/bin/python3
import psycopg2
import random
import select
import hashlib
def generate_conditions(nclauses, attributes = ['a', 'b', 'c', 'd'], operators = ['<', '<=', '=', '!=', '>=', '>']):
if nclauses == 1:
cols = [random.choice(attributes), random.choice(attributes)]
oper = ' ' + random.choice(operators) + ' '
clause = oper.join(cols)
if random.randint(0,100) < 50:
clause = 'NOT ' + clause
return clause
nparts = random.randint(2, nclauses)
# distribute the clauses between query parts
nclauses_parts = [1 for p in range(0, nparts)]
for x in range(0, nclauses - nparts):
nclauses_parts[random.randint(0, nparts) - 1] += 1
parts = []
for p in range(0, nparts):
parts.append('(' + generate_conditions(nclauses_parts[p], attributes, operators) + ')')
c = random.choice([' AND ', ' OR '])
return c.join(parts)
def generate_data(nrows, attributes = ['a', 'b', 'c', 'd']):
sql = 'insert into t (' + ','.join(attributes) + ') select '
attrs = []
for attr in attributes:
x = random.choice([-1, 2, 5, 10, 20, 30])
if x == -1:
x = random.randint(5, 20)
expr = '(random() * ' + str(x) + ')::int'
else:
expr = 'mod(i,' + str(x) + ')'
if random.randint(0,100) < 50:
x = random.choice([2, 5, 10, 20, 30])
attrs.append('case when mod(i,' + str(x) + ') = 0 then null else ' + expr + ' end')
else:
attrs.append(expr)
sql += ', '.join(attrs) + ' from generate_series(1,' + str(nrows) + ') s(i)'
return sql
def wait(conn):
while True:
state = conn.poll()
if state == psycopg2.extensions.POLL_OK:
break
elif state == psycopg2.extensions.POLL_WRITE:
select.select([], [conn.fileno()], [])
elif state == psycopg2.extensions.POLL_READ:
select.select([conn.fileno()], [], [])
else:
raise psycopg2.OperationalError("poll() returned %s" % state)
def run_everywhere(conns, queries):
curs = [conn.cursor() for conn in conns]
for q in queries:
[cur.execute(q) for cur in curs]
[wait(conn) for conn in conns]
[cur.close() for cur in curs]
if __name__ == '__main__':
conns = [
psycopg2.connect('host=localhost port=5001 user=postgres dbname=postgres', async_=True),
psycopg2.connect('host=localhost port=5002 user=postgres dbname=postgres', async_=True),
psycopg2.connect('host=localhost port=5003 user=postgres dbname=postgres', async_=True),
psycopg2.connect('host=localhost port=5004 user=postgres dbname=postgres', async_=True)]
[wait(conn) for conn in conns]
curs = [conn.cursor() for conn in conns]
# 100 data sets
for cnt in [3, 10, 100]:
for d in range(1,100):
# generate the data on all versions
data_sql = generate_data(cnt)
data_md5 = hashlib.md5(data_sql.encode('utf-8')).hexdigest()
with open('data.csv', 'a') as f:
f.write('%s\t%s\n' % (data_md5, data_sql))
run_everywhere(conns, ['drop table if exists t', 'create table t (a int, b int, c int, d int)', data_sql, 'commit', 'analyze'])
# generate the clauses
conditions = []
for c in range(1, 6):
for q in range(1,100):
conditions.append({'clauses' : c, 'conditions' : generate_conditions(c)})
with open('results.csv', 'a') as f:
for conds in conditions:
sql = "select * from check_estimated_rows('select * from t where " + conds['conditions'] + "')"
[cur.execute(sql) for cur in curs]
[wait(conn) for conn in conns]
r = [cur.fetchone() for cur in curs]
actual_rows = r[0][1]
estimated_rows = [str(x[0]) for x in r]
f.write(('%s\t%s\t%s\t%s\t%s\t%s\t%s\n') % (data_md5, cnt, conds['clauses'], conds['conditions'], 'no', actual_rows, '\t'.join(estimated_rows)))
run_everywhere(conns, ['create statistics s (mcv) on a, b, c, d from t', 'commit', 'analyze'])
with open('results.csv', 'a') as f:
for conds in conditions:
sql = "select * from check_estimated_rows('select * from t where " + conds['conditions'] + "')"
[cur.execute(sql) for cur in curs]
[wait(conn) for conn in conns]
r = [cur.fetchone() for cur in curs]
actual_rows = r[0][1]
estimated_rows = [str(x[0]) for x in r]
f.write(('%s\t%s\t%s\t%s\t%s\t%s\t%s\n') % (data_md5, cnt, conds['clauses'], conds['conditions'], 'yes', actual_rows, '\t'.join(estimated_rows)))
From 33090e336b81dea2a81a71548f43207dcf399728 Mon Sep 17 00:00:00 2001
From: Ilia Evdokimov
Date: Sat, 10 Aug 2024 14:35:25 +0300
Subject: [PATCH v4] Add support for (Var op Var) clause in extended MCV
statistics
Added a new leaf to the existing clauses tree, allowing the calculation
of selectivity specifically for (Var op Var) clauses. The new function
for this selectivity calculation has been integratedinto
the extended statistics mechanism, ensuri
Re: Add contrib/pg_logicalsnapinspect
On Fri, Aug 30, 2024 at 5:18 PM Bertrand Drouvot wrote: > > On Fri, Aug 30, 2024 at 03:43:12PM +0530, Amit Kapila wrote: > > On Thu, Aug 29, 2024 at 6:33 PM Bharath Rupireddy > > wrote: > > > > > > On Thu, Aug 29, 2024 at 3:44 PM Bertrand Drouvot > > > wrote: > > > > > > > > Yeah that's fair. And now I'm wondering if we need an extra module. I > > > > think > > > > we could "simply" expose 2 new functions in core, thoughts? > > > > > > > > > > What do you think? Did you have something else in mind? > > > > > > > > > > > > > > > > On similar lines, we can also provide a function to get the slot's > > > > > on-disk data. > > > > > > > > Yeah, having a way to expose the data from the disk makes fully sense > > > > to me. > > > > > > > > > IIRC, Bharath had previously proposed a tool to achieve > > > > > the same. It is fine if we don't want to add that as part of this > > > > > patch but I mentioned it because by having that we can have a set of > > > > > functions to view logical decoding data. > > > > > > > > That's right. I think this one would be simply enough to expose one or > > > > two > > > > functions in core too (and probably would not need an extra module). > > > > > > +1 for functions in core unless this extra module > > > pg_logicalsnapinspect works as a tool to be helpful even when the > > > server is down. > > > > > > > We have an example of pageinspect which provides low-level functions > > to aid debugging. The proposal for these APIs also seems to fall in > > the same category, > > That's right, but... > > > so why go for the core for these functions? > > as we decided not to expose the SnapBuildOnDisk and SnapBuild structs to > public > and to create/expose 2 new functions in snapbuild.c then the functions in the > module would do nothing but expose the data coming from the snapbuild.c's > functions (get the tuple and send it to the client). That sounds weird to me > to > create a module that would "only" do so, that's why I thought that in core > functions taking care of everything make more sense. > I see your point. Does anyone else have an opinion on the need for these functions and whether to expose them from a contrib module or have them as core functions? -- With Regards, Amit Kapila.
broken build - FC 41
Hi
I try new Fedora 41. Build fails
echo 'Name: libpq' >>libpq.pc
echo 'Description: PostgreSQL libpq library' >>libpq.pc
echo 'URL: https://www.postgresql.org/' >>libpq.pc
echo 'Version: 18devel' >>libpq.pc
echo 'Requires: ' >>libpq.pc
echo 'Requires.private: libssl, libcrypto' >>libpq.pc
echo 'Cflags: -I${includedir}' >>libpq.pc
echo 'Libs: -L${libdir} -lpq' >>libpq.pc
echo 'Libs.private: -L/usr/lib64 -lpgcommon -lpgport -lssl -lm' >>libpq.pc
fe-secure-openssl.c:62:10: fatal error: openssl/engine.h: Adresář nebo
soubor neexistuje
62 | #include
| ^~
compilation terminated.
Regards
Pavel
Re: broken build - FC 41
> On 9 Sep 2024, at 13:45, Pavel Stehule wrote: > echo 'Libs.private: -L/usr/lib64 -lpgcommon -lpgport -lssl -lm' >>libpq.pc > fe-secure-openssl.c:62:10: fatal error: openssl/engine.h: Adresář nebo soubor > neexistuje >62 | #include > | ^~ > compilation terminated. That implies OPENSSL_NO_ENGINE isn't defined while the engine header is missing, which isn't really a workable combination. Which version of OpenSSL is this? -- Daniel Gustafsson
Re: broken build - FC 41
On Mon, 09 Sep 2024 13:45:50 +0200, Pavel Stehule wrote:
>
> Hi
>
> I try new Fedora 41. Build fails
>
> echo 'Name: libpq' >>libpq.pc
> echo 'Description: PostgreSQL libpq library' >>libpq.pc
> echo 'URL: https://www.postgresql.org/' >>libpq.pc
> echo 'Version: 18devel' >>libpq.pc
> echo 'Requires: ' >>libpq.pc
> echo 'Requires.private: libssl, libcrypto' >>libpq.pc
> echo 'Cflags: -I${includedir}' >>libpq.pc
> echo 'Libs: -L${libdir} -lpq' >>libpq.pc
> echo 'Libs.private: -L/usr/lib64 -lpgcommon -lpgport -lssl -lm' >>libpq.pc
> fe-secure-openssl.c:62:10: fatal error: openssl/engine.h: Adresář nebo
> soubor neexistuje
>62 | #include
> | ^~
> compilation terminated.
I am not a Fedora user but have you installed openssl-devel-engine?
--
Herbert
access numeric data in module
I'm the maintainer of the PL/Haskell language extension. ( https://github.com/ed-o-saurus/PLHaskell/) I want to be able to have a function received and/or return numeric data. However, I'm having trouble getting data from Datums and building Datums to return. numeric.h does not contain the macros to do this. They are in numeric.c. Is there a way to access the values in the numeric structures? If not, would a PR to move macros to numeric.h be welcome in the next commitfest? -Ed
Re: Add support to TLS 1.3 cipher suites and curves lists
> On 22 Jul 2024, at 19:14, Jacob Champion
> wrote:
>
> On Fri, Jul 12, 2024 at 1:03 PM Daniel Gustafsson wrote:
>> The original author added the string parsing in order to provide a good error
>> message in case of an error in the list, and since that seemed like a nice
>> idea
>> I kept in my review revision. With what you said above I agree it's not
>> worth
>> the extra complexity it brings so the attached revision removes it.
>
> Misspelling a group now leads to the following error message for OpenSSL 3.0:
>
>FATAL: ECDH: failed to set curve names: no SSL error reported
>
> Maybe a HINT would be nice here?:
>
>HINT: Check that each group name is both spelled correctly and
> supported by the installed version of OpenSSL.
Good catch. OpenSSL 3.2 changed the error message to be a lot more helpful,
before that there is no error added to the queue at all for this processing
(hence the "no SSL error reported"). The attached adds a hint as well as a
proper error message for OpenSSL versions prior to 3.2. Pushing an error on
the queue would've been nice but we can't replicate the OpenSSL level of detail
in the error until we require OpenSSL 3.0 as the base since that's when _data
error reporting was added.
>> I don't have strong opinions on
>> renaming ssl_ecdh_curve to reflect that it can take a list of multiple
>> values,
>> there is merit to having descriptive names but it would also be an invasive
>> change for adding suffix 's'.
>
> Can we just add an entry to map_old_guc_names to handle it? Something
> like (untested)
>
> static const char *const map_old_guc_names[] = {
> "sort_mem", "work_mem",
> "vacuum_mem", "maintenance_work_mem",
> +"ssl_ecdh_curve", "ssl_groups",
> NULL
> };
>
> Re: Andres' concern about the ECDH part of the name, we could probably
> keep the "dh" part, but I'd be wary of that changing underneath us
> too. IANA changed the registry name to "TLS Supported Groups".
Fair point, I've renamed to ssl_groups and added a mapping from the old name as
well as a note in the docs that the parameter has changed name (and ability to
handle more than one).
> Is there an advantage to setting it to a compile-time default, as
> opposed to just leaving it alone and not setting it at all? With the
> current patch, if you dropped in a more advanced OpenSSL 3.x that
> changed up the defaults, you wouldn't see any benefit.
Not really, I have changed such that a blank GUC does *no* OpenSSL call at all
which will retain the default from the local OpenSSL installation.
The attached version also has a new 0001 which bumps the minimum required
OpenSSL version to 1.1.1 (from 1.1.0) since this patchset requires API's only
present in 1.1.1 and onwards. To keep it from being hidden here I will raise a
separate thread about it.
--
Daniel Gustafsson
v5-0003-Support-configuring-TLSv1.3-cipher-suites.patch
Description: Binary data
v5-0002-Support-configuring-multiple-ECDH-curves.patch
Description: Binary data
v5-0001-Raise-the-minimum-supported-OpenSSL-version-to-1..patch
Description: Binary data
Retire support for OpenSSL 1.1.1 due to raised API requirements
Commit a70e01d430 removed support for OpenSSL 1.0.2 in order to simplify the code by removing the need for finicky initialization of the library. Based on our API usage the new minimum version was defined as 1.1.0. The patchset in https://commitfest.postgresql.org/49/5025/ which adds support for configuring cipher suites in TLS 1.3 handshakes require an API available in OpenSSL 1.1.1 and onwards. With that as motivation I'd like to propose that we remove support for OpenSSL 1.1.0 and set the minimum required version to 1.1.1. OpenSSL 1.1.0 was EOL in September 2019 and was never an LTS version, so it's not packaged in anything anymore AFAICT and should be very rare in production use in conjunction with an updated postgres. 1.1.1 LTS will be 2 years EOL by the time v18 ships so I doubt this will be all that controversial. The attached is the 0001 from the above mentioned patchset for illustration. The removal should happen when pushing the rest of the patchset. Does anyone see any reason not to go to 1.1.1 as the minimum? -- Daniel Gustafsson v5-0001-Raise-the-minimum-supported-OpenSSL-version-to-1..patch Description: Binary data
Re: [PATCH] Add roman support for to_number function
Hi,
On Sun, Sep 8, 2024 at 4:09 AM Tom Lane wrote:
> A few notes from a quick read of the patch:
>
> * roman_to_int() should have a header comment, notably explaining its
> result convention. I find it fairly surprising that "0" means an
> error --- I realize that Roman notation can't represent zero, but
> wouldn't it be better to use -1?
>
Noted.
>
> * Do *not* rely on toupper(). There are multiple reasons why not,
> but in this context the worst one is that in Turkish locales it's
> likely to translate "i" to "İ", on which you will fail. I'd use
> pg_ascii_toupper().
>
Noted.
>
> * I think roman_to_int() is under-commented internally too.
> To start with, where did the magic "15" come from? And why
> should we have such a test anyway --- what if the format allows
> for trailing stuff after the roman numeral? (That is, I think
> you need to fix this so that roman_to_int reports how much data
> it ate, instead of assuming that it must read to the end of the
> input.)
MMMDCCCLXXXVIII is the longest valid standard roman numeral (15
characters). I will add appropriate comment on length check.
I am not sure I am able to understand the latter part. Could you please
elaborate it?
Are you referring to cases like these?
SELECT to_number('CXXIII foo', 'RN foo');
SELECT to_number('CXXIII foo', 'RN');
Please check my comments on Oracle's behaviour below.
> The logic for detecting invalid numeral combinations
> feels a little opaque too. Do you have a source for the rules
> you're implementing, and if so could you cite it?
>
There are many sources on the internet.
A few sources:
1. https://www.toppr.com/guides/maths/knowing-our-numbers/roman-numerals/
2. https://www.cuemath.com/numbers/roman-numerals/
Note that we are following the standard form:
https://en.wikipedia.org/wiki/Roman_numerals#Standard_form
>
> * This code will get run through pgindent before commit, so you
> might want to revisit whether your comments will still look nice
> afterwards. There's not a lot of consistency in them about initial
> cap versus lower case or trailing period versus not, too.
>
Noted.
>
> * roman_result could be declared where used, no?
>
Noted.
>
> * I'm not really convinced that we need a new errcode
> ERRCODE_INVALID_ROMAN_NUMERAL rather than using a generic one like
> ERRCODE_INVALID_TEXT_REPRESENTATION. However, if we do do it
> like that, why did you pick the out-of-sequence value 22P07?
>
22P07 is not out-of-sequence. 22P01 to 22P06 are already used.
However, I do agree with you that we can use
ERRCODE_INVALID_TEXT_REPRESENTATION. I will change it.
>
> * Might be good to have a few test cases demonstrating what happens
> when there's other stuff combined with the RN format spec. Notably,
> even if RN itself won't eat whitespace, there had better be a way
> to write the format string to allow that.
>
The current patch (v3) simply ignores other formats with RN except when RN
is used which returns error.
```
postgres=# SELECT to_number('CXXIII', 'foo RN');
to_number
---
123
(1 row)
postgres=# SELECT to_number('CXXIII', 'MIrn');
to_number
---
123
(1 row)
postgres=# SELECT to_number('CXXIII', 'rn');
ERROR: "" must be the last pattern used
```
However, I think that other formats except FM should be rejected when used
with RN in NUMDesc_prepare function. Any opinions?
If we look into Oracle, they are strict in this regard and don't allow any
other format with RN.
1. SELECT to_char(12, 'MIrn') from dual;
2. SELECT to_char(12, 'foo RN') from dual;
results in error:
ORA-01481: invalid number format model
I also found that there is no check when RN is used twice (both in to_char
and to_number) and that results in unexpected output.
```
postgres=# SELECT to_number('CXXIII', 'RNrn');
to_number
---
123
(1 row)
postgres=# SELECT to_char(12, 'RNrn');
to_char
XIIxii
(1 row)
postgres=# SELECT to_char(12, 'rnrn');
to_char
xiixii
(1 row)
postgres=# SELECT to_char(12, 'FMrnrn');
to_char
-
xiixii
(1 row)
```
> * Further to Aleksander's point about lack of test coverage for
> the to_char direction, I see from
>
> https://coverage.postgresql.org/src/backend/utils/adt/formatting.c.gcov.html
> that almost none of the existing roman-number code paths are covered
> today. While it's not strictly within the charter of this patch
> to improve that, maybe we should try to do so --- at the very
> least it'd raise formatting.c's coverage score a few notches.
>
>
I see that you have provided a patch to increase test coverage of to_char
roman format including some other formats. I will try to add more tests for
the to_number roman format.
I will provide the next patch soon.
Regards,
Hunaid Sohail
Re: Remove hardcoded hash opclass function signature exceptions
On 06.09.24 21:43, Tom Lane wrote:
Peter Eisentraut writes:
hashvalidate(), which validates the signatures of support functions for
the hash AM, contains several hardcoded exceptions.
...
This patch removes those exceptions by providing new support functions
that have the proper declared signatures. They internally share most of
the C code with the "wrong" functions they replace, so the behavior is
still the same.
+1 for cleaning this up. A couple of minor nitpicks:
* I don't really like the new control structure, or rather lack of
structure, in hashvalidate. In particular the uncommented
s/break/continue/ changes look like typos. They aren't, but can't
you do this in a less confusing fashion? Or at least add comments
like "continue not break because the code below the switch doesn't
apply to this case".
Ok, I cleaned that up a bit.
* Hand-picking OIDs as you did in pg_proc.dat is kind of passé now.
I guess it's all right as long as nobody else does the same thing in
the near future, but ...
Renumbered with the suggested "random" numbers.
Not done here, but maybe hashvarlena() and hashvarlenaextended() should
be removed from pg_proc.dat, since their use as opclass support
functions is now dubious.
I wish we could get rid of those, but according to
codesearch.debian.net, postgis and a couple of other extensions are
relying on them. If we remove them we'll break any convenient upgrade
path for those extensions.
Those are using the C function, which is ok. I was thinking about
removing the SQL function (from pg_proc.dat), because you can't use that
for much anymore. (You can't call it directly, and the hash AM will no
longer accept it.) I have done that in this patch version and added
some code comments around it.
From 423a2ea6688b2e71b149c6cf2cbef9eb5b280f0e Mon Sep 17 00:00:00 2001
From: Peter Eisentraut
Date: Mon, 9 Sep 2024 14:22:00 +0200
Subject: [PATCH v2] Remove hardcoded hash opclass function signature
exceptions
hashvalidate(), which validates the signatures of support functions
for the hash AM, contained several hardcoded exceptions. For example,
hash/date_ops support function 1 was hashint4(), which would
ordinarily fail validation because the function argument is int4, not
date. But this works internally because int4 and date are of the same
size. There are several more exceptions like this that happen to work
and were allowed historically but would now fail the function
signature validation.
This patch removes those exceptions by providing new support functions
that have the proper declared signatures. They internally share most
of the code with the "wrong" functions they replace, so the behavior
is still the same.
With the exceptions gone, hashvalidate() is now simplified and relies
fully on check_amproc_signature().
hashvarlena() and hashvarlenaextended() are removed from pg_proc.dat.
Their use as opclass support functions is dubious, and since they have
an argument of type internal, they can't be called directly. The
underlying C functions continue to exist in the C code as internal
support functions. (Some extensions define SQL-level hash functions
that use them as the C implementation.)
Discussion:
https://www.postgresql.org/message-id/flat/29c3b746-69e7-482a-b37c-dbbf7e5b0...@eisentraut.org
TODO: catversion
---
src/backend/access/hash/hashfunc.c | 21
src/backend/access/hash/hashvalidate.c | 130 +
src/backend/utils/adt/bool.c | 13 +++
src/backend/utils/adt/date.c | 12 +++
src/backend/utils/adt/timestamp.c | 12 +++
src/backend/utils/adt/xid.c| 37 +++
src/include/catalog/pg_amproc.dat | 30 +++---
src/include/catalog/pg_proc.dat| 48 +++--
8 files changed, 177 insertions(+), 126 deletions(-)
diff --git a/src/backend/access/hash/hashfunc.c
b/src/backend/access/hash/hashfunc.c
index 86f1adecb75..dd44e4330af 100644
--- a/src/backend/access/hash/hashfunc.c
+++ b/src/backend/access/hash/hashfunc.c
@@ -376,7 +376,16 @@ hashtextextended(PG_FUNCTION_ARGS)
/*
* hashvarlena() can be used for any varlena datatype in which there are
* no non-significant bits, ie, distinct bitpatterns never compare as equal.
+ *
+ * (However, you need to define an SQL-level wrapper function around it with
+ * the concrete input data type; otherwise hashvalidate() won't accept it.
+ * Moreover, at least for built-in types, a C-level wrapper function is also
+ * recommended; otherwise, the opr_sanity test will get upset.)
*/
+
+extern Datum hashvarlena(PG_FUNCTION_ARGS);
+extern Datum hashvarlenaextended(PG_FUNCTION_ARGS);
+
Datum
hashvarlena(PG_FUNCTION_ARGS)
{
@@ -406,3 +415,15 @@ hashvarlenaextended(PG_FUNCTION_ARGS)
return result;
}
+
+Datum
+hashbytea(PG_FUNCTION_ARGS)
+{
+ return hashvarlena(fcinfo);
+}
+
+Datum
+hashbyteaextended(PG_FUNCTION_ARGS)
+{
+ return hashvarlenaextended(fcinfo);
+}
diff --git a/src/backend/access/hash/hashval
Re: Undocumented functions
Em sáb., 7 de set. de 2024 às 17:18, Tom Lane escreveu Functions that are primarily meant to implement operators are normally not documented separately: we feel it would bloat the docs without adding a lot Those two functions, elem_contained_by_range and pg_relation_is_updatable were only examples of hundreds of functions which are not documented. The real question here is not for the ones that are used internally because operators or types need them, I'm talking about those ones which does not have a way to replace it ? pg_get_shmem_allocations is cool and is not mentioned on DOC pg_is_in_backup was mentioned until version 12, then removed. Why, it´s not used anymore. This is the question, what functions exist and are useful but are not documented ? regards Marcos Marcos
Re: broken build - FC 41
po 9. 9. 2024 v 13:58 odesílatel Herbert J. Skuhra
napsal:
> On Mon, 09 Sep 2024 13:45:50 +0200, Pavel Stehule wrote:
> >
> > Hi
> >
> > I try new Fedora 41. Build fails
> >
> > echo 'Name: libpq' >>libpq.pc
> > echo 'Description: PostgreSQL libpq library' >>libpq.pc
> > echo 'URL: https://www.postgresql.org/' >>libpq.pc
> > echo 'Version: 18devel' >>libpq.pc
> > echo 'Requires: ' >>libpq.pc
> > echo 'Requires.private: libssl, libcrypto' >>libpq.pc
> > echo 'Cflags: -I${includedir}' >>libpq.pc
> > echo 'Libs: -L${libdir} -lpq' >>libpq.pc
> > echo 'Libs.private: -L/usr/lib64 -lpgcommon -lpgport -lssl -lm'
> >>libpq.pc
> > fe-secure-openssl.c:62:10: fatal error: openssl/engine.h: Adresář nebo
> > soubor neexistuje
> >62 | #include
> > | ^~
> > compilation terminated.
>
> I am not a Fedora user but have you installed openssl-devel-engine?
>
> <
> https://packages.fedoraproject.org/pkgs/openssl/openssl-devel-engine/fedora-41.html#files
> >
>
It helps
Thank you.
Pavel
>
> --
> Herbert
>
>
>
Re: broken build - FC 41
po 9. 9. 2024 v 13:57 odesílatel Daniel Gustafsson napsal: > > On 9 Sep 2024, at 13:45, Pavel Stehule wrote: > > > echo 'Libs.private: -L/usr/lib64 -lpgcommon -lpgport -lssl -lm' > >>libpq.pc > > fe-secure-openssl.c:62:10: fatal error: openssl/engine.h: Adresář nebo > soubor neexistuje > >62 | #include > > | ^~ > > compilation terminated. > > That implies OPENSSL_NO_ENGINE isn't defined while the engine header is > missing, which isn't really a workable combination. Which version of > OpenSSL > is this? > I needed to install Name : openssl-devel-engine Epoch : 1 Version: 3.2.2 Release: 5.fc41 Architecture : x86_64 Download size : 44.0 KiB Installed size : 52.8 KiB Source : openssl-3.2.2-5.fc41.src.rpm Repository : fedora Summary: Files for development of applications which will use OpenSSL and use deprecated ENGINE API. URL: http://www.openssl.org/ License: Apache-2.0 Description: OpenSSL is a toolkit for supporting cryptography. The openssl-devel-engine : package contains include files needed to develop applications which : use deprecated OpenSSL ENGINE functionality. Vendor : Fedora Project pavel@nemesis:~$ sudo dnf install openssl-devel-engine Updating and loading repositories: Repositories loaded. Package Today I upgraded from FC40 to FC41, and only this library was installed to make the build. The question is why the missing header was not detected by configure? The description of this package says so the OpenSSL ENGINE is deprecated? Regards Pavel > -- > Daniel Gustafsson > >
Re: Psql meta-command conninfo+
Hi Hunaid On 02.08.24 14:11, Hunaid Sohail wrote: > > I have also edited the documentation and added it to the patch. Please > let me know if any changes are required. > I just wanted to review this patch again but v30 does not apply === Applying patches on top of PostgreSQL commit ID d8df7ac5c04cd17bf13bd3123dcfcaf8007c6280 === /etc/rc.d/jail: WARNING: Per-jail configuration via jail_* variables is obsolete. Please consider migrating to /etc/jail.conf. === applying patch ./v30-0001-psql-meta-command-conninfo-plus.patch patch unexpectedly ends in middle of line gpatch: Only garbage was found in the patch input. I will set the status to "Waiting on Author". -- Jim
Re: tiny step toward threading: reduce dependence on setlocale()
On 08.08.24 22:00, Jeff Davis wrote:
On Wed, 2024-08-07 at 22:44 +0200, Peter Eisentraut wrote:
But after this patch set, locale cannot be NULL anymore, so the third
branch is obsolete.
...
Second, there are a number of functions in like.c like the above that
take separate arguments like pg_locale_t locale, bool locale_is_c.
Because pg_locale_t now contains the locale_is_c information, these
can
be combined.
I believe these patches are correct, but the reasoning is fairly
complex:
1. Some MatchText variants are called with 0 for locale. But that's OK
because ...
2. A MatchText variant only cares about the locale if MATCH_LOWER(t) is
defined, and ...
3. Only one variant, SB_IMatchText() defines MATCH_LOWER(), and ...
4. SB_IMatchText() is called with a non-zero locale.
All of these are a bit confusing to follow because it's generated code.
#2 is particularly non-obvious, because "locale" is not even an
argument of the MATCH_LOWER(t) or GETCHAR(t) macros, it's taken
implicitly from the outer scope.
I don't think your patches cause this confusion, but is there a way you
can clarify some of this along the way?
In the end, I figured the best thing to do here is to add an explicit
locale argument to MATCH_LOWER() and GETCHAR() so one can actually
understand this code by reading it. New patch attached.
From 53c5dd60ca8a7d9500624a430c7776bfa345a396 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut
Date: Mon, 9 Sep 2024 15:33:25 +0200
Subject: [PATCH v2] Remove separate locale_is_c arguments
Since e9931bfb751, ctype_is_c is part of pg_locale_t. Some functions
passed a pg_locale_t and a bool argument separately. This can now be
combined into one argument.
Since some callers call MatchText() with locale 0, it is a bit
confusing whether this is all correct. But it is the case that only
callers that pass a non-zero locale object to MatchText() end up
checking locale->ctype_is_c. To make that flow a bit more
understandable, add the locale argument to MATCH_LOWER() and GETCHAR()
in like_match.c, instead of implicitly taking it from the outer scope.
---
src/backend/utils/adt/like.c | 30 +++---
src/backend/utils/adt/like_match.c | 20 +---
2 files changed, 24 insertions(+), 26 deletions(-)
diff --git a/src/backend/utils/adt/like.c b/src/backend/utils/adt/like.c
index f87675d7557..9fb036ad014 100644
--- a/src/backend/utils/adt/like.c
+++ b/src/backend/utils/adt/like.c
@@ -33,18 +33,18 @@
static int SB_MatchText(const char *t, int tlen, const char *p, int plen,
-pg_locale_t locale, bool
locale_is_c);
+pg_locale_t locale);
static text *SB_do_like_escape(text *pat, text *esc);
static int MB_MatchText(const char *t, int tlen, const char *p, int plen,
-pg_locale_t locale, bool
locale_is_c);
+pg_locale_t locale);
static text *MB_do_like_escape(text *pat, text *esc);
static int UTF8_MatchText(const char *t, int tlen, const char *p, int plen,
- pg_locale_t locale, bool
locale_is_c);
+ pg_locale_t locale);
static int SB_IMatchText(const char *t, int tlen, const char *p, int plen,
- pg_locale_t locale, bool
locale_is_c);
+ pg_locale_t locale);
static int GenericMatchText(const char *s, int slen, const char *p, int
plen, Oid collation);
static int Generic_Text_IC_like(text *str, text *pat, Oid collation);
@@ -91,9 +91,9 @@ wchareq(const char *p1, const char *p2)
* fold-on-the-fly processing, however.
*/
static char
-SB_lower_char(unsigned char c, pg_locale_t locale, bool locale_is_c)
+SB_lower_char(unsigned char c, pg_locale_t locale)
{
- if (locale_is_c)
+ if (locale->ctype_is_c)
return pg_ascii_tolower(c);
else
return tolower_l(c, locale->info.lt);
@@ -129,7 +129,7 @@ SB_lower_char(unsigned char c, pg_locale_t locale, bool
locale_is_c)
#include "like_match.c"
/* setup to compile like_match.c for single byte case insensitive matches */
-#define MATCH_LOWER(t) SB_lower_char((unsigned char) (t), locale, locale_is_c)
+#define MATCH_LOWER(t, locale) SB_lower_char((unsigned char) (t), locale)
#define NextChar(p, plen) NextByte((p), (plen))
#define MatchText SB_IMatchText
@@ -158,11 +158,11 @@ GenericMatchText(const char *s, int slen, const char *p,
int plen, Oid collation
}
if (pg_database_encoding_max_length() == 1)
- return SB_MatchText(s, slen, p, plen, 0, true);
+ return SB_MatchText(s, slen, p, plen, 0);
else if (GetDatabaseEncoding() == PG_UTF8)
- return UTF8_MatchText(s, slen, p, plen, 0, true);
+
Re: pgstattuple: fix free space calculation
Hi Tom, thanks for your review. On 9/7/24 22:10, Tom Lane wrote: I looked at this patch. I agree with making the change. However, I don't agree with the CF entry's marking of "target version: stable" (i.e., requesting back-patch). I think this falls somewhere in the gray area between a bug fix and a definitional change. Also, people are unlikely to be happy if they suddenly get new, not-comparable numbers after a minor version update. So I think we should just fix it in HEAD. OK, I did the change. As far as the patch itself goes, the one thing that is bothering me is this comment change /* - * It's not safe to call PageGetHeapFreeSpace() on new pages, so we + * It's not safe to call PageGetExactFreeSpace() on new pages, so we * treat them as being free space for our purposes. */ which looks like it wasn't made with a great deal of thought. Now it seems to me that the comment was already bogus when written: there isn't anything uncertain about what will happen if you call either of these functions on a "new" page. PageIsNew checks for return ((PageHeader) page)->pd_upper == 0; If pd_upper is 0, PageGet[Exact]FreeSpace is absolutely guaranteed to return zero, even if pd_lower contains garbage. And then Indeed. I failed to notice that LocationIndex was an unsigned int, so I thought that pg_upper - pd_upper could be positive with garbage in pg_upper. PageGetHeapFreeSpace will likewise return zero. Perhaps there could be trouble if we got into the line-pointer-checking part of PageGetHeapFreeSpace, but we can't. So this comment is wrong, and is even more obviously wrong after the above change. I thought for a moment about removing the PageIsNew test altogether, but then I decided that it probably*is* what we want and is just mis-explained. I think the comment should read more like /* * PageGetExactFreeSpace() will return zero for a "new" page, * but it's actually usable free space, so count it that way. */ Now alternatively you could argue that a "new" page isn't usable free space yet and so we should count it as zero, just as we don't count dead tuples as usable free space. You need VACUUM to turn either of those things into real free space. But that'd be a bigger definitional change, and I'm not sure we want it. Thoughts? Also, do we need any documentation change for this? I looked through https://www.postgresql.org/docs/devel/pgstattuple.html and didn't see anything that was being very specific about what "free space" means, so maybe it's fine as-is. It's not easy. Maybe something like this? "For any initialized page, free space refers to anything that isn't page metadata (header and special), a line pointer or a tuple pointed to by a valid line pointer. In particular, a dead tuple is not free space because there's still a valid line pointer pointer pointing to it, until VACUUM or some other maintenance mechanism (e.g. page pruning) cleans up the page. A dead line pointer is not free space either, but the tuple it points to has become free space. An unused line pointer could be considered free space, but pgstattuple doesn't take it into account."
Re: pgstattuple: fix free space calculation
On 9/7/24 22:45, Tom Lane wrote:
I wrote:
Now alternatively you could argue that a "new" page isn't usable free
space yet and so we should count it as zero, just as we don't count
dead tuples as usable free space. You need VACUUM to turn either of
those things into real free space. But that'd be a bigger definitional
change, and I'm not sure we want it. Thoughts?
On the third hand: the code in question is in statapprox_heap, which
is presumably meant to deliver numbers comparable to pgstat_heap.
And pgstat_heap takes no special care for "new" pages, it just applies
PageGetHeapFreeSpace (or PageGetExactFreeSpace after this patch).
So that leaves me feeling pretty strongly that this whole stanza
is wrong and we should just do PageGetExactFreeSpace here.
+1
v4 patch attached.
Best regards,
Frédéric
From a59c16d33ff37ce5c9d0e663809be190c608c75b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20Yhuel?=
Date: Wed, 21 Aug 2024 15:05:11 +0200
Subject: [PATCH v4] pgstattuple: use PageGetExactFreeSpace()
instead of PageGetHeapFreeSpace() or PageGetFreeSpace()
We want the exact free space, we don't care if there's enough room for
another line pointer.
Also, statapprox_heap() shouldn't have to take any special care with new
pages.
---
contrib/pgstattuple/pgstatapprox.c | 9 +
contrib/pgstattuple/pgstatindex.c | 2 +-
contrib/pgstattuple/pgstattuple.c | 6 +++---
3 files changed, 5 insertions(+), 12 deletions(-)
diff --git a/contrib/pgstattuple/pgstatapprox.c b/contrib/pgstattuple/pgstatapprox.c
index c84c642355..04457f4b79 100644
--- a/contrib/pgstattuple/pgstatapprox.c
+++ b/contrib/pgstattuple/pgstatapprox.c
@@ -106,14 +106,7 @@ statapprox_heap(Relation rel, output_type *stat)
page = BufferGetPage(buf);
- /*
- * It's not safe to call PageGetHeapFreeSpace() on new pages, so we
- * treat them as being free space for our purposes.
- */
- if (!PageIsNew(page))
- stat->free_space += PageGetHeapFreeSpace(page);
- else
- stat->free_space += BLCKSZ - SizeOfPageHeaderData;
+ stat->free_space += PageGetExactFreeSpace(page);
/* We may count the page as scanned even if it's new/empty */
scanned++;
diff --git a/contrib/pgstattuple/pgstatindex.c b/contrib/pgstattuple/pgstatindex.c
index 5c06ba6db4..1b6b768cf8 100644
--- a/contrib/pgstattuple/pgstatindex.c
+++ b/contrib/pgstattuple/pgstatindex.c
@@ -311,7 +311,7 @@ pgstatindex_impl(Relation rel, FunctionCallInfo fcinfo)
max_avail = BLCKSZ - (BLCKSZ - ((PageHeader) page)->pd_special + SizeOfPageHeaderData);
indexStat.max_avail += max_avail;
- indexStat.free_space += PageGetFreeSpace(page);
+ indexStat.free_space += PageGetExactFreeSpace(page);
indexStat.leaf_pages++;
diff --git a/contrib/pgstattuple/pgstattuple.c b/contrib/pgstattuple/pgstattuple.c
index 3bd8b96197..7e2a7262a3 100644
--- a/contrib/pgstattuple/pgstattuple.c
+++ b/contrib/pgstattuple/pgstattuple.c
@@ -372,7 +372,7 @@ pgstat_heap(Relation rel, FunctionCallInfo fcinfo)
buffer = ReadBufferExtended(rel, MAIN_FORKNUM, block,
RBM_NORMAL, hscan->rs_strategy);
LockBuffer(buffer, BUFFER_LOCK_SHARE);
- stat.free_space += PageGetHeapFreeSpace((Page) BufferGetPage(buffer));
+ stat.free_space += PageGetExactFreeSpace((Page) BufferGetPage(buffer));
UnlockReleaseBuffer(buffer);
block++;
}
@@ -385,7 +385,7 @@ pgstat_heap(Relation rel, FunctionCallInfo fcinfo)
buffer = ReadBufferExtended(rel, MAIN_FORKNUM, block,
RBM_NORMAL, hscan->rs_strategy);
LockBuffer(buffer, BUFFER_LOCK_SHARE);
- stat.free_space += PageGetHeapFreeSpace((Page) BufferGetPage(buffer));
+ stat.free_space += PageGetExactFreeSpace((Page) BufferGetPage(buffer));
UnlockReleaseBuffer(buffer);
block++;
}
@@ -565,7 +565,7 @@ pgstat_index_page(pgstattuple_type *stat, Page page,
{
OffsetNumber i;
- stat->free_space += PageGetFreeSpace(page);
+ stat->free_space += PageGetExactFreeSpace(page);
for (i = minoff; i <= maxoff; i = OffsetNumberNext(i))
{
--
2.39.2
Re: Add has_large_object_privilege function
On 2024/07/02 16:34, Yugo NAGATA wrote:
So, I would like to propose to add
has_large_object_function for checking if a user has the privilege on a large
object.
+1
BTW since we already have pg_largeobject, using has_largeobject_privilege might
offer better consistency. However, I'm okay with the current name for now.
Even after committing the patch, we can rename it if others prefer
has_largeobject_privilege.
I am not sure why these
duplicated codes have been left for long time, and there might be some reasons.
However, otherwise, I think this deduplication also could reduce possible
maintenance cost in future.
I couldn't find the discussion that mentioned that reason either,
but I agree with simplifying the code.
As for 0001.patch, should we also remove the inclusion of "access/genam.h" and
"access/htup_details.h" since they're no longer needed?
0002 adds has_large_object_privilege function.There are three variations whose
arguments are combinations of large object OID with user name, user OID, or
implicit user (current_user).
As for 0002.patch, as the code in these three functions is mostly the same,
it might be more efficient to create a common internal function and have
the three functions call it for simplicity.
Here are other review comments for 0002.patch.
+
+
+
+ has_large_object_privilege
In the documentation, access privilege inquiry functions are listed
alphabetically. So, this function's description should come right
after has_language_privilege.
+ * has_large_objec_privilege variants
Typo: s/objec/object
+ * The result is a boolean value: true if user has been granted
+ * the indicated privilege or false if not.
The comment should clarify that NULL is returned if the specified
large object doesn’t exist. For example,
--
The result is a boolean value: true if user has the indicated
privilege, false if not, or NULL if object doesn't exist.
--
+convert_large_object_priv_string(text *priv_text)
It would be better to use "priv_type_text" instead of "priv_text"
for consistency with similar functions.
+ static const priv_map parameter_priv_map[] = {
+ {"SELECT", ACL_SELECT},
+ {"UPDATE", ACL_UPDATE},
parameter_priv_map should be large_object_priv_map.
Additionally, the entries for "WITH GRANT OPTION" should be included here.
+-- not-existing user
+SELECT has_large_object_privilege(-9, 1001, 'SELECT'); -- false
+ has_large_object_privilege
+
+ t
+(1 row)
The comment states the result should be false, but the actual result is true.
One of them seems incorrect.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
Re: access numeric data in module
Ed Behn writes: > I want to be able to have a function received and/or return numeric data. > However, I'm having trouble getting data from Datums and building Datums to > return. numeric.h does not contain the macros to do this. They are in > numeric.c. > Is there a way to access the values in the numeric structures? If not, > would a PR to move macros to numeric.h be welcome in the next commitfest? It's intentional that that stuff is not exposed, so no. What actual functionality do you need that numeric.h doesn't expose? regards, tom lane
Re: Change COPY ... ON_ERROR ignore to ON_ERROR ignore_row
Hi there On 26.08.24 02:00, jian he wrote: > hi all. > patch updated. > simplified the code a lot. > > idea is same: > COPY t_on_error_null FROM STDIN WITH (on_error set_to_null); > > If the STDIN number of columns is the same as the target table, then > InputFunctionCallSafe > call failure will make that column values be null. > > > If the STDIN number of columns is not the same as the target table, then error > ERROR: missing data for column \"%s\" > ERROR: extra data after last expected column > which is status quo. I wanted to give it another try, but the patch does not apply ... $ git apply ~/patches/copy_on_error/v3-0001-on_error-set_to_null.patch -v Checking patch doc/src/sgml/ref/copy.sgml... Checking patch src/backend/commands/copy.c... Checking patch src/backend/commands/copyfrom.c... Checking patch src/backend/commands/copyfromparse.c... Checking patch src/include/commands/copy.h... Checking patch src/test/regress/expected/copy2.out... error: while searching for: NOTICE: skipping row due to data type incompatibility at line 8 for column k: "a" CONTEXT: COPY check_ign_err NOTICE: 6 rows were skipped due to data type incompatibility -- tests for on_error option with log_verbosity and null constraint via domain CREATE DOMAIN dcheck_ign_err2 varchar(15) NOT NULL; CREATE TABLE check_ign_err2 (n int, m int[], k int, l dcheck_ign_err2); error: patch failed: src/test/regress/expected/copy2.out:753 error: src/test/regress/expected/copy2.out: patch does not apply Checking patch src/test/regress/sql/copy2.sql... -- Jim
Remove old RULE privilege completely
Hi, In v8.2, the RULE privilege for tables was removed, but for backward compatibility, GRANT/REVOKE RULE, has_table_privilege(..., 'RULE') etc are still accepted, though they don't perform any actions. Do we still need to maintain this backward compatibility? Could we consider removing the RULE privilege entirely? Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: Retire support for OpenSSL 1.1.1 due to raised API requirements
Daniel Gustafsson writes: > The patchset in https://commitfest.postgresql.org/49/5025/ which adds support > for configuring cipher suites in TLS 1.3 handshakes require an API available > in > OpenSSL 1.1.1 and onwards. With that as motivation I'd like to propose that > we > remove support for OpenSSL 1.1.0 and set the minimum required version to > 1.1.1. > OpenSSL 1.1.0 was EOL in September 2019 and was never an LTS version, so it's > not packaged in anything anymore AFAICT and should be very rare in production > use in conjunction with an updated postgres. 1.1.1 LTS will be 2 years EOL by > the time v18 ships so I doubt this will be all that controversial. Yeah ... the alternative would be to conditionally compile the new functionality. That doesn't seem like a productive use of developer time if it's supporting just one version that should be extinct in the wild by now. regards, tom lane
Re: query ID goes missing with extended query protocol
On Wed, Aug 28, 2024 at 6:48 PM Michael Paquier wrote: > This is being discussed already on a different thread: > - Thread: > https://www.postgresql.org/message-id/ca+427g8diw3az6popvgkpbqk97oubdf18vlihfesea2juk3...@mail.gmail.com > - CF entry: https://commitfest.postgresql.org/49/4963/ > > There is a patch proposed there, that attempts to deal with the two > cases of a portal where ExecutorRun() is called once while holding a > tuplestore and where ExecutorRun() is called multiple times. A few > approaches have been discussed for the tests, where the new psql > metacommands added in d55322b0da60 should become handy. That's on my > TODO spreadsheet of things to look at, but I did not come back to it > yet. That's interesting, but it sort of seems like it's reinventing the wheel, vs. the one-line change that I proposed. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Use read streams in pg_visibility
Hi,
On Thu, 5 Sept 2024 at 18:54, Noah Misch wrote:
>
> On Thu, Sep 05, 2024 at 03:59:53PM +0300, Nazir Bilal Yavuz wrote:
> > On Wed, 4 Sept 2024 at 21:43, Noah Misch wrote:
> > > https://postgr.es/m/caeudqaozv3wty5tv2t29jcwpydbmkbiwqkzd42s2ogzdixp...@mail.gmail.com
> > > then observed that collect_corrupt_items() was now guaranteed to never
> > > detect
> > > corruption. I have pushed revert ddfc556 of the pg_visibility.c changes.
> > > For
> > > the next try, could you add that testing we discussed?
> >
> > Do you think that corrupting the visibility map and then seeing if
> > pg_check_visible() and pg_check_frozen() report something is enough?
>
> I think so. Please check that it would have caught both the blkno bug and the
> above bug.
The test and updated patch files are attached. In that test I
overwrite the visibility map file with its older state. Something like
this:
1- Create the table, then run VACUUM FREEZE on the table.
2- Shutdown the server, create a copy of the vm file, restart the server.
3- Run the DELETE command on some $random_tuples.
4- Shutdown the server, overwrite the vm file with the #2 vm file,
restart the server.
5- pg_check_visible and pg_check_frozen must report $random_tuples as corrupted.
Do you think this test makes sense and enough?
--
Regards,
Nazir Bilal Yavuz
Microsoft
From 1f7b2cfe0d2247eb4768ce79d14982701437d473 Mon Sep 17 00:00:00 2001
From: Nazir Bilal Yavuz
Date: Mon, 9 Sep 2024 15:27:53 +0300
Subject: [PATCH v6 1/2] Add tests that pg_check_[visible|frozen] report
corrupted tuples
Currently, there are no tests that pg_check_[visible|frozen] need to
report corrupted tuples. Add that kind of tests.
To add these tests, visibility map needs to be corrupted. It is done by
overwriting visibility map by its older state.
---
contrib/pg_visibility/meson.build | 1 +
contrib/pg_visibility/t/002_corrupt_vm.pl | 100 ++
2 files changed, 101 insertions(+)
create mode 100644 contrib/pg_visibility/t/002_corrupt_vm.pl
diff --git a/contrib/pg_visibility/meson.build b/contrib/pg_visibility/meson.build
index f3c1263313a..609fc5f9f3e 100644
--- a/contrib/pg_visibility/meson.build
+++ b/contrib/pg_visibility/meson.build
@@ -36,6 +36,7 @@ tests += {
'tap': {
'tests': [
't/001_concurrent_transaction.pl',
+ 't/002_corrupt_vm.pl',
],
},
}
diff --git a/contrib/pg_visibility/t/002_corrupt_vm.pl b/contrib/pg_visibility/t/002_corrupt_vm.pl
new file mode 100644
index 000..cc6d043b8b7
--- /dev/null
+++ b/contrib/pg_visibility/t/002_corrupt_vm.pl
@@ -0,0 +1,100 @@
+# Copyright (c) 2021-2024, PostgreSQL Global Development Group
+
+# Check that pg_check_visible() and pg_check_frozen() reports
+# correct TIDs when there is a corruption.
+use strict;
+use warnings FATAL => 'all';
+use File::Copy;
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+
+my $node = PostgreSQL::Test::Cluster->new('main');
+$node->init;
+$node->start;
+
+my $blck_size = $node->safe_psql("postgres", "SHOW block_size;");
+
+# Create a sample table with at least 10 pages and then run VACUUM. 10 is
+# selected manually as it is big enough to select 5 random tuples from the
+# relation.
+$node->safe_psql(
+ 'postgres', qq(
+ CREATE EXTENSION pg_visibility;
+ CREATE TABLE corruption_test
+WITH (autovacuum_enabled = false) AS
+ SELECT
+i,
+repeat('a', 10) AS data
+ FROM
+ generate_series(1, $blck_size) i;
+ VACUUM (FREEZE, DISABLE_PAGE_SKIPPING) corruption_test;
+)
+);
+
+# VACUUM is run, it is safe to get the number of pages from the relpages.
+my $npages = $node->safe_psql(
+ "postgres",
+ "SELECT relpages FROM pg_class
+ WHERE relname = 'corruption_test';"
+);
+
+ok($npages >= 10, "There should be at least 10 pages in the table");
+
+my $file = $node->safe_psql("postgres",
+ "SELECT pg_relation_filepath('corruption_test');");
+
+# Copy visibility map.
+$node->stop;
+my $vm_file = $node->data_dir . '/' . $file . '_vm';
+copy("$vm_file", "${vm_file}_temp");
+$node->start;
+
+# Select 5 random tuples from the relation.
+my $tuples = $node->safe_psql(
+ "postgres",
+ "SELECT ctid FROM (
+SELECT ctid FROM corruption_test
+ORDER BY random() LIMIT 5)
+ORDER BY ctid ASC;"
+);
+
+# Do the changes below to use tuples in the query.
+# "\n" -> ","
+# "(" -> "'("
+# ")" -> ")'"
+(my $tuples_query = $tuples) =~ s/\n/,/g;
+$tuples_query =~ s/\(/\'\(/g;
+$tuples_query =~ s/\)/\)\'/g;
+
+$node->safe_psql(
+ "postgres",
+ "DELETE FROM corruption_test
+ WHERE ctid in ($tuples_query);"
+);
+
+# Overwrite visibility map with the old one.
+$node->stop;
+move("${vm_file}_temp", "$vm_file");
+$node->start;
+
+my $result = $node->safe_psql(
+ "postgres",
+ "SELECT DISTINCT t_ctid
+ FROM pg_check_visible('corruption_test')
+ ORDER BY t_ctid ASC;"
+);
+
+is($result, $tuples, 'pg_check_visible must report tuples as corrupted');
+
+$result = $node->safe_psql(
+
Re: On disable_cost
On Mon, Sep 9, 2024 at 12:09 AM Richard Guo wrote: > Fixed. Thanks to Alexander for the very good catch and to Richard for pushing the fix. (I started to respond to this last week but didn't quite get to it before I ran out of time/energy.) -- Robert Haas EDB: http://www.enterprisedb.com
[PATCH] Fix small overread during SASLprep
Hi all, pg_utf8_string_len() doesn't check the remaining string length before calling pg_utf8_is_legal(), so there's a possibility of jumping a couple of bytes past the end of the string. (The overread stops there, because the function won't validate a sequence containing a null byte.) Here's a quick patch to fix it. I didn't see any other uses of pg_utf8_is_legal() with missing length checks. Thanks, --Jacob pg_utf8_string_len-honor-null-terminators.patch Description: Binary data
Re: Use streaming read API in ANALYZE
Hi, On Thu, Sep 05, 2024 at 09:12:07PM +1200, Thomas Munro wrote: > On Thu, Sep 5, 2024 at 6:45 PM Mats Kindahl wrote: > > Making the ReadStream API non-opaque (that is, moving the definition > > to the header file) would at least solve our problem (unless I am > > mistaken). However, I am ignorant about long-term plans which might > > affect this, so there might be a good reason to revert it for > > reasons I am not aware of. > > The second thing. I am a bit confused about the status of this thread. Robert mentioned RC1, so I guess it pertains to v17 but I don't see it on the open item wiki list? Does the above mean you are going to revert it for v17, Thomas? And if so, what exactly? The ANALYZE changes on top of the streaming read API or something else about that API that is being discussed on this thread? I am also asking because this feature (i.e. Use streaming read API in ANALYZE) is being mentioned in the release announcement and that was just frozen for translations. Michael
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
On Mon, Aug 26, 2024 at 8:40 AM Robert Haas wrote: > On Fri, Aug 23, 2024 at 3:40 PM Jacob Champion > wrote: > > Agreed on the name. I've attached a reconfigured version of v15-0003, > > with an extension that should hopefully not throw off the cfbot, and a > > commit message that should hopefully not misrepresent the discussion > > so far? > > LGTM. Objections? Hearing none, committed. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Make query cancellation keys longer
On Fri, Aug 16, 2024 at 11:29 AM Robert Haas wrote: > > I'll split this patch like that, to make it easier to compare and merge > > with Jelte's corresponding patches. > > That sounds great. IMHO, comparing and merging the patches is the next > step here and would be great to see. Heikki, do you have any update on this work? -- Robert Haas EDB: http://www.enterprisedb.com
Re: Remove old RULE privilege completely
On Mon, Sep 9, 2024 at 10:37 AM Fujii Masao wrote: > In v8.2, the RULE privilege for tables was removed, but for backward > compatibility, > GRANT/REVOKE RULE, has_table_privilege(..., 'RULE') etc are still accepted, > though they don't perform any actions. > > Do we still need to maintain this backward compatibility? > Could we consider removing the RULE privilege entirely? 8.2 is a long time ago. If it's really been dead since then, I think we should remove it. -- Robert Haas EDB: http://www.enterprisedb.com
Re: SPI_connect, SPI_connect_ext return type
I wrote: > I too think it's good to go. If no one complains or asks for > more time to review, I will push it Monday or so. And done. regards, tom lane
Re: SPI_connect, SPI_connect_ext return type
On Mon, Sep 9, 2024 at 12:20 PM Tom Lane wrote: > I wrote: > > I too think it's good to go. If no one complains or asks for > > more time to review, I will push it Monday or so. > > And done. I didn't see this thread until after the commit had already happened, but a belated +1 for this and any other cruft removal we can do in SPI-land. -- Robert Haas EDB: http://www.enterprisedb.com
Re: access numeric data in module
On Mon, Sep 9, 2024 at 10:14 AM Tom Lane wrote: > Ed Behn writes: > > I want to be able to have a function received and/or return numeric data. > > However, I'm having trouble getting data from Datums and building Datums to > > return. numeric.h does not contain the macros to do this. They are in > > numeric.c. > > > Is there a way to access the values in the numeric structures? If not, > > would a PR to move macros to numeric.h be welcome in the next commitfest? > > It's intentional that that stuff is not exposed, so no. > > What actual functionality do you need that numeric.h doesn't expose? I don't agree with this reponse at all. It seems entirely reasonable for third-party code to want to have a way to construct and interpret numeric datums. Keeping the details private would MAYBE make sense if the internal details were changing release to release, but that's clearly not the case. Even if it were, an extension author is completely entitled to say "hey, I'd rather have access to an unstable API and update my code for new releases" and we should accommodate that. If we don't, people don't give up on writing the code that they want to write -- they just cut-and-paste private declarations/code into their own source tree, which is WAY worse than if we just put the stuff in a .h file. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Jargon and acronyms on this mailing list
On 2024-09-08 Su 11:35 PM, Tom Lane wrote: David Rowley writes: I think HEAD is commonly misused to mean master instead of the latest commit of the current branch. I see the buildfarm even does that. Thanks for getting that right in your blog post. IIRC, HEAD *was* the technically correct term back when we were using CVS. Old habits die hard. Yeah. The reason we kept doing it that way in the buildfarm was that for a period we actually had some animals using CVS and some using that new-fangled git thing. I guess I could try to write code to migrate everything, but it would be somewhat fragile. And what would we do if we ever decided to migrate "master" to another name like "main"? I do at least have code ready for that eventuality, but it would (currently) still keep the visible name of HEAD. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: Jargon and acronyms on this mailing list
On Mon, Sep 9, 2024 at 1:03 PM Andrew Dunstan wrote: > I guess I could try to write code to migrate everything, but it would be > somewhat fragile. And what would we do if we ever decided to migrate > "master" to another name like "main"? I do at least have code ready for > that eventuality, but it would (currently) still keep the visible name > of HEAD. Personally, I think using HEAD to mean master is really confusing. In git, master is a branch name, and HEAD is the tip of some branch, or the random commit you've checked out that isn't even a branch. I know that's not how it worked in CVS, but CVS was a very long time ago. If we rename master to main or devel or something, we'll have to adjust the way we speak again, but that's not a reason to keep using the wrong terminology for the way things are now. -- Robert Haas EDB: http://www.enterprisedb.com
Re: access numeric data in module
Robert Haas writes: > On Mon, Sep 9, 2024 at 10:14 AM Tom Lane wrote: >> It's intentional that that stuff is not exposed, so no. >> What actual functionality do you need that numeric.h doesn't expose? > I don't agree with this reponse at all. It seems entirely reasonable > for third-party code to want to have a way to construct and interpret > numeric datums. Keeping the details private would MAYBE make sense if > the internal details were changing release to release, but that's > clearly not the case. We have changed numeric's internal representation in the past, and I'd like to keep the freedom to do so again. There's been discussion for example of reconsidering the choice of NBASE to make more sense on 64-bit hardware. Yeah, maintaining on-disk compatibility limits what we can do there, but not as much as if some external module is in bed with the representation. > Even if it were, an extension author is > completely entitled to say "hey, I'd rather have access to an unstable > API and update my code for new releases" and we should accommodate > that. If we don't, people don't give up on writing the code that they > want to write -- they just cut-and-paste private declarations/code > into their own source tree, which is WAY worse than if we just put the > stuff in a .h file. IMO it'd be a lot better if numeric.c exposed whatever functionality Ed feels is missing, while keeping the contents of a numeric opaque. regards, tom lane
Re: Remove old RULE privilege completely
On 2024/09/10 1:02, Robert Haas wrote:
On Mon, Sep 9, 2024 at 10:37 AM Fujii Masao wrote:
In v8.2, the RULE privilege for tables was removed, but for backward
compatibility,
GRANT/REVOKE RULE, has_table_privilege(..., 'RULE') etc are still accepted,
though they don't perform any actions.
Do we still need to maintain this backward compatibility?
Could we consider removing the RULE privilege entirely?
8.2 is a long time ago. If it's really been dead since then, I think
we should remove it.
Ok, so, patch attached.
There was a test to check if has_table_privilege() accepted the keyword RULE.
The patch removed it since it's now unnecessary and would only waste cycles
testing that has_table_privilege() no longer accepts the keyword.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
From 8e16ba2cfc988031d27a1a5ccbc71169e1956933 Mon Sep 17 00:00:00 2001
From: Fujii Masao
Date: Mon, 9 Sep 2024 23:33:55 +0900
Subject: [PATCH v1] Remove old RULE privilege completely.
The RULE privilege for tables was removed in v8.2, but for backward
compatibility, GRANT/REVOKE and privilege functions like
has_table_privilege continued to accept the RULE keyword without any effect.
After discussions on pgsql-hackers, it was agreed that this compatibility
is no longer needed. Since it's been long enough since the deprecation,
we've decided to fully remove support for RULE privilege,
so GRANT/REVOKE and privilege functions will no longer accept it.
---
src/backend/catalog/aclchk.c | 2 --
src/backend/utils/adt/acl.c | 6 --
src/test/regress/expected/privileges.out | 9 -
src/test/regress/sql/privileges.sql | 4
4 files changed, 21 deletions(-)
diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
index a44ccee3b6..d2abc48fd8 100644
--- a/src/backend/catalog/aclchk.c
+++ b/src/backend/catalog/aclchk.c
@@ -2641,8 +2641,6 @@ string_to_privilege(const char *privname)
return ACL_ALTER_SYSTEM;
if (strcmp(privname, "maintain") == 0)
return ACL_MAINTAIN;
- if (strcmp(privname, "rule") == 0)
- return 0; /* ignore old RULE
privileges */
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
errmsg("unrecognized privilege type \"%s\"",
privname)));
diff --git a/src/backend/utils/adt/acl.c b/src/backend/utils/adt/acl.c
index d7b39140b3..4ad222b8d1 100644
--- a/src/backend/utils/adt/acl.c
+++ b/src/backend/utils/adt/acl.c
@@ -341,9 +341,6 @@ aclparse(const char *s, AclItem *aip, Node *escontext)
case ACL_MAINTAIN_CHR:
read = ACL_MAINTAIN;
break;
- case 'R': /* ignore old RULE
privileges */
- read = 0;
- break;
default:
ereturn(escontext, NULL,
(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
@@ -1639,7 +1636,6 @@ makeaclitem(PG_FUNCTION_ARGS)
{"SET", ACL_SET},
{"ALTER SYSTEM", ACL_ALTER_SYSTEM},
{"MAINTAIN", ACL_MAINTAIN},
- {"RULE", 0},/* ignore old RULE privileges */
{NULL, 0}
};
@@ -2063,8 +2059,6 @@ convert_table_priv_string(text *priv_type_text)
{"TRIGGER WITH GRANT OPTION",
ACL_GRANT_OPTION_FOR(ACL_TRIGGER)},
{"MAINTAIN", ACL_MAINTAIN},
{"MAINTAIN WITH GRANT OPTION",
ACL_GRANT_OPTION_FOR(ACL_MAINTAIN)},
- {"RULE", 0},/* ignore old RULE privileges */
- {"RULE WITH GRANT OPTION", 0},
{NULL, 0}
};
diff --git a/src/test/regress/expected/privileges.out
b/src/test/regress/expected/privileges.out
index fab0cc800f..430f097114 100644
--- a/src/test/regress/expected/privileges.out
+++ b/src/test/regress/expected/privileges.out
@@ -1422,15 +1422,6 @@ from (select oid from pg_roles where rolname =
current_user) as t2;
t
(1 row)
--- 'rule' privilege no longer exists, but for backwards compatibility
--- has_table_privilege still recognizes the keyword and says FALSE
-select has_table_privilege(current_user,t1.oid,'rule')
-from (select oid from pg_class where relname = 'pg_authid') as t1;
- has_table_privilege
--
- f
-(1 row)
-
select has_table_privilege(current_user,t1.oid,'references')
from (select oid from pg_class where relname = 'pg_authid') as t1;
has_table_privilege
diff --git a/src/test/regress/sql/privileges.sql
b/src/test/regress/sql/privileges.sql
index ae338e8cc8..e55b32f9d4 100644
--- a/src/test/regress/sql/privileges.sql
+++ b/src/test/regress/sql/privileges.sql
@@ -1004,10 +1004,6 @@ from (select oid
Re: Remove hardcoded hash opclass function signature exceptions
Peter Eisentraut writes: > On 06.09.24 21:43, Tom Lane wrote: >> * I don't really like the new control structure, or rather lack of >> structure, in hashvalidate. In particular the uncommented >> s/break/continue/ changes look like typos. They aren't, but can't >> you do this in a less confusing fashion? Or at least add comments >> like "continue not break because the code below the switch doesn't >> apply to this case". > Ok, I cleaned that up a bit. That looks nicer. Thanks. >> I wish we could get rid of those, but according to >> codesearch.debian.net, postgis and a couple of other extensions are >> relying on them. If we remove them we'll break any convenient upgrade >> path for those extensions. > Those are using the C function, which is ok. I was thinking about > removing the SQL function (from pg_proc.dat), because you can't use that > for much anymore. (You can't call it directly, and the hash AM will no > longer accept it.) I have done that in this patch version and added > some code comments around it. No, it isn't okay. What postgis (and the others) is doing is equivalent to regression=# create function myhash(bytea) returns int as 'hashvarlena' LANGUAGE 'internal' IMMUTABLE STRICT PARALLEL SAFE; CREATE FUNCTION After applying the v2 patch, I get regression=# create function myhash(bytea) returns int as 'hashvarlena' LANGUAGE 'internal' IMMUTABLE STRICT PARALLEL SAFE; ERROR: there is no built-in function named "hashvarlena" The reason is that the fmgr_builtins table is built from pg_proc.dat, and only names appearing in it can be used as 'internal' function definitions. So you really can't remove the pg_proc entry. The other thing that's made from pg_proc.dat is the list of extern function declarations in fmgrprotos.h. That's why you had to add those cowboy declarations inside hashfunc.c, which are both ugly and not helpful for any external module that might wish to call those functions at the C level. Other than the business about removing those pg_proc entries, I think this is good to go. regards, tom lane
Re: access numeric data in module
On Mon, Sep 9, 2024 at 1:25 PM Tom Lane wrote: > We have changed numeric's internal representation in the past, and > I'd like to keep the freedom to do so again. There's been discussion > for example of reconsidering the choice of NBASE to make more sense > on 64-bit hardware. Yeah, maintaining on-disk compatibility limits > what we can do there, but not as much as if some external module > is in bed with the representation. I disagree with the idea that a contrib module looking at the details of a Numeric value means we can't make these kinds of updates. > > Even if it were, an extension author is > > completely entitled to say "hey, I'd rather have access to an unstable > > API and update my code for new releases" and we should accommodate > > that. If we don't, people don't give up on writing the code that they > > want to write -- they just cut-and-paste private declarations/code > > into their own source tree, which is WAY worse than if we just put the > > stuff in a .h file. > > IMO it'd be a lot better if numeric.c exposed whatever functionality > Ed feels is missing, while keeping the contents of a numeric opaque. We could certainly expose a bunch of functions, but I think that would actually be a bigger maintenance burden for us than just exposing some of the details that are currently private to numeric.c. It would also presumably be less performant, since it means somebody has to call a function rather than just using a macro. Also, this seems to me to be holding the numeric data type to a different standard than other things. For numeric, we have NumericData, NumericChoice, NumericShort, and NumericLong as structs that define the on-disk representation. They're in numeric.c. But ArrayType is in array.h. RangeType is in rangetypes.h. MultiRangeType is in multirangetypes.h. PATH and POLYGON are in geo_decls.h. inet and inet_data are in inet.h. int2vector and oidvector are in c.h (which seems like questionable placement, but I digress). And there must be tons of third-party code out there that knows how to interpret a text or bytea varlena. So it's not like we have some principled project-wide policy of hiding these implementation details. At first look, numeric seems like an outlier. -- Robert Haas EDB: http://www.enterprisedb.com
Re: access numeric data in module
Robert Haas writes: > On Mon, Sep 9, 2024 at 1:25 PM Tom Lane wrote: >> IMO it'd be a lot better if numeric.c exposed whatever functionality >> Ed feels is missing, while keeping the contents of a numeric opaque. > We could certainly expose a bunch of functions, but I think that would > actually be a bigger maintenance burden for us than just exposing some > of the details that are currently private to numeric.c. This whole argument is contingent on details that haven't been provided, namely exactly what it is that Ed wants to do that he can't do today. I think we should investigate that before deciding that publishing previously-private detail is the best solution. > Also, this seems to me to be holding the numeric data type to a > different standard than other things. By that argument, we should move every declaration in every .c file into c.h and be done. I'd personally be happier if we had *not* exposed the other data structure details you mention, but that ship has sailed. If we do do what you're advocating, I'd at least insist that the declarations go into a new file numeric_internal.h, so that it's clear to all concerned that they're playing with fire if they depend on that stuff. regards, tom lane
Re: access numeric data in module
On 09/09/24 13:00, Robert Haas wrote: > I don't agree with this reponse at all. It seems entirely reasonable > for third-party code to want to have a way to construct and interpret > numeric datums. Keeping the details private would MAYBE make sense if > the internal details were changing release to release, but that's > clearly not the case. Even if it were, an extension author is > completely entitled to say "hey, I'd rather have access to an unstable > API and update my code for new releases" and we should accommodate > that. If we don't, people don't give up on writing the code that they > want to write -- they just cut-and-paste private declarations/code > into their own source tree, which is WAY worse than if we just put the > stuff in a .h file. Amen. https://tada.github.io/pljava/preview1.7/pljava-api/apidocs/org.postgresql.pljava/org/postgresql/pljava/adt/Numeric.html The above API documentation was written when the PostgreSQL source comments read "values of NBASE other than 1 are considered of historical interest only and are no longer supported in any sense". I will have to generalize it a bit more if other NBASEs are now to be considered again. If Tom prefers the idea of keeping the datum layout strongly encapsulated (pretty much uniquely among PG data types) and providing only a callable C API for manipulating it, then I might propose something like the above- linked Java API as one source of API ideas. I think it's worth remembering that most PLs will have their own libraries (sometimes multiple alternatives) for arbitrary-precision numbers, and it's hard to generalize about /those/ libraries regarding what API they will provide for most efficiently and faithfully converting a foreign representation to or from their own. Conversion through a decimal string (a) may not be most efficient, and (b) may not faithfully roundtrip possible combinations of digits, displayScale, and weight. >From Java's perspective, there has historically been a significant JNI overhead for calling from Java into a C API, so that it's advantageous to know the memory layout and keep the processing in Java. There is at last a batteries-included Java foreign-function interface that can make it less costly to call into a C API, but that has only landed in Java 22, which not everyone will be using right away. Regards, -Chap
Re: [PATCH] Fix small overread during SASLprep
> On 9 Sep 2024, at 17:29, Jacob Champion > wrote: > pg_utf8_string_len() doesn't check the remaining string length before > calling pg_utf8_is_legal(), so there's a possibility of jumping a > couple of bytes past the end of the string. (The overread stops there, > because the function won't validate a sequence containing a null > byte.) > > Here's a quick patch to fix it. I didn't see any other uses of > pg_utf8_is_legal() with missing length checks. Just to make sure I understand, this is for guarding against overreads in validation of strings containing torn MB characters? Assuming I didn't misunderstand you this patch seems correct to me. -- Daniel Gustafsson
Re: pgstattuple: fix free space calculation
=?UTF-8?Q?Fr=C3=A9d=C3=A9ric_Yhuel?= writes: > v4 patch attached. LGTM, pushed. regards, tom lane
Disallow altering invalidated replication slots
Hi, ALTER_REPLICATION_SLOT on invalidated replication slots is unnecessary as there is no way to get the invalidated (logical) slot to work. Please find the patch to add an error in such cases. Relevant discussion is at [1]. Thoughts? [1] https://www.postgresql.org/message-id/CAA4eK1%2Bszcosq0nS109mMSxPWyNT1Q%3DUNYCJgXKYuCceaPS%2BhA%40mail.gmail.com -- Bharath Rupireddy PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com v1-0001-Disallow-altering-invalidated-replication-slots.patch Description: Binary data
Re: [PATCH] Fix small overread during SASLprep
On Mon, Sep 9, 2024 at 11:30 AM Daniel Gustafsson wrote: > Just to make sure I understand, this is for guarding against overreads in > validation of strings containing torn MB characters? Right. Our SASLprep code doesn't require/enforce UTF8-encoded inputs. > Assuming I didn't > misunderstand you this patch seems correct to me. Thanks for the review! --Jacob
Re: Introduce XID age and inactive timeout based replication slot invalidation
Hi, On Mon, Sep 9, 2024 at 3:04 PM Amit Kapila wrote: > > > > > > We should not allow the invalid replication slot to be altered > > > > > irrespective of the reason unless there is any benefit. > > > > > > > > Okay, then I think we need to change the existing behaviour of the > > > > other invalidation causes which still allow alter-slot. > > > > > > +1. Perhaps, track it in a separate thread? > > > > I think so. It does not come under the scope of this thread. > > It makes sense to me as well. But let's go ahead and get that sorted out > first. Moved the discussion to new thread - https://www.postgresql.org/message-id/CALj2ACW4fSOMiKjQ3%3D2NVBMTZRTG8Ujg6jsK9z3EvOtvA4vzKQ%40mail.gmail.com. Please have a look. -- Bharath Rupireddy PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Re: access numeric data in module
On Mon, Sep 9, 2024 at 2:02 PM Tom Lane wrote: > By that argument, we should move every declaration in every .c file > into c.h and be done. I'd personally be happier if we had *not* > exposed the other data structure details you mention, but that > ship has sailed. Not every declaration in every .c file is of general interest, but the ones that are probably should be moved into .h files. The on-disk representation of a commonly-used data type certainly qualifies. You can see from Chapman's reply that I'm not making this up: when we don't expose things, it doesn't keep people from depending on them, it just makes them copy our code into their own repository. That's not a win. It makes those extensions more fragile, not less, and it makes the PostgreSQL extension ecosystem worse. pg_hint_plan is another, recently-discussed example of this phenomenon: refuse to give people the keys, and they start hot-wiring stuff. > If we do do what you're advocating, I'd at least insist that the > declarations go into a new file numeric_internal.h, so that it's > clear to all concerned that they're playing with fire if they > depend on that stuff. I think that's a bit pointless considering that we don't do it in any of the other cases. I'd rather be consistent with our usual practice. But if it ends up in a separate header file that's still better than the status quo. -- Robert Haas EDB: http://www.enterprisedb.com
Re: optimizing pg_upgrade's once-in-each-database steps
On Thu, Sep 05, 2024 at 01:32:34PM +0200, Daniel Gustafsson wrote: > I've read and tested through the latest version of this patchset and I think > it's ready to go in. Thanks for reviewing. I'm aiming to commit it later this week. > The one concern I have is that tasks can exit(1) on libpq > errors tearing down perfectly functional connections without graceful > shutdown. > Longer term I think it would make sense to add similar exit handler callbacks > to the ones in pg_dump for graceful cleanup of connections. However, in order > to keep goalposts in clear view I don't think this patch need to have it, but > it would be good to consider once in. This did cross my mind. I haven't noticed any problems in my testing, and it looks like there are several existing places in pg_upgrade that call pg_fatal() with open connections, so I'm inclined to agree that this is a nice follow-up task that needn't hold up this patch set. > Spotted a small typo in the comments: > > + * nothing to process. This is primarily intended for the inital step > in > s/inital/initial/ Will fix. -- nathan
Re: Opinion poll: Sending an automated email to a thread when it gets added to the commitfest
So, there was limited enthusiasm for a new message here, but I noticed that current CF messages don't include the CF entry link [1]. What about adding a link to its existing e-mails? Thanks, Maciek [1]: e.g., https://www.postgresql.org/message-id/172579582925.1126.2395496302629349708.pgcf%40coridan.postgresql.org
Re: Remove old RULE privilege completely
On Tue, Sep 10, 2024 at 02:45:37AM +0900, Fujii Masao wrote: > On 2024/09/10 1:02, Robert Haas wrote: >> On Mon, Sep 9, 2024 at 10:37 AM Fujii Masao >> wrote: >> > In v8.2, the RULE privilege for tables was removed, but for backward >> > compatibility, >> > GRANT/REVOKE RULE, has_table_privilege(..., 'RULE') etc are still accepted, >> > though they don't perform any actions. >> > >> > Do we still need to maintain this backward compatibility? >> > Could we consider removing the RULE privilege entirely? >> >> 8.2 is a long time ago. If it's really been dead since then, I think >> we should remove it. +1. It seems more likely to cause confusion at this point. > Ok, so, patch attached. > > There was a test to check if has_table_privilege() accepted the keyword RULE. > The patch removed it since it's now unnecessary and would only waste cycles > testing that has_table_privilege() no longer accepts the keyword. LGTM -- nathan
Re: Jargon and acronyms on this mailing list
On 2024-09-09 Mo 1:19 PM, Robert Haas wrote: On Mon, Sep 9, 2024 at 1:03 PM Andrew Dunstan wrote: I guess I could try to write code to migrate everything, but it would be somewhat fragile. And what would we do if we ever decided to migrate "master" to another name like "main"? I do at least have code ready for that eventuality, but it would (currently) still keep the visible name of HEAD. Personally, I think using HEAD to mean master is really confusing. In git, master is a branch name, and HEAD is the tip of some branch, or the random commit you've checked out that isn't even a branch. I know that's not how it worked in CVS, but CVS was a very long time ago. If we rename master to main or devel or something, we'll have to adjust the way we speak again, but that's not a reason to keep using the wrong terminology for the way things are now. There are some serious obstacles to changing it all over, though. I don't want to rewrite all the history, for example. What we could do relatively simply is change what is seen publicly. e.g. we could rewrite the status page to read "Branch: master". We could also change URLs we generate to use master instead of HEAD (and change it back when processing the URLs. And so on. Changing things on the client side would be far more complicated and difficult. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: pg_verifybackup: TAR format backup verification
I would rather that you didn't add simple_ptr_list_destroy_deep()
given that you don't need it for this patch series.
+
"\"%s\" unexpected file in the tar format backup",
This doesn't seem grammatical to me. Perhaps change this to: file
\"%s\" is not expected in a tar format backup
+ /* We are only interested in files that are not in the ignore list. */
+ if (member->is_directory || member->is_link ||
+ should_ignore_relpath(mystreamer->context, member->pathname))
+ return;
Doesn't this need to happen after we add pg_tblspc/$OID to the path,
rather than before? I bet this doesn't work correctly for files in
user-defined tablespaces, compared to the way it work for a
directory-format backup.
+ chartemp[MAXPGPATH];
+
+ /* Copy original name at temporary space */
+ memcpy(temp, member->pathname, MAXPGPATH);
+
+ snprintf(member->pathname, MAXPGPATH, "%s/%d/%s",
+"pg_tblspc", mystreamer->tblspc_oid, temp);
I don't like this at all. This function doesn't have any business
modifying the astreamer_member, and it doesn't need to. It can just do
char *pathname; char tmppathbuf[MAXPGPATH] and then set pathname to
either member->pathname or tmppathbuf depending on
OidIsValid(tblspcoid). Also, shouldn't this be using %u, not %d?
+ mystreamer->mfile = (void *) m;
Either the cast to void * isn't necessary, or it indicates that
there's a type mismatch that should be fixed.
+* We could have these checks while receiving contents. However, since
+* contents are received in multiple iterations, this would result in
+* these lengthy checks being performed multiple times. Instead, setting
+* flags here and using them before proceeding with verification will be
+* more efficient.
Seems unnecessary to explain this.
+ Assert(mystreamer->verify_checksum);
+
+ /* Should have came for the right file */
+ Assert(strcmp(member->pathname, m->pathname) == 0);
+
+ /*
+* The checksum context should match the type noted in the backup
+* manifest.
+*/
+ Assert(checksum_ctx->type == m->checksum_type);
What do you think about:
Assert(m != NULL && !m->bad);
Assert(checksum_ctx->type == m->checksum_type);
Assert(strcmp(member->pathname, m->pathname) == 0);
Or possibly change the first one to Assert(should_verify_checksum(m))?
+ memcpy(&control_file, streamer->bbs_buffer.data,
sizeof(ControlFileData));
This probably doesn't really hurt anything, but it's a bit ugly. You
first use astreamer_buffer_until() to force the entire file into a
buffer. And then here, you copy the entire file into a second buffer
which is exactly the same except that it's guaranteed to be properly
aligned. It would be possible to include a ControlFileData in
astreamer_verify and copy the bytes directly into it (you'd need a
second astreamer_verify field for the number of bytes already copied
into that structure). I'm not 100% sure that's worth the code, but it
seems like it wouldn't take more than a few lines, so perhaps it is.
+/*
+ * Verify plain backup.
+ */
+static void
+verify_plain_backup(verifier_context *context)
+{
+ Assert(context->format == 'p');
+ verify_backup_directory(context, NULL, context->backup_directory);
+}
+
This seems like a waste of space.
+verify_tar_backup(verifier_context *context)
I like this a lot better now! I'm still not quite sure about the
decision to have the ignore list apply to both the backup directory
and the tar file contents -- but given the low participation on this
thread, I don't think we have much chance of getting feedback from
anyone else right now, so let's just do it the way you have it and we
can change it later if someone shows up to complain.
+verify_all_tar_files(verifier_context *context, SimplePtrList *tarfiles)
I think this code could be moved into its only caller instead of
having a separate function. And then if you do that, maybe
verify_one_tar_file could be renamed to just verify_tar_file. Or
perhaps that function could also be removed and just move the code
into the caller. It's not much code and not very deeply nested.
Similarly create_archive_verifier() could be considered for this
treatment. Maybe inlining all of these is too much and the result will
look messy, but I think we should at least try to combine some of
them.
...Robert
Re: Opinion poll: Sending an automated email to a thread when it gets added to the commitfest
On Mon, Aug 26, 2024 at 9:39 PM Thomas Munro wrote: > I've built a system for pushing all the data required to show the > cfbot traffic lights over to the CF app, and shared it with Magnus to > see if he likes it. If so, hopefully some more progress soon... I don't see any traffic lights on commitfest.postgresql.org yet. Should I #blamemagnus? -- Robert Haas EDB: http://www.enterprisedb.com
Re: Adding skip scan (including MDAM style range skip scan) to nbtree
On Sat, Sep 7, 2024 at 11:27 AM Tomas Vondra wrote: > I started looking at this patch today. Thanks for taking a look! > The first thing I usually do for > new patches is a stress test, so I did a simple script that generates > random table and runs a random query with IN() clause with various > configs (parallel query, index-only scans, ...). And it got stuck on a > parallel query pretty quick. I can reproduce this locally, without too much difficulty. Unfortunately, this is a bug on master/Postgres 17. Some kind of issue in my commit 5bf748b8. The timing of this is slightly unfortunate. There's only a few weeks until the release of 17, plus I have to travel for work over the next week. I won't be back until the 16th, and will have limited availability between then and now. I think that I'll have ample time to debug and fix the issue ahead of the release of 17, though. Looks like the problem is a parallel index scan with SAOP array keys can find itself in a state where every parallel worker waits for the leader to finish off a scheduled primitive index scan, while the leader itself waits for the scan's tuple queue to return more tuples. Obviously, the query will effectively go to sleep indefinitely when that happens (unless and until the DBA cancels the query). This is only possible with just the right/wrong combination of array keys and index cardinality. I cannot recreate the problem with parallel_leader_participation=off, which strongly suggests that leader participation is a factor. I'll find time to study this in detail as soon as I can. Further background: I was always aware of the leader's tendency to go away forever shortly after the scan begins. That was supposed to be safe, since we account for it by serializing the scan's current array keys in shared memory, at the point a primitive index scan is scheduled -- any backend should be able to pick up where any other backend left off, no matter how primitive scans are scheduled. That now doesn't seem to be completely robust, likely due to restrictions on when and how other backends can pick up the scheduled work from within _bt_first, at the point that it calls _bt_parallel_seize. In short, one or two details of how backends call _bt_parallel_seize to pick up BTPARALLEL_NEED_PRIMSCAN work likely need to be rethought. -- Peter Geoghegan
Re: Wrong security context for deferred triggers?
On Wed, 2024-03-06 at 14:32 +0100, Laurenz Albe wrote: > On Mon, 2023-11-06 at 18:29 +0100, Tomas Vondra wrote: > > On 11/6/23 14:23, Laurenz Albe wrote: > > > This behavior looks buggy to me. What do you think? > > > I cannot imagine that it is a security problem, though. > > > > How could code getting executed under the wrong role not be a security > > issue? Also, does this affect just the role, or are there some other > > settings that may unexpectedly change (e.g. search_path)? > > Here is a patch that fixes this problem by keeping track of the > current role in the AfterTriggerSharedData. Funny enough, this problem has just surfaced on pgsql-general: https://postgr.es/m/89e33a53-909c-6a02-bfc6-2578ba974...@cloud.gatewaynet.com I take this as one more vote for this patch... Yours, Laurenz Albe
Re: optimizing pg_upgrade's once-in-each-database steps
> On 9 Sep 2024, at 21:17, Nathan Bossart wrote: > > On Thu, Sep 05, 2024 at 01:32:34PM +0200, Daniel Gustafsson wrote: >> I've read and tested through the latest version of this patchset and I think >> it's ready to go in. > > Thanks for reviewing. I'm aiming to commit it later this week. +1. Looking forward to seeing what all the pg_dump/pg_upgrade changes amount to in speed improvement when combined. >> The one concern I have is that tasks can exit(1) on libpq >> errors tearing down perfectly functional connections without graceful >> shutdown. >> Longer term I think it would make sense to add similar exit handler callbacks >> to the ones in pg_dump for graceful cleanup of connections. However, in >> order >> to keep goalposts in clear view I don't think this patch need to have it, but >> it would be good to consider once in. > > This did cross my mind. I haven't noticed any problems in my testing, and > it looks like there are several existing places in pg_upgrade that call > pg_fatal() with open connections, so I'm inclined to agree that this is a > nice follow-up task that needn't hold up this patch set. It could perhaps be a good introductory task for a new contributor who want a fairly confined project to hack on. -- Daniel Gustafsson
Re: Pgoutput not capturing the generated columns
On Mon, Sep 9, 2024 at 2:38 AM Shubham Khanna wrote: > > On Thu, Aug 29, 2024 at 11:46 AM Amit Kapila wrote: > > > > On Thu, Aug 29, 2024 at 8:44 AM Masahiko Sawada > > wrote: > > > > > > On Wed, Aug 28, 2024 at 1:06 AM Amit Kapila > > > wrote: > > > > > > > > On Mon, May 20, 2024 at 1:49 PM Masahiko Sawada > > > > wrote: > > > > > > > > > > As Euler mentioned earlier, I think it's a decision not to replicate > > > > > generated columns because we don't know the target table on the > > > > > subscriber has the same expression and there could be locale issues > > > > > even if it looks the same. I can see that a benefit of this proposal > > > > > would be to save cost to compute generated column values if the user > > > > > wants the target table on the subscriber to have exactly the same data > > > > > as the publisher's one. Are there other benefits or use cases? > > > > > > > > > > > > > The cost is one but the other is the user may not want the data to be > > > > different based on volatile functions like timeofday() > > > > > > Shouldn't the generation expression be immutable? > > > > > > > Yes, I missed that point. > > > > > > or the table on > > > > subscriber won't have the column marked as generated. > > > > > > Yeah, it would be another use case. > > > > > > > Right, apart from that I am not aware of other use cases. If they > > have, I would request Euler or Rajendra to share any other use case. > > > > > > Now, considering > > > > such use cases, is providing a subscription-level option a good idea > > > > as the patch is doing? I understand that this can serve the purpose > > > > but it could also lead to having the same behavior for all the tables > > > > in all the publications for a subscription which may or may not be > > > > what the user expects. This could lead to some performance overhead > > > > (due to always sending generated columns for all the tables) for cases > > > > where the user needs it only for a subset of tables. > > > > > > Yeah, it's a downside and I think it's less flexible. For example, if > > > users want to send both tables with generated columns and tables > > > without generated columns, they would have to create at least two > > > subscriptions. > > > > > > > Agreed and that would consume more resources. > > > > > Also, they would have to include a different set of > > > tables to two publications. > > > > > > > > > > > I think we should consider it as a table-level option while defining > > > > publication in some way. A few ideas could be: (a) We ask users to > > > > explicitly mention the generated column in the columns list while > > > > defining publication. This has a drawback such that users need to > > > > specify the column list even when all columns need to be replicated. > > > > (b) We can have some new syntax to indicate the same like: CREATE > > > > PUBLICATION pub1 FOR TABLE t1 INCLUDE GENERATED COLS, t2, t3, t4 > > > > INCLUDE ..., t5;. I haven't analyzed the feasibility of this, so there > > > > could be some challenges but we can at least investigate it. > > > > > > I think we can create a publication for a single table, so what we can > > > do with this feature can be done also by the idea you described below. > > > > > > > Yet another idea is to keep this as a publication option > > > > (include_generated_columns or publish_generated_columns) similar to > > > > "publish_via_partition_root". Normally, "publish_via_partition_root" > > > > is used when tables on either side have different partitions > > > > hierarchies which is somewhat the case here. > > > > > > It sounds more useful to me. > > > > > > > Fair enough. Let's see if anyone else has any preference among the > > proposed methods or can think of a better way. > > I have fixed the current issue. I have added the option > 'publish_generated_columns' to the publisher side and created the new > test cases accordingly. > The attached patches contain the desired changes. > Thank you for updating the patches. I have some comments: Do we really need to add this option to test_decoding? I think it would be good if this improves the test coverage. Otherwise, I'm not sure we need this part. If we want to add it, I think it would be better to have it in a separate patch. --- + + If the publisher-side column is also a generated column then this option + has no effect; the publisher column will be filled as normal with the + publisher-side computed or default data. + I don't understand this description. Why does this option have no effect if the publisher-side column is a generated column? --- + + This parameter can only be set true if copy_data is + set to false. + If I understand this patch correctly, it doesn't disallow to set copy_data to true when the publish_generated_columns option is specified. But do we want to disallow it? I think it would be more useful and understandable if we allow to use both pu
Re: [PATCH] Fix small overread during SASLprep
> On 9 Sep 2024, at 20:41, Jacob Champion > wrote: > > On Mon, Sep 9, 2024 at 11:30 AM Daniel Gustafsson wrote: >> Just to make sure I understand, this is for guarding against overreads in >> validation of strings containing torn MB characters? > > Right. Our SASLprep code doesn't require/enforce UTF8-encoded inputs. Thanks for confirming, I'll have another look in the morning and will apply then unless there are objections. -- Daniel Gustafsson
Re: Retire support for OpenSSL 1.1.1 due to raised API requirements
> On 9 Sep 2024, at 16:48, Tom Lane wrote: > > Daniel Gustafsson writes: >> The patchset in https://commitfest.postgresql.org/49/5025/ which adds support >> for configuring cipher suites in TLS 1.3 handshakes require an API available >> in >> OpenSSL 1.1.1 and onwards. With that as motivation I'd like to propose that >> we >> remove support for OpenSSL 1.1.0 and set the minimum required version to >> 1.1.1. >> OpenSSL 1.1.0 was EOL in September 2019 and was never an LTS version, so it's >> not packaged in anything anymore AFAICT and should be very rare in production >> use in conjunction with an updated postgres. 1.1.1 LTS will be 2 years EOL >> by >> the time v18 ships so I doubt this will be all that controversial. > > Yeah ... the alternative would be to conditionally compile the new > functionality. That doesn't seem like a productive use of developer > time if it's supporting just one version that should be extinct in > the wild by now. Agreed. OpenSSL 1.1.1 is very different story and I suspect we'll be stuck on that level for some time, but 1.1.0 is gone from production use. -- Daniel Gustafsson
Re: Use read streams in pg_visibility
On Mon, Sep 09, 2024 at 06:25:07PM +0300, Nazir Bilal Yavuz wrote:
> On Thu, 5 Sept 2024 at 18:54, Noah Misch wrote:
> > On Thu, Sep 05, 2024 at 03:59:53PM +0300, Nazir Bilal Yavuz wrote:
> > > On Wed, 4 Sept 2024 at 21:43, Noah Misch wrote:
> > > > https://postgr.es/m/caeudqaozv3wty5tv2t29jcwpydbmkbiwqkzd42s2ogzdixp...@mail.gmail.com
> > > > then observed that collect_corrupt_items() was now guaranteed to never
> > > > detect
> > > > corruption. I have pushed revert ddfc556 of the pg_visibility.c
> > > > changes. For
> > > > the next try, could you add that testing we discussed?
> > >
> > > Do you think that corrupting the visibility map and then seeing if
> > > pg_check_visible() and pg_check_frozen() report something is enough?
> >
> > I think so. Please check that it would have caught both the blkno bug and
> > the
> > above bug.
>
> The test and updated patch files are attached. In that test I
> overwrite the visibility map file with its older state. Something like
> this:
>
> 1- Create the table, then run VACUUM FREEZE on the table.
> 2- Shutdown the server, create a copy of the vm file, restart the server.
> 3- Run the DELETE command on some $random_tuples.
> 4- Shutdown the server, overwrite the vm file with the #2 vm file,
> restart the server.
> 5- pg_check_visible and pg_check_frozen must report $random_tuples as
> corrupted.
Copying the vm file is a good technique. In my test runs, this does catch the
"never detect" bug, but it doesn't catch the blkno bug. Can you make it catch
both? It's possible my hand-patching to recreate the blkno bug is what went
wrong, so I'm attaching what I used. It consists of
v1-0002-Use-read-stream-in-pg_visibility-in-collect_corru.patch plus these
fixes for the "never detect" bug from your v6-0002:
--- a/contrib/pg_visibility/pg_visibility.c
+++ b/contrib/pg_visibility/pg_visibility.c
@@ -724,4 +724,4 @@ collect_corrupt_items(Oid relid, bool all_visible, bool
all_frozen)
{
- boolcheck_frozen = false;
- boolcheck_visible = false;
+ boolcheck_frozen = all_frozen;
+ boolcheck_visible = all_visible;
Buffer buffer;
@@ -757,5 +757,5 @@ collect_corrupt_items(Oid relid, bool all_visible, bool
all_frozen)
*/
- if (all_frozen && !VM_ALL_FROZEN(rel, blkno, &vmbuffer))
+ if (check_frozen && !VM_ALL_FROZEN(rel, blkno, &vmbuffer))
check_frozen = false;
- if (all_visible && !VM_ALL_VISIBLE(rel, blkno, &vmbuffer))
+ if (check_visible && !VM_ALL_VISIBLE(rel, blkno, &vmbuffer))
check_visible = false;
diff --git a/contrib/pg_visibility/pg_visibility.c
b/contrib/pg_visibility/pg_visibility.c
index 773ba92..ac575a1 100644
--- a/contrib/pg_visibility/pg_visibility.c
+++ b/contrib/pg_visibility/pg_visibility.c
@@ -41,6 +41,20 @@ typedef struct corrupt_items
ItemPointer tids;
} corrupt_items;
+/*
+ * Helper struct for read stream object used in
+ * collect_corrupt_items() function.
+ */
+struct collect_corrupt_items_read_stream_private
+{
+ boolall_frozen;
+ boolall_visible;
+ BlockNumber blocknum;
+ BlockNumber nblocks;
+ Relationrel;
+ Buffer *vmbuffer;
+};
+
PG_FUNCTION_INFO_V1(pg_visibility_map);
PG_FUNCTION_INFO_V1(pg_visibility_map_rel);
PG_FUNCTION_INFO_V1(pg_visibility);
@@ -611,6 +625,35 @@ GetStrictOldestNonRemovableTransactionId(Relation rel)
}
/*
+ * Callback function to get next block for read stream object used in
+ * collect_corrupt_items() function.
+ */
+static BlockNumber
+collect_corrupt_items_read_stream_next_block(ReadStream *stream,
+
void *callback_private_data,
+
void *per_buffer_data)
+{
+ struct collect_corrupt_items_read_stream_private *p =
callback_private_data;
+
+ for (; p->blocknum < p->nblocks; p->blocknum++)
+ {
+ boolcheck_frozen = false;
+ boolcheck_visible = false;
+
+ if (p->all_frozen && VM_ALL_FROZEN(p->rel, p->blocknum,
p->vmbuffer))
+ check_frozen = true;
+ if (p->all_visible && VM_ALL_VISIBLE(p->rel, p->blocknum,
p->vmbuffer))
+ check_visible = true;
+ if (!check_visible && !check_frozen)
+ continue;
+
+ return p->blocknum++;
+ }
+
+ return InvalidBlockNumber;
+}
+
+/*
* Returns a list of items whose visibility map information does not match
* the status of the tuples on the page.
*
@@ -634,6 +677,10 @@ collect_corrupt_items(Oid relid, bool all_visible, bool
all_frozen)
Buffer vmbuffer =
Proposal to Enable/Disable Index using ALTER INDEX
Hello hackers, This is my first time posting here, and I’d like to propose a new feature related to PostgreSQL indexes. If this idea resonates, I’d be happy to follow up with a patch as well. *Problem*: Adding and removing indexes is a common operation in PostgreSQL. On larger databases, however, these operations can be resource-intensive. When evaluating the performance impact of one or more indexes, dropping them might not be ideal since as a user you may want a quicker way to test their effects without fully committing to removing & adding them back again. Which can be a time taking operation on larger tables. *Proposal*: I propose adding an ALTER INDEX command that allows for enabling or disabling an index globally. This could look something like: ALTER INDEX index_name ENABLE; ALTER INDEX index_name DISABLE; A disabled index would still receive updates and enforce constraints as usual but would not be used for queries. This allows users to assess whether an index impacts query performance before deciding to drop it entirely. *Implementation*: To keep this simple, I suggest toggling the indisvalid flag in pg_index during the enable/disable operation. *Additional Considerations*: - Keeping the index up-to-date while it’s disabled seems preferable, as it avoids the need to rebuild the index if it’s re-enabled later. The alternative would be dropping and rebuilding the index upon re-enabling, which I believe would introduce additional overhead in terms of application logic & complexity. - I am also proposing to reuse the existing indisvalid flag to avoid adding new state and the maintenance that comes with it, but I’m open to feedback if this approach has potential downsides. - To keep the scope minimal for now, I propose that we only allow enabling and disabling indexes globally, and not locally, by supporting it exclusively in ALTER INDEX. I would love to know if this would break any SQL grammar promises that I might be unaware of. I would love to learn if this sounds like a good idea and how it can be improved further. Accordingly, as a next step I would be very happy to propose a patch as well. Best regards, Shayon Mukherjee
Re: Proposal to Enable/Disable Index using ALTER INDEX
On Tue, 10 Sept 2024 at 09:39, Shayon Mukherjee wrote: > Adding and removing indexes is a common operation in PostgreSQL. On larger > databases, however, these operations can be resource-intensive. When > evaluating the performance impact of one or more indexes, dropping them might > not be ideal since as a user you may want a quicker way to test their effects > without fully committing to removing & adding them back again. Which can be a > time taking operation on larger tables. > > Proposal: > I propose adding an ALTER INDEX command that allows for enabling or disabling > an index globally. This could look something like: > > ALTER INDEX index_name ENABLE; > ALTER INDEX index_name DISABLE; > > A disabled index would still receive updates and enforce constraints as usual > but would not be used for queries. This allows users to assess whether an > index impacts query performance before deciding to drop it entirely. I personally think having some way to alter an index to stop it from being used in query plans would be very useful for the reasons you mentioned. I don't have any arguments against the syntax you've proposed. We'd certainly have to clearly document that constraints are still enforced. Perhaps there is some other syntax which would self-document slightly better. I just can't think of it right now. > Implementation: > To keep this simple, I suggest toggling the indisvalid flag in pg_index > during the enable/disable operation. That's not a good idea as it would allow ALTER INDEX ... ENABLE; to be used to make valid a failed concurrently created index. I think this would need a new flag and everywhere in the planner would need to be adjusted to ignore indexes when that flag is false. > Additional Considerations: > - Keeping the index up-to-date while it’s disabled seems preferable, as it > avoids the need to rebuild the index if it’s re-enabled later. The > alternative would be dropping and rebuilding the index upon re-enabling, > which I believe would introduce additional overhead in terms of application > logic & complexity. I think the primary use case here is to assist in dropping useless indexes in a way that can very quickly be undone if the index is more useful than thought. If you didn't keep the index up-to-date then that would make the feature useless for that purpose. If we get the skip scan feature for PG18, then there's likely going to be lots of people with indexes that they might want to consider removing after upgrading. Maybe this is a good time to consider this feature as it possibly won't ever be more useful than it will be after we get skip scans. David
Re: Use streaming read API in ANALYZE
On Tue, Sep 10, 2024 at 3:36 AM Michael Banck wrote:
> I am a bit confused about the status of this thread. Robert mentioned
> RC1, so I guess it pertains to v17 but I don't see it on the open item
> wiki list?
Yes, v17. Alight, I'll add an item.
> Does the above mean you are going to revert it for v17, Thomas? And if
> so, what exactly? The ANALYZE changes on top of the streaming read API
> or something else about that API that is being discussed on this thread?
I might have been a little pessimistic in that assessment. Another
workaround that seems an awful lot cleaner and less invasive would be
to offer a new ReadStream API function that provides access to block
numbers and the strategy, ie the arguments of v16's
scan_analyze_next_block() function. Mats, what do you think about
this? (I haven't tried to preserve the prefetching behaviour, which
probably didn't actually too work for you in v16 anyway at a guess,
I'm just looking for the absolute simplest thing we can do to resolve
this API mismatch.) TimeScale could then continue to use its v16
coding to handle the two-relations-in-a-trenchcoat problem, and we
could continue discussing how to make v18 better.
I looked briefly at another non-heap-like table AM, the Citus Columnar
TAM. I am not familiar with that code and haven't studied it deeply
this morning, but its _next_block() currently just returns true, so I
think it will somehow need to change to counting calls and returning
false when it thinks its been called enough times (otherwise the loop
in acquire_sample_rows() won't terminate, I think?). I suppose an
easy way to do that without generating extra I/O or having to think
hard about how to preserve the loop cound from v16 would be to use
this function.
I think there are broadly three categories of TAMs with respect to
ANALYZE block sampling: those that are very heap-like (blocks of one
SMgrRelation) and can just use the stream directly, those that are not
at all heap-like (doing something completely different to sample
tuples and ignoring the block aspect but using _next_block() to
control the loop), and then Timescale's case which is sort of
somewhere in between: almost heap-like from the point of view of this
sampling code, ie working with blocks, but fudging the meaning of
block numbers, which we didn't anticipate. (I wonder if it fails to
sample fairly across the underlying relation boundary anyway because
their data densities must surely be quite different, but that's not
what we're here to talk about.)
. o O { We need that wiki page listing TAMs with links to the open
source ones... }
From db05a33e742cb292b1a2d44665582042fcd05d2f Mon Sep 17 00:00:00 2001
From: Thomas Munro
Date: Tue, 10 Sep 2024 10:15:33 +1200
Subject: [PATCH] Allow raw block numbers to be read from ReadStream.
---
src/backend/storage/aio/read_stream.c | 14 ++
src/include/storage/read_stream.h | 3 +++
2 files changed, 17 insertions(+)
diff --git a/src/backend/storage/aio/read_stream.c b/src/backend/storage/aio/read_stream.c
index 7f0e07d9586..cf914a7712f 100644
--- a/src/backend/storage/aio/read_stream.c
+++ b/src/backend/storage/aio/read_stream.c
@@ -835,3 +835,17 @@ read_stream_end(ReadStream *stream)
read_stream_reset(stream);
pfree(stream);
}
+
+/*
+ * Transitional support for code that would like to perform a read directly,
+ * without using the stream. Returns, and skips, the next block number that
+ * would be read by the stream's look-ahead algorithm, or InvalidBlockNumber
+ * if the end of the stream is reached. Also reports the strategy that would
+ * be used to read it.
+ */
+BlockNumber
+read_stream_next_block(ReadStream *stream, BufferAccessStrategy *strategy)
+{
+ *strategy = stream->ios[0].op.strategy;
+ return read_stream_get_block(stream, NULL);
+}
diff --git a/src/include/storage/read_stream.h b/src/include/storage/read_stream.h
index 2f94ee744b9..37b51934f8d 100644
--- a/src/include/storage/read_stream.h
+++ b/src/include/storage/read_stream.h
@@ -68,6 +68,9 @@ extern ReadStream *read_stream_begin_relation(int flags,
void *callback_private_data,
size_t per_buffer_data_size);
extern Buffer read_stream_next_buffer(ReadStream *stream, void **per_buffer_data);
+extern BlockNumber read_stream_next_block(ReadStream *stream,
+ BufferAccessStrategy *strategy);
+
extern ReadStream *read_stream_begin_smgr_relation(int flags,
BufferAccessStrategy strategy,
SMgrRelation smgr,
--
2.46.0
Re: query ID goes missing with extended query protocol
On Mon, Sep 09, 2024 at 11:24:28AM -0400, Robert Haas wrote: > That's interesting, but it sort of seems like it's reinventing the > wheel, vs. the one-line change that I proposed. Probably. I'll try to look at all that this week (with some automated tests!). -- Michael signature.asc Description: PGP signature
Re: Opinion poll: Sending an automated email to a thread when it gets added to the commitfest
On Mon, Sep 9, 2024, 22:02 Robert Haas wrote: > Should I #blamemagnus? > Yes. He seems unavailable for whatever reason based on my private holidays (maybe summer holidays) >
Re: Retire support for OpenSSL 1.1.1 due to raised API requirements
On Mon, Sep 09, 2024 at 11:29:09PM +0200, Daniel Gustafsson wrote: > Agreed. OpenSSL 1.1.1 is very different story and I suspect we'll be stuck on > that level for some time, but 1.1.0 is gone from production use. The cleanup induced by the removal of 1.1.0 is minimal. I'm on board about your argument with SSL_CTX_set_ciphersuites() to drop 1.1.0 and simplify the other feature. I was wondering about HAVE_SSL_CTX_SET_NUM_TICKETS for a few seconds, but morepork that relies on LibreSSL 3.3.2 disagrees with me. -- Michael signature.asc Description: PGP signature
Re: Opinion poll: Sending an automated email to a thread when it gets added to the commitfest
On Tue, Sep 10, 2024, 00:52 Jelte Fennema-Nio wrote: > On Mon, Sep 9, 2024, 22:02 Robert Haas wrote: > >> Should I #blamemagnus? >> > > Yes. He seems unavailable for whatever reason based on my private holidays > (maybe summer holidays) > *based on my private/off-list communication with him >
Re: query_id, pg_stat_activity, extended query protocol
Sorry for the late reply on this thread.
On 14/8/2024 23:05, Imseih (AWS), Sami wrote:
> There are no tests as this requires more discussion in a separate thread(?)
> Unfortunately, TAP tests don't allow us to keep a connection and
> manually permutate the order of queries sent to different connections.
> But isolation tests are designed to do so. Of course, they aren't the
> best if you need to compare values produced by various queries but see a
> clumsy sketch doing that in the attachment.
It would be nice to use isolation tests as you have, those type of tests
don't support psql meta-commands. We need \parse, \bind, \bind_named
to test queryId for queries issued through extended query protocol.
With TAP tests we can use query_until in BackgroundPsql to have one
connection issue a command and another connection track the # of distinct
queryIds expected. See the 007_query_id.pl of an example TAP test that
could be added under test_misc.
An INJECTION_POINT can also be added right before we call pgstat_report_query_id
in plancache.c. This will allow us to test when we expect the queryId to
change after a cache revalidation. Thoughts?
> Also, while writing the test, I found out that now, JumbleQuery takes
> into account constants of the A_Const node, and calls of the same
> prepared statement with different parameters generate different
> query_id. Is it a reason to introduce JumbleQuery options and allow
> different logic of queryid generation?
Can you start a new thread for this prepared statement scenario?
--
Sami
# Copyright (c) 2021-2024, PostgreSQL Global Development Group
# Check that query_id is advertised as expected. This requires a concurrent
# session to check for the last query_id advertised by another session.
use strict;
use warnings FATAL => 'all';
use PostgreSQL::Test::Cluster;
use PostgreSQL::Test::Utils;
use Test::More;
# declare global variables
my $ret;
my $node;
my $psql_session1;
my $psql_session2;
# declare the helper functions
sub test_num_unique_qids
{
my ($command, $expected_qids, $wait_for) = @_;
$psql_session1->query_safe($command);
$psql_session2->query_safe(q(INSERT INTO track_query_id
SELECT query_id FROM pg_stat_activity
WHERE application_name = 'query_id_test'
AND rtrim(query) = 'SELECT * FROM test_query_id'
AND query_id is NOT NULL and query_id <> 0;));
$ret = $psql_session2->query_safe(
qq[
SELECT COUNT(DISTINCT query_id) FROM track_query_id;
]);
is($ret, $expected_qids, "# of distinct queryIds");
}
sub create_track_qid_table
{
$node->safe_psql("postgres", qq[CREATE TABLE track_query_id (query_id BIGINT);]);
}
sub create_test_qid_table
{
$node->safe_psql("postgres", qq[DROP TABLE IF EXISTS test_query_id;]);
$node->safe_psql("postgres", qq[CREATE TABLE test_query_id (id INT);]);
}
# Setup the test
## create the node
$node = PostgreSQL::Test::Cluster->new('main');
$node->init;
$node->append_conf('postgresql.conf', 'compute_query_id = on');
$node->start;
## create the sessions
## session1 will run the queries and session2 will query pg_stat_activity
## for the last query_id of session1 and write it to track_query_id
$psql_session1 = $node->background_psql('postgres');
$psql_session1->query_until(qr//, q(SET application_name = query_id_test;));
$psql_session2 = $node->background_psql('postgres');
create_track_qid_table();
# test 1 - check post-parse queryId is reported with extended query protocol
# We expected 1 queryId to be reported so far.
create_test_qid_table();
test_num_unique_qids("SELECT * FROM test_query_id \\parse n1\n", 1, qr//);
# test 2 - check bind-execute queryId is reported in extended query protocol
# Since we recreate the table of the underlysing named portal, we now expect
# 2 queryIds to be reported.
create_test_qid_table();
test_num_unique_qids("\\bind_named n1 \\g\n", 2, qr//);
# Destroy the sessions and the node
$psql_session1->quit;
$psql_session2->quit;
$node->stop;
done_testing();
Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation
On Fri, Sep 6, 2024 at 3:36 PM Tom Lane wrote: > > Noah Misch writes: > > Yes, that's one way to make it work. If we do it that way, it would be > > critical to make the ALTER EXTENSION UPDATE run before anything uses the > > index. Otherwise, we'll run new v18 "signed char" code on a v17 "unsigned > > char" data file. Running ALTER EXTENSION UPDATE early enough should be > > feasible, so that's fine. Some other options: > > > - If v18 "pg_dump -b" decides to emit CREATE INDEX ... > > gin_trgm_ops_unsigned, > > then make it also emit the statements to create the opclass. > > > - Ship pg_trgm--1.6--1.7.sql in back branches. If the upgrade wants to use > > gin_trgm_ops_unsigned, require the user to ALTER EXTENSION UPDATE first. > > (In back branches, the C code behind gin_trgm_ops_unsigned could just > > raise > > an error if called.) > > I feel like all of these are leaning heavily on users to get it right, > and therefore have a significant chance of breaking use-cases that > were perfectly fine before. > > Remind me of why we can't do something like this: > > * Add APIs that allow opclasses to read/write some space in the GIN > metapage. (We could get ambitious and add such APIs for other AMs > too, but doing it just for GIN is probably a prudent start.) Ensure > that this space is initially zeroed. > > * In gin_trgm_ops, read a byte of this space and interpret it as > 0 = unset > 1 = signed chars > 2 = unsigned chars > If the value is zero, set the byte on the basis of the native > char-signedness of the current platform. (Obviously, a > secondary server couldn't write the byte, and would have to > wait for the primary to update the value. In the meantime, > it's no more broken than today.) When do we set the byte on the primary server? If it's the first time to use the GIN index, secondary servers would have to wait for the primary to use the GIN index, which could be an unpredictable time or it may never come depending on index usages. Probably we can make pg_upgrade set the byte in the meta page of GIN indexes that use the gin_trgm_ops. Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com
Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation
Masahiko Sawada writes: > When do we set the byte on the primary server? If it's the first time > to use the GIN index, secondary servers would have to wait for the > primary to use the GIN index, which could be an unpredictable time or > it may never come depending on index usages. Probably we can make > pg_upgrade set the byte in the meta page of GIN indexes that use the > gin_trgm_ops. Hmm, perhaps. That plus set-it-during-index-create would remove the need for dynamic update like I suggested. So very roughly the amount of complexity would balance out. Do you have an idea for how we'd get this to happen during pg_upgrade, exactly? regards, tom lane
Re: Remove emode argument from XLogFileRead/XLogFileReadAnyTLI
On Mon, Sep 09, 2024 at 05:45:13PM +0900, Yugo NAGATA wrote: > I mean to remove emode from XLogFileRead, too, but this fix is accidentally > missed in the previous patch. I attached the updated patch. This is neat because we don't need to guess how XLogFileRead() should fail on PANIC, allow things with a DEBUG2 or something else, and XLogFileReadAnyTLI()'s sole caller used DEBUG2. Applied. -- Michael signature.asc Description: PGP signature
Re: access numeric data in module
Sorry for taking so long to respond. I was at my day-job. As I mentioned, I am the maintainer of the PL/Haskell language extension. This extension allows users to write code in the Haskell language. In order to use numeric types, I will need to create a Haskell type equivalent. Something like data Numeric = PosInfinity | NegInfinity | NaN | Number Integer Int16 where the Number constructor represents a numeric's mantissa and weight. In order to get or return data, I would need to be able to access those fields of the numeric type. I'm not proposing giving access to the actual numeric structure. Rather, the data should be accessed by function call or macro. This would allow future changes to the inner workings without breaking compatibility as long as the interface is maintained. It looks to me like all of the code to access data exists, it should simply be made accessible. An additional function should exist that allows an extension to create a numeric structure by passing the needed data. -Ed On Mon, Sep 9, 2024 at 2:45 PM Robert Haas wrote: > On Mon, Sep 9, 2024 at 2:02 PM Tom Lane wrote: > > By that argument, we should move every declaration in every .c file > > into c.h and be done. I'd personally be happier if we had *not* > > exposed the other data structure details you mention, but that > > ship has sailed. > > Not every declaration in every .c file is of general interest, but the > ones that are probably should be moved into .h files. The on-disk > representation of a commonly-used data type certainly qualifies. > > You can see from Chapman's reply that I'm not making this up: when we > don't expose things, it doesn't keep people from depending on them, it > just makes them copy our code into their own repository. That's not a > win. It makes those extensions more fragile, not less, and it makes > the PostgreSQL extension ecosystem worse. pg_hint_plan is another, > recently-discussed example of this phenomenon: refuse to give people > the keys, and they start hot-wiring stuff. > > > If we do do what you're advocating, I'd at least insist that the > > declarations go into a new file numeric_internal.h, so that it's > > clear to all concerned that they're playing with fire if they > > depend on that stuff. > > I think that's a bit pointless considering that we don't do it in any > of the other cases. I'd rather be consistent with our usual practice. > But if it ends up in a separate header file that's still better than > the status quo. > > -- > Robert Haas > EDB: http://www.enterprisedb.com >
Re: Partitioned tables and [un]loggedness
On Mon, Sep 09, 2024 at 03:56:14PM +0900, Michael Paquier wrote:
> How about inventing a new ATT_PARTITIONED_TABLE and make a clean split
> between both relkinds? I'd guess that blocking both SET LOGGED and
> UNLOGGED for partitioned tables is the best move, even if it is
> possible to block only one or the other, of course.
I gave it a try, and while it is much more invasive, it is also much
more consistent with the rest of the file.
Thoughts?
--
Michael
From fefb821c033784a01c39d99d477982ded1aebf40 Mon Sep 17 00:00:00 2001
From: Michael Paquier
Date: Tue, 10 Sep 2024 09:16:43 +0900
Subject: [PATCH v4 1/2] Introduce ATT_PARTITIONED_TABLE in tablecmds.c
---
src/backend/commands/tablecmds.c | 92 +---
1 file changed, 48 insertions(+), 44 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index b3cc6f8f69..f6a6e3e148 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -330,6 +330,7 @@ struct DropRelationCallbackState
#define ATT_FOREIGN_TABLE 0x0020
#define ATT_PARTITIONED_INDEX 0x0040
#define ATT_SEQUENCE 0x0080
+#define ATT_PARTITIONED_TABLE 0x0100
/*
* ForeignTruncateInfo
@@ -4777,7 +4778,7 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
{
case AT_AddColumn: /* ADD COLUMN */
ATSimplePermissions(cmd->subtype, rel,
-ATT_TABLE | ATT_COMPOSITE_TYPE | ATT_FOREIGN_TABLE);
+ATT_TABLE | ATT_PARTITIONED_TABLE | ATT_COMPOSITE_TYPE | ATT_FOREIGN_TABLE);
ATPrepAddColumn(wqueue, rel, recurse, recursing, false, cmd,
lockmode, context);
/* Recursion occurs during execution phase */
@@ -4798,7 +4799,7 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
* substitutes default values into INSERTs before it expands
* rules.
*/
- ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_VIEW | ATT_FOREIGN_TABLE);
+ ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_PARTITIONED_TABLE | ATT_VIEW | ATT_FOREIGN_TABLE);
ATSimpleRecursion(wqueue, rel, cmd, recurse, lockmode, context);
/* No command-specific prep needed */
pass = cmd->def ? AT_PASS_ADD_OTHERCONSTR : AT_PASS_DROP;
@@ -4806,19 +4807,19 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
case AT_CookedColumnDefault: /* add a pre-cooked default */
/* This is currently used only in CREATE TABLE */
/* (so the permission check really isn't necessary) */
- ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_FOREIGN_TABLE);
+ ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_PARTITIONED_TABLE | ATT_FOREIGN_TABLE);
/* This command never recurses */
pass = AT_PASS_ADD_OTHERCONSTR;
break;
case AT_AddIdentity:
- ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_VIEW | ATT_FOREIGN_TABLE);
+ ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_PARTITIONED_TABLE | ATT_VIEW | ATT_FOREIGN_TABLE);
/* Set up recursion for phase 2; no other prep needed */
if (recurse)
cmd->recurse = true;
pass = AT_PASS_ADD_OTHERCONSTR;
break;
case AT_SetIdentity:
- ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_VIEW | ATT_FOREIGN_TABLE);
+ ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_PARTITIONED_TABLE | ATT_VIEW | ATT_FOREIGN_TABLE);
/* Set up recursion for phase 2; no other prep needed */
if (recurse)
cmd->recurse = true;
@@ -4826,82 +4827,82 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
pass = AT_PASS_MISC;
break;
case AT_DropIdentity:
- ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_VIEW | ATT_FOREIGN_TABLE);
+ ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_PARTITIONED_TABLE | ATT_VIEW | ATT_FOREIGN_TABLE);
/* Set up recursion for phase 2; no other prep needed */
if (recurse)
cmd->recurse = true;
pass = AT_PASS_DROP;
break;
case AT_DropNotNull: /* ALTER COLUMN DROP NOT NULL */
- ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_FOREIGN_TABLE);
+ ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_PARTITIONED_TABLE | ATT_FOREIGN_TABLE);
ATPrepDropNotNull(rel, recurse, recursing);
ATSimpleRecursion(wqueue, rel, cmd, recurse, lockmode, context);
pass = AT_PASS_DROP;
break;
case AT_SetNotNull: /* ALTER COLUMN SET NOT NULL */
- ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_FOREIGN_TABLE);
+ ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_PARTITIONED_TABLE | ATT_FOREIGN_TABLE);
/* Need command-specific recursion decision */
ATPrepSetNotNull(wqueue, rel, cmd, recurse, recursing,
lockmode, context);
pass = AT_PASS_COL_ATTRS;
break;
case AT_CheckNotNull: /* check column is already marked NOT NULL */
- ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_FOREIGN_TABLE);
+ ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_PARTITIONED_TABLE | ATT_FOREIGN_TABLE);
ATSimpleRecursion(wqueue, rel, cmd, recurse, loc
Re: query_id, pg_stat_activity, extended query protocol
> >> I think the testing discussion should be moved to a different thread. > >> What do you think? > > See v4. > > > > 0001 deals with reporting queryId in exec_execute_message and > > exec_bind_message. > > 0002 deals with reporting queryId after a cache invalidation. > > > > There are no tests as this requires more discussion in a separate thread(?) > At first, these patches look good. > But I have a feeling of some mess here: > queryId should be initialised at the top-level query. At the same time, > the RevalidateCachedQuery routine can change this value in the case of > the query tree re-validation. > You can say that this routine can't be called from a non-top-level query > right now, except SPI. Yes, but what about extensions or future usage? This is a valid point. RevalidatePlanCache is forcing a new queryId to be advertised ( 'true' as the second argument to pgstat_report_query_id) . This means, v4-0002-Report-new-queryId-after-plancache-re-validation.patch will result in a non top-level queryId being advertised. See the attached test case. I need to think about this a bit. -- Sami --- --- REPRO STEPS --- drop table if exists t1 ; create table t1 ( id int ); CREATE OR REPLACE FUNCTION test_prep2() RETURNS void AS $$ DECLARE lid int; BEGIN SELECT * FROM t1 INTO lid; END; $$ LANGUAGE plpgsql; select test_prep(); select test_prep2(); select test_prep2(); select test_prep2(); drop table if exists t1 ; create table t1 ( id int ); select test_prep2(); -- --- RESULTS -- The first time "select query, query_id from pg_stat_activity ;" is ran, we see the correct query_id of the top-level "select test_prep2();" postgres=# select query, query_id from pg_stat_activity ; query | query_id +-- select query, query_id from pg_stat_activity ; | 4920466282180912204 select test_prep2(); | -8872343059522300094 | | | | | (7 rows) postgres=# explain verbose select test_prep2(); QUERY PLAN -- Result (cost=0.00..0.26 rows=1 width=4) Output: test_prep2() Query Identifier: -8872343059522300094 (3 rows) but after the table is dropped and recreated, the advertised query_id is wrong for the query text. It is the query_id of "select * from t1;" postgres=# select query, query_id from pg_stat_activity ; query | query_id +-- select query, query_id from pg_stat_activity ; | 4920466282180912204 select test_prep2(); | -6535803560158626277 | | | | | (7 rows) postgres=# explain verbose select * from t1; QUERY PLAN - Seq Scan on public.t1 (cost=0.00..35.50 rows=2550 width=4) Output: id Query Identifier: -6535803560158626277 (3 rows)
Re: Introduce XID age and inactive timeout based replication slot invalidation
Hi, here is the remainder of my v45-0001 review. These comments are
for the test code only.
==
Testcase #1
1.
+# Testcase start
+#
+# Invalidate streaming standby slot and logical failover slot on primary due to
+# inactive timeout. Also, check logical failover slot synced to standby from
+# primary doesn't invalidate on its own, but gets the invalidated
state from the
+# primary.
nit - s/primary/the primary/ (in a couple of places)
nit - s/standby/the standby/
nit - other trivial tweaks.
~~~
2.
+# Create sync slot on primary
+$primary->psql('postgres',
+ q{SELECT pg_create_logical_replication_slot('sync_slot1',
'test_decoding', false, false, true);}
+);
nit - s/primary/the primary/
~~~
3.
+$primary->safe_psql(
+ 'postgres', qq[
+SELECT pg_create_physical_replication_slot(slot_name :=
'sb_slot1', immediately_reserve := true);
+]);
Should this have a comment?
~~~
4.
+# Wait until standby has replayed enough data
+$primary->wait_for_catchup($standby1);
nit - s/standby/the standby/
~~~
5.
+# Sync primary slot to standby
+$standby1->safe_psql('postgres', "SELECT pg_sync_replication_slots();");
nit - /Sync primary slot to standby/Sync the primary slots to the standby/
~~~
6.
+# Confirm that logical failover slot is created on standby
nit - s/Confirm that logical failover slot is created on
standby/Confirm that the logical failover slot is created on the
standby/
~~~
7.
+is( $standby1->safe_psql(
+ 'postgres',
+ q{SELECT count(*) = 1 FROM pg_replication_slots
+ WHERE slot_name = 'sync_slot1' AND synced AND NOT temporary;}
+ ),
+ "t",
+ 'logical slot sync_slot1 has synced as true on standby');
IMO here you should also be checking that the sync slot state is NOT
invalidated, just as a counterpoint for the test part later that
checks that it IS invalidated.
~~~
8.
+my $inactive_timeout = 1;
+
+# Set timeout so that next checkpoint will invalidate inactive slot
+$primary->safe_psql(
+ 'postgres', qq[
+ALTER SYSTEM SET replication_slot_inactive_timeout TO
'${inactive_timeout}s';
+]);
+$primary->reload;
8a.
nit - I think that $inactive_timeout assignment belongs below your comment.
~
8b.
nit - s/Set timeout so that next checkpoint will invalidate inactive
slot/Set timeout GUC so that the next checkpoint will invalidate
inactive slots/
~~~
9.
+# Check for logical failover slot to become inactive on primary. Note that
+# nobody has acquired slot yet, so it must get invalidated due to
+# inactive timeout.
nit - /Check for logical failover slot to become inactive on
primary./Wait for logical failover slot to become inactive on the
primary./
nit - /has acquired slot/has acquired the slot/
~~~
10.
+# Sync primary slot to standby. Note that primary slot has already been
+# invalidated due to inactive timeout. Standby must just sync inavalidated
+# state.
nit - minor, add "the". fix typo "inavalidated", etc. suggestion:
Re-sync the primary slots to the standby. Note that the primary slot was already
invalidated (above) due to inactive timeout. The standby must just
sync the invalidated
state.
~~~
11.
+# Make standby slot on primary inactive and check for invalidation
+$standby1->stop;
nit - /standby slot/the standby slot/
nit - /on primary/on the primary/
==
Testcase #2
12.
I'm not sure it is necessary to do all this extra work. IIUC, there
was already almost everything you needed in the previous Testcase #1.
So, I thought you could just combine this extra standby timeout test
in Testcase #1.
Indeed, your Testcase #1 comment still says it is doing this: ("Also,
check logical failover slot synced to standby from primary doesn't
invalidate on its own,...")
e.g.
- NEW: set the GUC timeout on the standby
- sync the sync_slot (already doing in test #1)
- ensure the synced slot is NOT invalid (already suggested above for test #1)
- NEW: then do a standby sleep > timeout duration
- NEW: then do a standby CHECKPOINT...
- NEW: then ensure the sync slot invalidation did NOT happen
- then proceed with the rest of test #1...
==
Testcase #3
13.
nit - remove a few blank lines to group associated statements together.
~~~
14.
+$publisher->safe_psql(
+ 'postgres', qq[
+ALTER SYSTEM SET replication_slot_inactive_timeout TO '
${inactive_timeout}s';
+]);
+$publisher->reload;
nit - this deserves a comment, the same as in Testcase #1
==
sub wait_for_slot_invalidation
15.
+# Check for slot to first become inactive and then get invalidated
+sub check_for_slot_invalidation
nit - IMO the previous name was better (e.g. "wait_for.." instead of
"check_for...") because that describes exactly what the subroutine is
doing.
suggestion:
# Wait for the slot to first become inactive and then get invalidated
sub wait_for_slot_invalidation
~~~
16.
+{
+ my ($node, $slot, $offset, $inactive_timeout) = @_;
+ my $name = $node->name;
The variable $name seems too vague. How about $node_name?
~~~
17.
+ # Wait for invalidation reason to be set
+ $node->poll_query_until(
+ 'postgres', qq[
Re: query_id, pg_stat_activity, extended query protocol
> > >> I think the testing discussion should be moved to a different thread. > > >> What do you think? > > > See v4. > > > > > > 0001 deals with reporting queryId in exec_execute_message and > > > exec_bind_message. > > > 0002 deals with reporting queryId after a cache invalidation. > > > > > > There are no tests as this requires more discussion in a separate > > > thread(?) > > At first, these patches look good. > > But I have a feeling of some mess here: > > queryId should be initialised at the top-level query. At the same time, > > the RevalidateCachedQuery routine can change this value in the case of > > the query tree re-validation. > > You can say that this routine can't be called from a non-top-level query > > right now, except SPI. Yes, but what about extensions or future usage? > This is a valid point. RevalidatePlanCache is forcing a > new queryId to be advertised ( 'true' as the second argument to > pgstat_report_query_id) . This means, > v4-0002-Report-new-queryId-after-plancache-re-validation.patch > will result in a non top-level queryId being advertised. An idea would be to add bool field called force_update_qid to CachedPlanSource, and this field can be set to 'true' after a call to CreateCachedPlan. RevalidateCachedQuery will only update the queryId if this value is 'true'. For now, only exec_parse_message will set this field to 'true', but any caller can decide to set it to 'true' if there are other cases in the future. What do you think? -- Sami
Re: First draft of PG 17 release notes
On Sat, Sep 7, 2024 at 11:55:09AM +0200, Álvaro Herrera wrote: > On 2024-Sep-05, Bruce Momjian wrote: > > > That seems more infrastructure/extension author stuff which isn't > > normally mentioned in the release notes. I think such people really > > need to look at all the commit messages. > > Are you saying all extension authors should be reading the complete git > log for every single major release? That's a strange position to take. > Isn't this a good fit for "E.1.3.10. Source Code"? Yes. There are so many changes at the source code level it is unwise to try and get them into the main release notes. If someone wants to create an addendum, like was suggested for pure performance improvements, that would make sense. -- Bruce Momjian https://momjian.us EDB https://enterprisedb.com Only you can decide what is important to you.
Re: CI, macports, darwin version problems
On Mon, Sep 9, 2024 at 1:37 PM Joe Conway wrote: > Seems the mounted drive got unmounted somehow ¯\_(ツ)_/¯ > > Please check it out and let me know if it is working properly now. Looks good, thanks!
Re: Disallow altering invalidated replication slots
Hi, here are some review comments for patch v1.
==
Commit message
1.
ALTER_REPLICATION_SLOT on invalidated replication slots is unnecessary
as there is no way...
suggestion:
ALTER_REPLICATION_SLOT for invalid replication slots should not be
allowed because there is no way...
==
2. Missing docs update
Should this docs page [1] be updated to say ALTER_REPLICATION_SLOT is
not allowed for invalid slots?
==
src/backend/replication/slot.c
3.
+ if (MyReplicationSlot->data.invalidated != RS_INVAL_NONE)
+ ereport(ERROR,
+ errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("cannot alter replication slot \"%s\"", name),
+ errdetail("This replication slot was invalidated due to \"%s\".",
+ SlotInvalidationCauses[MyReplicationSlot->data.invalidated]));
+
I thought including the reason "invalid" (e.g. "cannot alter invalid
replication slot \"%s\"") in the message might be better, but OTOH I
see the patch message is the same as an existing one. Maybe see what
others think.
==
src/test/recovery/t/035_standby_logical_decoding.pl
3.
There is already a comment about this test:
##
# Recovery conflict: Invalidate conflicting slots, including in-use slots
# Scenario 1: hot_standby_feedback off and vacuum FULL
#
# In passing, ensure that replication slot stats are not removed when the
# active slot is invalidated.
##
IMO we should update that "In passing..." sentence to something like:
In passing, ensure that replication slot stats are not removed when
the active slot is invalidated, and check that an error occurs when
attempting to alter the invalid slot.
==
[1] docs - https://www.postgresql.org/docs/devel/protocol-replication.html
Kind Regards,
Peter Smith.
Fujitsu Austalia
change "attnum <=0" to "attnum <0" for better reflect system attribute
hi,
one minor issue. not that minor,
since many DDLs need to consider the system attribute.
looking at these functions:
SearchSysCacheCopyAttName
SearchSysCacheAttName
get_attnum
get_attnum says:
Returns InvalidAttrNumber if the attr doesn't exist (or is dropped).
So I conclude that "attnum == 0" is not related to the idea of a system column.
for example, ATExecColumnDefault, following code snippet,
the second ereport should be "if (attnum < 0)"
==
attnum = get_attnum(RelationGetRelid(rel), colName);
if (attnum == InvalidAttrNumber)
ereport(ERROR,
(errcode(ERRCODE_UNDEFINED_COLUMN),
errmsg("column \"%s\" of relation \"%s\" does not exist",
colName, RelationGetRelationName(rel;
/* Prevent them from altering a system attribute */
if (attnum <= 0)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("cannot alter system column \"%s\"",
colName)));
==
but there are many occurrences of "attnum <= 0".
I am sure tablecmds.c, we can change to "attnum < 0".
not that sure with other places.
In some places in tablecmd.c,
we already use "attnum < 0" to represent the system attribute.
so it's kind of inconsistent already.
Should we do the change?
Re: change "attnum <=0" to "attnum <0" for better reflect system attribute
jian he writes: > get_attnum says: > Returns InvalidAttrNumber if the attr doesn't exist (or is dropped). > So I conclude that "attnum == 0" is not related to the idea of a system > column. attnum = 0 is also used for whole-row Vars. This is a pretty unfortunate choice given the alternative meaning of "invalid", but cleaning it up would be a daunting task (with not a whole lot of payoff in the end, AFAICS). It's deeply embedded. That being the case, you have to tread *very* carefully when considering making changes like this. > for example, ATExecColumnDefault, following code snippet, > the second ereport should be "if (attnum < 0)" > /* Prevent them from altering a system attribute */ > if (attnum <= 0) I think that's just fine as-is. Sure, the == case is unreachable, but it is very very common to consider whole-row Vars as being more like system attributes than user attributes. In this particular case, for sure we don't want to permit attaching a default to a whole-row Var. So I'm content to allow the duplicative rejection. regards, tom lane
Re: Wrong results with grouping sets
On Wed, Sep 4, 2024 at 9:16 AM Richard Guo wrote: > I'm seeking the possibility to push 0001 and 0002 sometime this month. > Please let me know if anyone thinks this is unreasonable. > > For 0003, it might be extended to remove all no-op PHVs except those > that are serving to isolate subexpressions, not only the PHVs used to > carry the nullingrel bit that represents the grouping step. There is > a separate thread for it [1]. I went ahead and pushed 0001 and 0002, and am now waiting for the upcoming bug reports. Thanks for all the discussions and reviews. Thanks Richard
Re: change "attnum <=0" to "attnum <0" for better reflect system attribute
On Tue, Sep 10, 2024 at 8:46 AM jian he wrote:
>
> hi,
> one minor issue. not that minor,
> since many DDLs need to consider the system attribute.
>
> looking at these functions:
> SearchSysCacheCopyAttName
> SearchSysCacheAttName
> get_attnum
>
> get_attnum says:
> Returns InvalidAttrNumber if the attr doesn't exist (or is dropped).
>
> So I conclude that "attnum == 0" is not related to the idea of a system
> column.
>
>
> for example, ATExecColumnDefault, following code snippet,
> the second ereport should be "if (attnum < 0)"
> ==
> attnum = get_attnum(RelationGetRelid(rel), colName);
> if (attnum == InvalidAttrNumber)
> ereport(ERROR,
> (errcode(ERRCODE_UNDEFINED_COLUMN),
> errmsg("column \"%s\" of relation \"%s\" does not exist",
> colName, RelationGetRelationName(rel;
>
> /* Prevent them from altering a system attribute */
> if (attnum <= 0)
> ereport(ERROR,
> (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> errmsg("cannot alter system column \"%s\"",
> colName)));
> ==
> but there are many occurrences of "attnum <= 0".
> I am sure tablecmds.c, we can change to "attnum < 0".
> not that sure with other places.
What it really means is "Prevent them from altering any attribute not
defined by user" - a whole row reference is not defined explicitly by
user; it's collection of user defined attributes and it's not
cataloged.
I think we generally confuse between system attribute and !(user
attribute); the grey being attnum = 0. It might be better to create
macros for these cases and use them to make their usage clear.
e.g. #define ATTNUM_IS_SYSTEM(attnum) ((attnum) < 0)
#define ATTNUM_IS_USER_DEFINED(attnum) ((attnum) > 0)
#define WholeRowAttrNumber 0
add good comments about usage near their definitions and use
appropriately in the code.
Example above would then turn into (notice ! in the condition)
/* Prevent them from altering an attribute not defined by user */
if (!ATTNUM_IS_USER_DEFINED(attnum) )
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("attribute \"%s\" is not a user-defined attribute",
colName)));
--
Best Wishes,
Ashutosh Bapat
Re: Use streaming read API in ANALYZE
On Tue, Sep 10, 2024 at 10:27 AM Thomas Munro wrote:
> Mats, what do you think about
> this? (I haven't tried to preserve the prefetching behaviour, which
> probably didn't actually too work for you in v16 anyway at a guess,
> I'm just looking for the absolute simplest thing we can do to resolve
> this API mismatch.) TimeScale could then continue to use its v16
> coding to handle the two-relations-in-a-trenchcoat problem, and we
> could continue discussing how to make v18 better.
. o O { Spitballing here: if we add that tiny function I showed to get
you unstuck for v17, then later in v18, if we add a multi-relation
ReadStream constructor/callback (I have a patch somewhere, I want to
propose that as it is needed for streaming recovery), you could
construct a new ReadSteam of your own that is daisy-chained from that
one. You could keep using your N + M block numbering scheme if you
want to, and the callback of the new stream could decode the block
numbers and redirect to the appropriate relation + real block number.
That way you'd get I/O concurrency for both relations (for now just
read-ahead advice, but see Andres's AIO v2 thread). That'd
essentially be a more supported version of the 'access the struct
internals' idea (or at least my understanding of what you had in
mind), through daisy-chained streams. A little weird maybe, and maybe
the redesign work will result in something completely
different/better... just a thought... }
Re: Pgoutput not capturing the generated columns
On Tue, Sep 10, 2024 at 2:51 AM Masahiko Sawada wrote: > > On Mon, Sep 9, 2024 at 2:38 AM Shubham Khanna > wrote: > > > > Thank you for updating the patches. I have some comments: > > Do we really need to add this option to test_decoding? > I don't see any reason to have such an option in test_decoding, otherwise, we need a separate option for each publication option. I guess this is leftover of the previous subscriber-side approach. > I think it > would be good if this improves the test coverage. Otherwise, I'm not > sure we need this part. If we want to add it, I think it would be > better to have it in a separate patch. > Right. > --- > + > + If the publisher-side column is also a generated column > then this option > + has no effect; the publisher column will be filled as normal with > the > + publisher-side computed or default data. > + > > I don't understand this description. Why does this option have no > effect if the publisher-side column is a generated column? > Shouldn't it be subscriber-side? I have one additional comment: /* - * If the publication is FOR ALL TABLES then it is treated the same as - * if there are no column lists (even if other publications have a - * list). + * If the publication is FOR ALL TABLES and include generated columns + * then it is treated the same as if there are no column lists (even + * if other publications have a list). */ - if (!pub->alltables) + if (!pub->alltables || !pub->pubgencolumns) Why do we treat pubgencolumns at the same level as the FOR ALL TABLES case? I thought that if the user has provided a column list, we only need to publish the specified columns even when the publish_generated_columns option is set. -- With Regards, Amit Kapila.
