Re: Triage on old commitfest entries

[Jaime Casanova]

On Sun, Oct 03, 2021 at 03:14:58PM -0400, Tom Lane wrote:
[...]
> 
> Here's what I found, along with some commentary about each one.
> 
> Patch Age in CFs
> 
> Protect syscache from bloating with negative cache entries23
>   Last substantive discussion 2021-01, currently passing cfbot
> 
>   It's well known that I've never liked this patch, so I can't
>   claim to be unbiased.  But what I see here is a lot of focus
>   on specific test scenarios with little concern for the
>   possibility that other scenarios will be made worse.
>   I think we need some new ideas to make progress.
>   Proposed action: RWF

if we RwF this patch we should add the thread to the TODO entry 
it refers to 

> 
> Transactions involving multiple postgres foreign servers  18
>   Last substantive discussion 2021-07, currently failing cfbot
> 
>   This has been worked on fairly recently, but frankly I'm
>   dubious that we want to integrate a 2PC XM into Postgres.
>   Proposed action: Reject
> 

Masahiko has marked the patch as RwF already

> schema variables, LET command 18
>   Last substantive discussion 2021-09, currently passing cfbot
> 
>   Seems to be actively worked on, but is it ever going to get
>   committed?
> 

I had already moved this to Next CF when I read this, but I found this 
sounds useful

> Remove self join on a unique column   16
>   Last substantive discussion 2021-07, currently passing cfbot
> 
>   I'm not exactly sold that this has a good planning-cost-to-
>   usefulness ratio.
>   Proposed action: RWF
> 

It seems there is no proof that this will increase performance in the
thread.
David you're reviewer on this patch, what your opinion on this is?

> Index Skip Scan   16
>   Last substantive discussion 2021-05, currently passing cfbot
> 
>   Seems possibly useful, but we're not making progress.
> 

Peter G mentioned this would be useful. What we need to advance this
one? 

> standby recovery fails when re-replaying due to missing directory which was 
> removed in previous replay13
>   Last substantive discussion 2021-09, currently passing cfbot
> 
>   This is a bug fix, so we shouldn't drop it.
> 

Moved to Next CF

> Remove page-read callback from XLogReaderState12
>   Last substantive discussion 2021-04, currently failing cfbot
> 
>   Not sure what to think about this one, but given that it
>   was pushed and later reverted, I'm suspicious of it.
> 

I guess those are enough for a decision: marked as RwF
If this is useful a new patch would be sent.

> Incremental Materialized View Maintenance 12
>   Last substantive discussion 2021-09, currently passing cfbot
> 
>   Seems to be actively worked on.

Moved to Next CF

> 
> pg_upgrade fails with non-standard ACL12
>   Last substantive discussion 2021-03, currently passing cfbot
> 
>   This is a bug fix, so we shouldn't drop it.
> 

Moved to Next CF

> Fix up partitionwise join on how equi-join conditions between the partition 
> keys are identified   11
>   Last substantive discussion 2021-07, currently passing cfbot
> 
>   This is another one where I feel we need new ideas to make
>   progress.
>   Proposed action: RWF

It seems there has been no activity since last version of the patch so I
don't think RwF is correct. What do we need to advance on this one?

> 
> A hook for path-removal decision on add_path  11
>   Last substantive discussion 2021-03, currently passing cfbot
> 
>   I don't think this is a great idea: a hook there will be
>   costly, and it's very unclear how multiple extensions could
>   interact correctly.
>   Proposed action: Reject
> 

Any other comments on this one?

> Implement INSERT SET syntax   11
>   Last substantive discussion 2020-03, currently passing cfbot
> 
>   This one is clearly stalled.  I don't think it's necessarily
>   a bad idea, but we seem not to be very interested.
>   Proposed action: Reject for lack of interest
> 

Again, no activity after last patch. 

> SQL:2011 application time 11
>   Last substantive discussion 2021-10, currently failing cfbot
> 
>   Actively worked on, and it's a big feature so long gestation
>   isn't surprising.
> 

Moved to Next CF

> WITH SYSTEM VERSIONING Temporal Tables11
>   Last substantive discussion 2021-09, currently failing cfbot
> 
>   Actively worked on, and it's a big feature so long gestation
>   isn't surprising.
> 

Moved to Next CF

> psql - add SHOW_ALL_RESULTS option11
>   Last substantive discussion 2021-09, currently passing cfbot
> 
>   This got committed and reverted once already.  I have to be
>   suspicious of whether this is a good design.
> 

No activity after last patch

> Split StdRdOptions into HeapOptions and ToastOptions  10
>   Last substantive discussion 2021-06, currently failing cfbot

Re: Use simplehash.h instead of dynahash in SMgr

[Jaime Casanova]

On Mon, Sep 27, 2021 at 04:30:25PM +1300, David Rowley wrote:
> On Fri, 24 Sept 2021 at 20:26, Jaime Casanova
>  wrote:
> > Are you planning to work on this in this CF?
> > This is marked as "Ready for committer" but it doesn't apply anymore.
> 
> I've attached an updated patch. Since this patch is pretty different
> from the one that was marked as ready for committer, I'll move this to
> needs review.
> 
> However, I'm a bit disinclined to go ahead with this patch at all.
> Thomas made it quite clear it's not for the patch, and on discussing
> the patch with Andres, it turned out he does not like the idea either.
> Andres' argument was along the lines of bitmaps being slow.  The hash
> table uses bitmaps to record which items in each segment are in use. I
> don't really agree with him about that, so we'd likely need some more
> comments to help reach a consensus about if we want this or not.
> 
> Maybe Andres has more comments, so I've included him here.
> 

Hi David,

Thanks for the updated patch.

Based on your comments I will mark this patch as withdrawn at midday of 
my monday unless someone objects to that.

-- 
Jaime Casanova
Director de Servicios Profesionales
SystemGuards - Consultores de PostgreSQL

Re: Function scan FDW pushdown

[Alexander Pyhalov]

Ashutosh Bapat писал 2021-06-15 16:15:

Hi Alexander,


Hi.

The current version of the patch is based on asymetric partition-wise 
join.
Currently it is applied after 
v19-0001-Asymmetric-partitionwise-join.patch from
on 
https://www.postgresql.org/message-id/792d60f4-37bc-e6ad-68ca-c2af5cbb2...@postgrespro.ru 
.


So far I don't know how to visualize actual function expression used 
in

function RTE, as in postgresExplainForeignScan() es->rtable comes from
queryDesc->plannedstmt->rtable, and rte->functions is already 0.


The actual function expression will be part of the Remote SQL of
ForeignScan node so no need to visualize it separately.


We still need to create tuple description for functions in 
get_tupdesc_for_join_scan_tuples(),
so I had to remove setting newrte->functions to NIL in 
add_rte_to_flat_rtable().

With rte->functions in place, there's no issues for explain.



The patch will have problems when there are multiple foreign tables
all on different servers or use different FDWs. In such a case the
function scan's RelOptInfo will get the fpinfo based on the first
foreign table the function scan is paired with during join planning.
But that may not be the best foreign table to join. We should be able
to plan all the possible joins. Current infra to add one fpinfo per
RelOptInfo won't help there. We need something better.


I suppose attached version of the patch is more mature.



The patch targets only postgres FDW, how do you see this working with
other FDWs?


Not now. We introduce necessary APIs for other FDWs, but implementing 
TryShippableJoinPaths()

doesn't seem straightforward.



If we come up with the right approach we could use it for 1. pushing
down queries with IN () clause 2. joining a small local table with a
large foreign table by sending the local table rows down to the
foreign server.



--
Best regards,
Alexander Pyhalov,
Postgres ProfessionalFrom d997c313daf0031b812d3fca59d338be1a4f2196 Mon Sep 17 00:00:00 2001
From: Alexander Pyhalov 
Date: Mon, 17 May 2021 19:19:31 +0300
Subject: [PATCH] Push join with function scan to remote server

---
 contrib/postgres_fdw/deparse.c|  199 ++-
 .../postgres_fdw/expected/postgres_fdw.out| 1095 +
 contrib/postgres_fdw/postgres_fdw.c   |  497 +++-
 contrib/postgres_fdw/postgres_fdw.h   |6 +
 contrib/postgres_fdw/sql/postgres_fdw.sql |  336 +
 src/backend/optimizer/path/joinpath.c |   11 +
 src/backend/optimizer/plan/setrefs.c  |1 -
 src/backend/optimizer/util/relnode.c  |2 +
 src/include/foreign/fdwapi.h  |1 +
 9 files changed, 2035 insertions(+), 113 deletions(-)

diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c
index d98bd666818..7f08575ef60 100644
--- a/contrib/postgres_fdw/deparse.c
+++ b/contrib/postgres_fdw/deparse.c
@@ -151,6 +151,7 @@ static void deparseConst(Const *node, deparse_expr_cxt *context, int showtype);
 static void deparseParam(Param *node, deparse_expr_cxt *context);
 static void deparseSubscriptingRef(SubscriptingRef *node, deparse_expr_cxt *context);
 static void deparseFuncExpr(FuncExpr *node, deparse_expr_cxt *context);
+static void deparseFuncColnames(StringInfo buf, int varno, RangeTblEntry *rte, bool qualify_col);
 static void deparseOpExpr(OpExpr *node, deparse_expr_cxt *context);
 static void deparseOperatorName(StringInfo buf, Form_pg_operator opform);
 static void deparseDistinctExpr(DistinctExpr *node, deparse_expr_cxt *context);
@@ -1740,13 +1741,54 @@ deparseFromExprForRel(StringInfo buf, PlannerInfo *root, RelOptInfo *foreignrel,
 	{
 		RangeTblEntry *rte = planner_rt_fetch(foreignrel->relid, root);
 
-		/*
-		 * Core code already has some lock on each rel being planned, so we
-		 * can use NoLock here.
-		 */
-		Relation	rel = table_open(rte->relid, NoLock);
+		Assert(rte->rtekind == RTE_RELATION || rte->rtekind == RTE_FUNCTION);
+		if (rte->rtekind == RTE_RELATION)
+		{
+			/*
+			 * Core code already has some lock on each rel being planned, so
+			 * we can use NoLock here.
+			 */
+			Relation	rel = table_open(rte->relid, NoLock);
 
-		deparseRelation(buf, rel);
+			deparseRelation(buf, rel);
+
+			table_close(rel, NoLock);
+		}
+		else if (rte->rtekind == RTE_FUNCTION)
+		{
+			RangeTblFunction *rtfunc;
+			deparse_expr_cxt context;
+			ListCell   *lc;
+			bool		first = true;
+			int			n;
+
+			n = list_length(rte->functions);
+			Assert(n >= 1);
+
+			if (n > 1)
+appendStringInfoString(buf, "ROWS FROM (");
+
+			foreach(lc, rte->functions)
+			{
+if (!first)
+	appendStringInfoString(buf, ", ");
+else
+	first = false;
+
+rtfunc = (RangeTblFunction *) lfirst(lc);
+
+context.root = root;
+context.foreignrel = foreignrel;
+context.scanrel = foreignrel;
+context.buf = buf;
+context.params_list = params_list;
+
+deparseExpr((Expr *) rtfunc->funcexpr, &context);
+			}
+
+			if (n > 1)
+appendStringInfoStr

Remove an obsolete comment in snapbuild.c

[Masahiko Sawada]

Hi all,

While reading the code I realized that the following comment of
SnapBuildOnDick is obsolete:

/*
 * We store current state of struct SnapBuild on disk in the following manner:
 *
 * struct SnapBuildOnDisk;
 * TransactionId * running.xcnt_space;
 * TransactionId * committed.xcnt; (*not xcnt_space*)
 *
 */
typedef struct SnapBuildOnDisk

Since SnapBuild has no longer "running" struct, it should be removed.
Please find an attached patch.

Regards,

-- 
Masahiko Sawada
EDB:  https://www.enterprisedb.com/


fix_obsolete_comment_in_snapbuild.c.patch
Description: Binary data

replace InvalidXid(a macro that doesn't exist) with InvalidTransactionId(a macro that exists) in code comments

[Bharath Rupireddy]

Hi,

It seems like we have macro InvalidTransactionId but InvalidXid is
used in some of the code comments.Here's a small patch that does
$subject?

Regards,
Bharath Rupireddy.


v1-0001-replace-InvalidXid-with-InvalidTransactionId-in-c.patch
Description: Binary data

Re: [Patch] ALTER SYSTEM READ ONLY

[Rushabh Lathia]

On Fri, Oct 1, 2021 at 2:29 AM Robert Haas  wrote:

> On Thu, Sep 30, 2021 at 7:59 AM Amul Sul  wrote:
> > To find the value of InRecovery after we clear it, patch still uses
> > ControlFile's DBState, but now the check condition changed to a more
> > specific one which is less confusing.
> >
> > In casual off-list discussion, the point was made to check
> > SharedRecoveryState to find out the InRecovery value afterward, and
> > check that using RecoveryInProgress().  But we can't depend on
> > SharedRecoveryState because at the start it gets initialized to
> > RECOVERY_STATE_CRASH irrespective of InRecovery that happens later.
> > Therefore, we can't use RecoveryInProgress() which always returns
> > true if SharedRecoveryState != RECOVERY_STATE_DONE.
>
> Uh, this change has crept into 0002, but it should be in 0004 with the
> rest of the changes to remove dependencies on variables specific to
> the startup process. Like I said before, we should really be trying to
> separate code movement from functional changes. Also, 0002 doesn't
> actually apply for me. Did you generate these patches with 'git
> format-patch'?
>
> [rhaas pgsql]$ patch -p1 <
> ~/Downloads/v36-0001-Refactor-some-end-of-recovery-code-out-of-Startu.patch
> patching file src/backend/access/transam/xlog.c
> Hunk #1 succeeded at 889 (offset 9 lines).
> Hunk #2 succeeded at 939 (offset 12 lines).
> Hunk #3 succeeded at 5734 (offset 37 lines).
> Hunk #4 succeeded at 8038 (offset 70 lines).
> Hunk #5 succeeded at 8248 (offset 70 lines).
> [rhaas pgsql]$ patch -p1 <
> ~/Downloads/v36-0002-Postpone-some-end-of-recovery-operations-relatin.patch
> patching file src/backend/access/transam/xlog.c
> Reversed (or previously applied) patch detected!  Assume -R? [n]
> Apply anyway? [n] y
> Hunk #1 FAILED at 7954.
> Hunk #2 succeeded at 8079 (offset 70 lines).
> 1 out of 2 hunks FAILED -- saving rejects to file
> src/backend/access/transam/xlog.c.rej
> [rhaas pgsql]$ git reset --hard
> HEAD is now at b484ddf4d2 Treat ETIMEDOUT as indicating a
> non-recoverable connection failure.
> [rhaas pgsql]$ patch -p1 <
> ~/Downloads/v36-0002-Postpone-some-end-of-recovery-operations-relatin.patch
> patching file src/backend/access/transam/xlog.c
> Reversed (or previously applied) patch detected!  Assume -R? [n]
> Apply anyway? [n]
> Skipping patch.
> 2 out of 2 hunks ignored -- saving rejects to file
> src/backend/access/transam/xlog.c.rej
>
>
I tried to apply the patch on the master branch head and it's failing
with conflicts.

Later applied patch on below commit and it got applied cleanly:

commit 7d1aa6bf1c27bf7438179db446f7d1e72ae093d0
Author: Tom Lane 
Date:   Mon Sep 27 18:48:01 2021 -0400

Re-enable contrib/bloom's TAP tests.

rushabh@rushabh:postgresql$ git apply
v36-0001-Refactor-some-end-of-recovery-code-out-of-Startu.patch
rushabh@rushabh:postgresql$ git apply
v36-0002-Postpone-some-end-of-recovery-operations-relatin.patch
rushabh@rushabh:postgresql$ git apply
v36-0003-Create-XLogAcceptWrites-function-with-code-from-.patch
v36-0003-Create-XLogAcceptWrites-function-with-code-from-.patch:34: space
before tab in indent.
  /*
v36-0003-Create-XLogAcceptWrites-function-with-code-from-.patch:38: space
before tab in indent.
  */
v36-0003-Create-XLogAcceptWrites-function-with-code-from-.patch:39: space
before tab in indent.
  Insert->fullPageWrites = lastFullPageWrites;
warning: 3 lines add whitespace errors.
rushabh@rushabh:postgresql$ git apply
v36-0004-Remove-dependencies-on-startup-process-specifica.patch

There are whitespace errors on patch 0003.


> It seems to me that the approach you're pursuing here can't work,
> because the long-term goal is to get to a place where, if the system
> starts up read-only, XLogAcceptWrites() might not be called until
> later, after StartupXLOG() has exited. But in that case the control
> file state would be DB_IN_PRODUCTION. But my idea of using
> RecoveryInProgress() won't work either, because we set
> RECOVERY_STATE_DONE just after we set DB_IN_PRODUCTION. Put
> differently, the question we want to answer is not "are we in recovery
> now?" but "did we perform recovery?". After studying the code a bit, I
> think a good test might be
> !XLogRecPtrIsInvalid(XLogCtl->lastReplayedEndRecPtr). If InRecovery
> gets set to true, then we're certain to enter the if (InRecovery)
> block that contains the main redo loop. And that block unconditionally
> does XLogCtl->lastReplayedEndRecPtr = XLogCtl->replayEndRecPtr. I
> think that replayEndRecPtr can't be 0 because it's supposed to
> represent the record we're pretending to have last replayed, as
> explained by the comments. And while lastReplayedEndRecPtr will get
> updated later as we replay more records, I think it will never be set
> back to 0. It's only going to increase, as we replay more records. On
> the other hand if InRecovery = false then we'll never change it, and
> it seems that it starts out as 0.
>
> I was hoping to have more time today to comment on 0004, but the day

Re: Incorrect snapshots while promoting hot standby node when 2PC is used

[Michael Paquier]

On Fri, Oct 01, 2021 at 02:11:15PM +0900, Michael Paquier wrote:
> A couple of months later, I have looked back at this thread and this
> report.  I have rechecked all the standby handling and snapshot builds
> involving KnownAssignedXids and looked at all the phases that are
> getting called until we call ShutdownRecoveryTransactionEnvironment()
> to release these, and I don't think that there is a problem with the
> solution proposed here.  So I propose to move on and apply this
> patch.  Please let me know if there are any objections.

Okay, I have worked more on that today, did more tests and applied the
fix as of 8a42379.
--
Michael


signature.asc
Description: PGP signature

Re: Remove an obsolete comment in snapbuild.c

[Amit Kapila]

On Mon, Oct 4, 2021 at 1:24 PM Masahiko Sawada  wrote:
>
> Hi all,
>
> While reading the code I realized that the following comment of
> SnapBuildOnDick is obsolete:
>
> /*
>  * We store current state of struct SnapBuild on disk in the following manner:
>  *
>  * struct SnapBuildOnDisk;
>  * TransactionId * running.xcnt_space;
>  * TransactionId * committed.xcnt; (*not xcnt_space*)
>  *
>  */
> typedef struct SnapBuildOnDisk
>
> Since SnapBuild has no longer "running" struct, it should be removed.
> Please find an attached patch.
>

LGTM. I'll push this tomorrow unless someone thinks otherwise.

-- 
With Regards,
Amit Kapila.

Re: replace InvalidXid(a macro that doesn't exist) with InvalidTransactionId(a macro that exists) in code comments

[Daniel Gustafsson]

> On 4 Oct 2021, at 10:19, Bharath Rupireddy 
>  wrote:

> It seems like we have macro InvalidTransactionId but InvalidXid is
> used in some of the code comments.Here's a small patch that does
> $subject?

While I doubt anyone would be confused by these, I do agree it's worth being
consistent and use the right terms. Pushed to master, thanks!

--
Daniel Gustafsson   https://vmware.com/

Re: BUG #17212: pg_amcheck fails on checking temporary relations

[Alexander Lakhin]

Hello Mark,

04.10.2021 01:20, Mark Dilger wrote:
> The attached patch includes a test case for this, which shows the problems 
> against the current pg_amcheck.c, and a new version of pg_amcheck.c which 
> fixes the bug.  Could you review it?
>
> Thanks for bringing this to my attention.
There is another issue, that maybe should be discussed separately (or
this thread could be renamed to "... on checking specific relations"),
but the solution could be similar to that.
pg_amcheck also fails on checking invalid indexes, that could be created
legitimately by the CREATE INDEX CONCURRENTLY command.
For example, consider the following script:
psql -c "CREATE TABLE t(i numeric); INSERT INTO t VALUES
(generate_series(1, 1000));"
psql -c "CREATE INDEX CONCURRENTLY t_idx ON t(i);" &
pg_amcheck -a --install-missing --heapallindexed --rootdescend
--progress || echo "FAIL"

pg_amcheck fails with:
btree index "regression.public.t_idx":
    ERROR:  cannot check index "t_idx"
    DETAIL:  Index is not valid.
781/781 relations (100%), 2806/2806 pages (100%)
FAIL

When an index created without CONCURRENTLY, it runs successfully.

Beside that, it seems that pg_amcheck produces a deadlock in such a case:
2021-10-04 11:23:38.584 MSK [1451296] ERROR:  deadlock detected
2021-10-04 11:23:38.584 MSK [1451296] DETAIL:  Process 1451296 waits for
ShareLock on virtual transaction 5/542; blocked by process 1451314.
    Process 1451314 waits for ShareLock on relation 16385 of database
16384; blocked by process 1451296.
    Process 1451296: CREATE INDEX CONCURRENTLY t_idx ON t(i);
    Process 1451314: SELECT * FROM
"pg_catalog".bt_index_parent_check(index := '16390'::regclass,
heapallindexed := true, rootdescend := true)
2021-10-04 11:23:38.584 MSK [1451296] HINT:  See server log for query
details.
2021-10-04 11:23:38.584 MSK [1451296] STATEMENT:  CREATE INDEX
CONCURRENTLY t_idx ON t(i);

I think that the deadlock is yet another issue, as invalid indexes could
appear in other circumstances too.

Best regards,
Alexander

Re: [Proposal] Fully WAL logged CREATE DATABASE - No Checkpoints

[Dilip Kumar]

On Mon, Sep 27, 2021 at 12:23 PM Dilip Kumar  wrote:
>
>
> Open question:
> - Scan pg_class vs scan directories
> - Whether to retain the old created database mechanism as option or not.

I have done some code improvement in 0001 and 0002.

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
From fce1a87e25d20bf4b1a85c6cc535db42b5bdfc73 Mon Sep 17 00:00:00 2001
From: Dilip Kumar 
Date: Wed, 1 Sep 2021 14:06:29 +0530
Subject: [PATCH v4 1/6] Refactor relmap load and relmap write functions

Currently, write_relmap_file and load_relmap_file are tightly
coupled with shared_map and local_map.  As part of the higher
level patch set we need remap read/write interfaces that are
not dependent upon shared_map and local_map, and we should be
able to pass map memory as an external parameter instead.
---
 src/backend/utils/cache/relmapper.c | 163 ++--
 1 file changed, 99 insertions(+), 64 deletions(-)

diff --git a/src/backend/utils/cache/relmapper.c b/src/backend/utils/cache/relmapper.c
index a6e38ad..bb39632 100644
--- a/src/backend/utils/cache/relmapper.c
+++ b/src/backend/utils/cache/relmapper.c
@@ -136,6 +136,12 @@ static void apply_map_update(RelMapFile *map, Oid relationId, Oid fileNode,
 			 bool add_okay);
 static void merge_map_updates(RelMapFile *map, const RelMapFile *updates,
 			  bool add_okay);
+static void read_relmap_file(char *mapfilename, RelMapFile *map,
+			 bool lock_held);
+static void write_relmap_file_internal(char *mapfilename, RelMapFile *newmap,
+	   bool write_wal, bool send_sinval,
+	   bool preserve_files, Oid dbid, Oid tsid,
+	   const char *dbpath);
 static void load_relmap_file(bool shared, bool lock_held);
 static void write_relmap_file(bool shared, RelMapFile *newmap,
 			  bool write_wal, bool send_sinval, bool preserve_files,
@@ -687,36 +693,19 @@ RestoreRelationMap(char *startAddress)
 }
 
 /*
- * load_relmap_file -- load data from the shared or local map file
+ * read_relmap_file -- read data from given mapfilename file.
  *
  * Because the map file is essential for access to core system catalogs,
  * failure to read it is a fatal error.
- *
- * Note that the local case requires DatabasePath to be set up.
  */
 static void
-load_relmap_file(bool shared, bool lock_held)
+read_relmap_file(char *mapfilename, RelMapFile *map, bool lock_held)
 {
-	RelMapFile *map;
-	char		mapfilename[MAXPGPATH];
 	pg_crc32c	crc;
 	int			fd;
 	int			r;
 
-	if (shared)
-	{
-		snprintf(mapfilename, sizeof(mapfilename), "global/%s",
- RELMAPPER_FILENAME);
-		map = &shared_map;
-	}
-	else
-	{
-		snprintf(mapfilename, sizeof(mapfilename), "%s/%s",
- DatabasePath, RELMAPPER_FILENAME);
-		map = &local_map;
-	}
-
-	/* Read data ... */
+	/* Open the relmap file for reading. */
 	fd = OpenTransientFile(mapfilename, O_RDONLY | PG_BINARY);
 	if (fd < 0)
 		ereport(FATAL,
@@ -779,62 +768,50 @@ load_relmap_file(bool shared, bool lock_held)
 }
 
 /*
- * Write out a new shared or local map file with the given contents.
- *
- * The magic number and CRC are automatically updated in *newmap.  On
- * success, we copy the data to the appropriate permanent static variable.
- *
- * If write_wal is true then an appropriate WAL message is emitted.
- * (It will be false for bootstrap and WAL replay cases.)
- *
- * If send_sinval is true then a SI invalidation message is sent.
- * (This should be true except in bootstrap case.)
- *
- * If preserve_files is true then the storage manager is warned not to
- * delete the files listed in the map.
+ * load_relmap_file -- load data from the shared or local map file
  *
- * Because this may be called during WAL replay when MyDatabaseId,
- * DatabasePath, etc aren't valid, we require the caller to pass in suitable
- * values.  The caller is also responsible for being sure no concurrent
- * map update could be happening.
+ * Note that the local case requires DatabasePath to be set up.
  */
 static void
-write_relmap_file(bool shared, RelMapFile *newmap,
-  bool write_wal, bool send_sinval, bool preserve_files,
-  Oid dbid, Oid tsid, const char *dbpath)
+load_relmap_file(bool shared, bool lock_held)
 {
-	int			fd;
-	RelMapFile *realmap;
+	RelMapFile *map;
 	char		mapfilename[MAXPGPATH];
 
-	/*
-	 * Fill in the overhead fields and update CRC.
-	 */
-	newmap->magic = RELMAPPER_FILEMAGIC;
-	if (newmap->num_mappings < 0 || newmap->num_mappings > MAX_MAPPINGS)
-		elog(ERROR, "attempt to write bogus relation mapping");
-
-	INIT_CRC32C(newmap->crc);
-	COMP_CRC32C(newmap->crc, (char *) newmap, offsetof(RelMapFile, crc));
-	FIN_CRC32C(newmap->crc);
-
-	/*
-	 * Open the target file.  We prefer to do this before entering the
-	 * critical section, so that an open() failure need not force PANIC.
-	 */
 	if (shared)
 	{
 		snprintf(mapfilename, sizeof(mapfilename), "global/%s",
  RELMAPPER_FILENAME);
-		realmap = &shared_map;
+		map = &shared_map;
 	}
 	else
 	{
 		snprintf(mapfilename, sizeof(map

Re: Better context for "TAP tests not enabled" error message

[Daniel Gustafsson]

> On 3 Oct 2021, at 01:27, Daniel Gustafsson  wrote:
> 
>> On 3 Oct 2021, at 00:39, Kevin Burke  wrote:
> 
>> Updated patch that removes the "Maybe" 
> 
> Thanks, I’ll take care of this tomorrow along with Rachels patch.

I was off-by-one on date, but it's been pushed to master now. Thanks!

--
Daniel Gustafsson   https://vmware.com/

Re: 2021-09 Commitfest

[Magnus Hagander]

On Sun, Oct 3, 2021 at 3:48 PM Tom Lane  wrote:

> Magnus Hagander  writes:
> > On Sat, Oct 2, 2021 at 7:31 AM Michael Paquier 
> wrote:
> >> That's the tricky part.  It does not really make sense either to keep
> >> moving patches that are waiting on author for months.
>
> > I'm pretty sure this is the original reason for adding this -- to enforce
> > that this review happens.
>
> The CF tool is in no way able to enforce that, though.  All it's doing
> is making extra work for the CFM.
>

I've now deployed this:
https://git.postgresql.org/gitweb/?p=pgcommitfest2.git;a=commitdiff;h=65073ba7614ba539aff961e59c9eddbbb8d095f9


-- 
 Magnus Hagander
 Me: https://www.hagander.net/ 
 Work: https://www.redpill-linpro.com/ 

Re: VS2022: Support Visual Studio 2022 on Windows

[Laurenz Albe]

On Fri, 2021-10-01 at 15:15 +, Hans Buschmann wrote:
> During testing of the new Visual Studio 2022 Preview Version 4.1 from 
> Microsoft I also tried PG14.0 on it.
> The x64 version built without error!.
> 
> Even when this is only a preview version (the real thing is to expected soon) 
> it seems appropriate to include the support to Postgres msvc tools directory.
> 
> I followed the guideline of the patch msvc-2019-support-v4.patch for VS2019 
> support. New patch attached.
[...]
> HELP NEEDED:
> 
> Please could somebody test the patch and enter it to the next commit fest?

Thanks for that work; help with Windows is always welcome.

Please go ahead and add the patch to the commitfest yourself.
Testing will (hopefully) be done by a reviewer who has access to MSVC 2022.

Yours,
Laurenz Albe

ALTER INDEX .. RENAME allows to rename tables/views as well

[Onder Kalaci]

Hi hackers,

I realized a subtle behavior with ALTER INDEX .. RENAME. It seems like a bug to 
me, please see the steps below.

Test 1: Rename table via RENAME .. INDEX

CREATE TABLE test_table (a int);
SELECT 'test_table'::regclass::oid;
  oid
---
34470
(1 row)
-- rename table using ALTER INDEX ..
ALTER INDEX test_table RENAME TO test_table_2;

-- see that table is rename
SELECT 34470::regclass;
   regclass
--
test_table_2
(1 row)


Test 2: Rename view via RENAME .. INDEX
CREATE VIEW test_view AS SELECT * FROM pg_class;
SELECT 'test_view'::regclass::oid;
  oid
---
34473
(1 row)

ALTER INDEX test_view RENAME TO test_view_2;
ELECT 34473::regclass;
  regclass
-
test_view_2
(1 row)


It seems like an oversight in ExecRenameStmt(), and probably applies to 
sequences, mat. views and foreign tables as well.

I can reproduce this on both 13.2 and 14.0. Though haven’t checked earlier 
versions.

Thanks,
Onder

Re: Added schema level support for publication.

[Amit Kapila]

On Sun, Oct 3, 2021 at 11:25 PM vignesh C  wrote:
>
> On Sat, Oct 2, 2021 at 1:13 PM Amit Kapila  wrote:
> >
>
> > 2. In GetSchemaPublicationRelations(), I think we need to perform a
> > second scan using RELKIND_PARTITIONED_TABLE only if we
> > publish_via_root (aka pub_partopt is PUBLICATION_PART_ROOT). This is
> > what we are doing in GetAllTablesPublicationRelations. Is there a
> > reason to be different here?
>
> In the first table scan we are getting all the ordinary tables present
> in the schema. In the second table scan we will get all the
> partitioned table present in the schema and the relations will be
> added based on pub_partopt. I felt if we have the check we will not
> get the relations in the following case:
> create schema sch1;
> create schema sch2;
> create table sch1.p (a int) partition by list (a);
> create table sch2.c1 partition of sch1.p for values in (1);
>

But we don't need to get the partitioned tables for the invalidations,
see the corresponding case for tables. So, I am not sure why you have
used two scans to the system table for such scenarios?

Few additional comments on
v36-0002-Client-side-changes-to-support-FOR-ALL-TABLES-IN:
=
1.
@@ -3961,21 +3965,25 @@ getPublications(Archive *fout, int *numPublications)
  appendPQExpBuffer(query,
"SELECT p.tableoid, p.oid, p.pubname, "
"(%s p.pubowner) AS rolname, "
-   "p.puballtables, p.pubinsert, p.pubupdate, p.pubdelete,
p.pubtruncate, p.pubviaroot "
+   "p.puballtables, p.pubinsert, p.pubupdate, "
+   "p.pubdelete, p.pubtruncate, p.pubviaroot "
"FROM pg_publication p",
username_subquery);
  else if (fout->remoteVersion >= 11)
  appendPQExpBuffer(query,
"SELECT p.tableoid, p.oid, p.pubname, "
"(%s p.pubowner) AS rolname, "
-   "p.puballtables, p.pubinsert, p.pubupdate, p.pubdelete,
p.pubtruncate, false AS pubviaroot "
+   "p.puballtables, p.pubinsert, p.pubupdate, "
+   "p.pubdelete, p.pubtruncate, false AS pubviaroot "
"FROM pg_publication p",
username_subquery);
  else
  appendPQExpBuffer(query,
"SELECT p.tableoid, p.oid, p.pubname, "
"(%s p.pubowner) AS rolname, "
-   "p.puballtables, p.pubinsert, p.pubupdate, p.pubdelete, false AS
pubtruncate, false AS pubviaroot "
+   "p.puballtables, p.pubinsert, p.pubupdate, "
+   "p.pubdelete, false AS pubtruncate, "
+   "false AS pubviaroot "
"FROM pg_publication p",
username_subquery);

Is there a reason to change this part of the code?

2.
@@ -257,6 +257,9 @@ getSchemaData(Archive *fout, int *numTablesPtr)
  pg_log_info("reading publication membership");
  getPublicationTables(fout, tblinfo, numTables);

+ pg_log_info("reading publication tables in schemas");
+ getPublicationNamespaces(fout, nspinfo, numNamespaces);

I think for the above change, the first should be changed to "reading
publication membership of tables" and the second one should be changed
to "reading publication membership of schemas".

3. The function names getPublicationNamespaces and
dumpPublicationSchema are not in sync. Let's name the second one as
dumpPublicationNamespace.

4. It is not clear to me why the patch has introduced a new component
type object DUMP_COMPONENT_PUBSCHEMA. In particular, in the below code
if we are already setting DUMP_COMPONENT_ALL, how the additional
setting of DUMP_COMPONENT_PUBSCHEMA helps?

@@ -1631,9 +1631,13 @@ selectDumpableNamespace(NamespaceInfo *nsinfo,
Archive *fout)
  if (nsinfo->nspowner == ROLE_PG_DATABASE_OWNER)
  nsinfo->dobj.dump &= ~DUMP_COMPONENT_DEFINITION;
  nsinfo->dobj.dump_contains = DUMP_COMPONENT_ALL;
+ nsinfo->dobj.dump |= DUMP_COMPONENT_PUBSCHEMA;
  }
  else
+ {
  nsinfo->dobj.dump_contains = nsinfo->dobj.dump = DUMP_COMPONENT_ALL;
+ nsinfo->dobj.dump |= DUMP_COMPONENT_PUBSCHEMA;
+ }

-- 
With Regards,
Amit Kapila.

Re: Triage on old commitfest entries - JSON_PATH

[Andrew Dunstan]

On 10/3/21 3:56 PM, Erik Rijkers wrote:
> Op 03-10-2021 om 21:14 schreef Tom Lane:
>> As I threatened in another thread, I've looked through all of the
>> oldest commitfest entries to see which ones should maybe be tossed,
>> on the grounds that they're unlikely to ever get committed so we
>> should stop pushing them forward to the next CF.
>>
>> An important note to make here is that we don't have any explicit
>> mechanism for saying "sorry, this patch is perhaps useful but it
>> seems that nobody is going to take an interest in it".  Closing
>> such a patch as "rejected" seems harsh, but R-W-F isn't very
>> appropriate either if the patch never got any real review.
>> Perhaps we should create a new closure state?
>>
>> I looked at entries that are at least 10 CFs old, as indicated by
>> the handy sort field.  That's a pretty small population: 16 items
>> out of the 317 listed in the 2021-09 CF.  A quick look in recent
>> CFs shows that it's very rare that we commit entries older than
>> 10 CFs.
>>
>> Here's what I found, along with some commentary about each one.
>>
>> Patch    Age in CFs
>
> May I add one more?
>
> SQL/JSON: JSON_TABLE started 2018 (the commitfest page shows only 4 as
> 'Age in CFs' but that obviously can't be right)
>
> Although I like the patch & new functionality and Andrew Dunstan has
> worked to keep it up-to-date, there seems to be very little further
> discussion. I makes me a little worried that the time I put in will
> end up sunk in a dead project.
>
>


I'm working on the first piece of it, i.e. the SQL/JSON functions.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: VS2022: Support Visual Studio 2022 on Windows

[Andrew Dunstan]

On 10/4/21 6:13 AM, Laurenz Albe wrote:
> On Fri, 2021-10-01 at 15:15 +, Hans Buschmann wrote:
>> During testing of the new Visual Studio 2022 Preview Version 4.1 from 
>> Microsoft I also tried PG14.0 on it.
>> The x64 version built without error!.
>>
>> Even when this is only a preview version (the real thing is to expected 
>> soon) it seems appropriate to include the support to Postgres msvc tools 
>> directory.
>>
>> I followed the guideline of the patch msvc-2019-support-v4.patch for VS2019 
>> support. New patch attached.
> [...]
>> HELP NEEDED:
>>
>> Please could somebody test the patch and enter it to the next commit fest?
> Thanks for that work; help with Windows is always welcome.
>
> Please go ahead and add the patch to the commitfest yourself.
> Testing will (hopefully) be done by a reviewer who has access to MSVC 2022.
>

I think we'll want to wait for the official release before we add
support for it.


cheers


andew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: [PATCH] Print error when libpq-refs-stamp fails

[Daniel Gustafsson]

> On 28 Sep 2021, at 17:52, Rachel Heaton  wrote:

> Patch attached.

I tweaked the error message a little bit and pushed to master. Thanks!

--
Daniel Gustafsson   https://vmware.com/

Re: Triage on old commitfest entries

[Jesper Pedersen]

Hi,

On 10/3/21 16:18, Peter Geoghegan wrote:

Index Skip Scan 16
 Last substantive discussion 2021-05, currently passing cfbot

 Seems possibly useful, but we're not making progress.

This feature is definitely useful. My pet theory is that it hasn't
made more progress because it requires expertise in two fairly
distinct areas of the system. There is a lot of B-Tree stuff here,
which is clearly my thing. But I know that I personally am much less
likely to work on a patch that requires significant changes to the
planner. Maybe this is a coordination problem.



I still believe that this is an important user-visible improvement.


However, there has been conflicting feedback on the necessary planner 
changes leading to doing double work in order to figure the best way 
forward.



Dmitry and Andy are doing a good job on keeping the patches current, but 
maybe there needs to be a firm decision from a committer on what the 
planner changes should look like before these patches can move forward.



So, is RfC the best state for that ?


Best regards,

 Jesper

Re: 2021-09 Commitfest

[Daniel Gustafsson]

> On 4 Oct 2021, at 12:06, Magnus Hagander  wrote:
> 
> On Sun, Oct 3, 2021 at 3:48 PM Tom Lane  > wrote:
> Magnus Hagander mailto:mag...@hagander.net>> writes:
> > On Sat, Oct 2, 2021 at 7:31 AM Michael Paquier  > > wrote:
> >> That's the tricky part.  It does not really make sense either to keep
> >> moving patches that are waiting on author for months.
> 
> > I'm pretty sure this is the original reason for adding this -- to enforce
> > that this review happens.
> 
> The CF tool is in no way able to enforce that, though.  All it's doing
> is making extra work for the CFM.
> 
> I've now deployed this: 
> https://git.postgresql.org/gitweb/?p=pgcommitfest2.git;a=commitdiff;h=65073ba7614ba539aff961e59c9eddbbb8d095f9
>  
> 
AFAICT this should now allow for WoA patches to be moved to the next CF, but
trying that on a patch in the current CF failed with "Invalid existing patch
status" in a red topbar.  Did I misunderstand what this change was?

--
Daniel Gustafsson   https://vmware.com/

RE: Allow escape in application_name

[kuroda.hay...@fujitsu.com]

Dear Horiguchi-san,

Thank you for giving many comments! I attached new patches.
I'm sorry for the late reply.

> I think we don't have a predecessor of the case like this where a
> behavior is decided from object option and GUC.
> 
> I'm a bit uncomfortable with .conf configuration overrides server
> options, but I also think in-session-set GUC should override server
> options.  So, it's slightly against POLA but from the standpoint of
> usability +0.5 to that prioritization since I cannot come up with a
> better idea.

OK, I keep the current spec. Please tell me if you come up with something.

> I thought it is nice to share process_format_string but the function
> is too tightly coupled with ErrorData detail as you pointed off-list.
> So I came to think we cannot use the function from outside.  It could
> be a possibility to make the function be just a skeleton that takes a
> list of pairs of an escaped character and the associated callback
> function but that is apparently too-much complex.  (It would be worth
> doing if we share the same backbone processing with archive_command,
> restore_command, recover_end_command and so on, but that is
> necessarily accompanied by the need to change the behavior either
> log_line_prefix or others.)

I agree that combining them makes source too complex.
And we have an another problem about native language support.
Sometimes espaces becomes "[unkown]" string in log_line_prefix(),
and they are gettext()'s target. So if we combine log_line_prefix() and 
parse_pgfdw_appname(),
some non-ascii characters may be set to postgres_fdw.applicaiton_name.
Of cause we can implement callback function, but I think it is the next step.

> I personally don't care even if this feature implements
> padding-reading logic differently from process_format_string, but if
> we don't like that, I would return to suggest using strtol in both
> functions.
> 
> As Fujii-san pointed upthread, strtol behaves a bit different way than
> we expect here, but that can be handled by checking the starting
> characters.

>   if (*p == '-' ? isdigit(p[1]) : isdigit(p[0]))
>   {
>   char *endptr;
>   padding = strtol(p, &endptr, 10);
>   if (p == endptr)
>   break;
>   p = endptr;
>   }
>   else
>   padding = 0;

I removed the support function based on your suggestion,
but added some if-statement in order to treats the special case: '%-p'.

And I found and fixed another bug. If users set application_name 
both in server option and GUC, concatenated string was set as appname.
This is caused because we reused same StringInfo and parsing all appname string
even if we sucsess it. Now the array search will stop if sucseed,
and in failure case do resetStringInfo() and re-search.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED



v19_0002_allow_escapes.patch
Description: v19_0002_allow_escapes.patch


v19_0001_remove_padding_function.patch
Description: v19_0001_remove_padding_function.patch

Re: proposal: possibility to read dumped table's name from file

[Daniel Gustafsson]

> On 2 Oct 2021, at 08:18, Erik Rijkers  wrote:

> So the issue is not as serious as it seemed.  

This is also not related to this patch in any way, or am I missing a point
here? This can just as well be achieved without this patch.

> The complaint remaining is only that this could somehow be documented better.

The pg_dump documentation today have a large highlighted note about this:

"When --include-foreign-data is specified, pg_dump does not check that the
foreign table is writable.  Therefore, there is no guarantee that the
results of a foreign table dump can be successfully restored."

This was extensively discussed [0] when this went in, is there additional
documentation you'd like to see for this?

--
Daniel Gustafsson   https://vmware.com/

[0] 
https://postgr.es/m/lejpr01mb0185483c0079d2f651b16231e7...@lejpr01mb0185.deuprd01.prod.outlook.de

Re: 2021-09 Commitfest

[Magnus Hagander]

On Mon, Oct 4, 2021 at 2:41 PM Daniel Gustafsson  wrote:

> > On 4 Oct 2021, at 12:06, Magnus Hagander  wrote:
> >
> > On Sun, Oct 3, 2021 at 3:48 PM Tom Lane  t...@sss.pgh.pa.us>> wrote:
> > Magnus Hagander mailto:mag...@hagander.net>>
> writes:
> > > On Sat, Oct 2, 2021 at 7:31 AM Michael Paquier  > wrote:
> > >> That's the tricky part.  It does not really make sense either to keep
> > >> moving patches that are waiting on author for months.
> >
> > > I'm pretty sure this is the original reason for adding this -- to
> enforce
> > > that this review happens.
> >
> > The CF tool is in no way able to enforce that, though.  All it's doing
> > is making extra work for the CFM.
> >
> > I've now deployed this:
> https://git.postgresql.org/gitweb/?p=pgcommitfest2.git;a=commitdiff;h=65073ba7614ba539aff961e59c9eddbbb8d095f9
> <
> https://git.postgresql.org/gitweb/?p=pgcommitfest2.git;a=commitdiff;h=65073ba7614ba539aff961e59c9eddbbb8d095f9
> >
> AFAICT this should now allow for WoA patches to be moved to the next CF,
> but
> trying that on a patch in the current CF failed with "Invalid existing
> patch
> status" in a red topbar.  Did I misunderstand what this change was?
>

Ugh. i missed one of the two checks. That's what I get for not testing
properly when the change "was so simple"...

Please try again.

-- 
 Magnus Hagander
 Me: https://www.hagander.net/ 
 Work: https://www.redpill-linpro.com/ 

Duplicat-word typos in code comments

[Dagfinn Ilmari Mannsåker]

Hi hackers,

I noticed a duplicate-word typo in a comments recently, and cooked up
the following ripgrep command to find some more.

  rg --multiline --pcre2 --type=c '(?>From 7ed1ce16a37e7a63e7015cc4ef5b2b70ba915498 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= 
Date: Mon, 4 Oct 2021 13:44:51 +0100
Subject: [PATCH] Fix some duplicate words in comments

---
 src/backend/access/transam/parallel.c | 2 +-
 src/backend/catalog/heap.c| 5 ++---
 src/backend/commands/copyfromparse.c  | 2 +-
 src/backend/partitioning/partdesc.c   | 4 ++--
 src/backend/storage/buffer/bufmgr.c   | 2 +-
 src/backend/storage/ipc/standby.c | 2 +-
 src/include/access/tableam.h  | 2 +-
 7 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/src/backend/access/transam/parallel.c b/src/backend/access/transam/parallel.c
index 5fdcff2a3b..bb1881f573 100644
--- a/src/backend/access/transam/parallel.c
+++ b/src/backend/access/transam/parallel.c
@@ -1534,7 +1534,7 @@ ParallelWorkerReportLastRecEnd(XLogRecPtr last_xlog_end)
  *
  * Also explicitly detach from dsm segment so that subsystems using
  * on_dsm_detach() have a chance to send stats before the stats subsystem is
- * shut down as as part of a before_shmem_exit() hook.
+ * shut down as part of a before_shmem_exit() hook.
  *
  * One might think this could instead be solved by carefully ordering the
  * attaching to dsm segments, so that the pgstats segments get detached from
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index 83746d3fd9..41d1f3b1c3 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -3547,9 +3547,8 @@ heap_truncate_find_FKs(List *relationIds)
 		/*
 		 * If this constraint has a parent constraint which we have not seen
 		 * yet, keep track of it for the second loop, below.  Tracking parent
-		 * constraints allows us to climb up to the top-level level constraint
-		 * and look for all possible relations referencing the partitioned
-		 * table.
+		 * constraints allows us to climb up to the top-level constraint and
+		 * look for all possible relations referencing the partitioned table.
 		 */
 		if (OidIsValid(con->conparentid) &&
 			!list_member_oid(parent_cons, con->conparentid))
diff --git a/src/backend/commands/copyfromparse.c b/src/backend/commands/copyfromparse.c
index fdf57f1556..aac10165ec 100644
--- a/src/backend/commands/copyfromparse.c
+++ b/src/backend/commands/copyfromparse.c
@@ -37,7 +37,7 @@
  * the data is valid in the current encoding.
  *
  * In binary mode, the pipeline is much simpler.  Input is loaded into
- * into 'raw_buf', and encoding conversion is done in the datatype-specific
+ * 'raw_buf', and encoding conversion is done in the datatype-specific
  * receive functions, if required.  'input_buf' and 'line_buf' are not used,
  * but 'attribute_buf' is used as a temporary buffer to hold one attribute's
  * data when it's passed the receive function.
diff --git a/src/backend/partitioning/partdesc.c b/src/backend/partitioning/partdesc.c
index 9a9d6a9643..3220d4808d 100644
--- a/src/backend/partitioning/partdesc.c
+++ b/src/backend/partitioning/partdesc.c
@@ -91,8 +91,8 @@ RelationGetPartitionDesc(Relation rel, bool omit_detached)
 	 * cached descriptor too.  We determine that based on the pg_inherits.xmin
 	 * that was saved alongside that descriptor: if the xmin that was not in
 	 * progress for that active snapshot is also not in progress for the
-	 * current active snapshot, then we can use use it.  Otherwise build one
-	 * from scratch.
+	 * current active snapshot, then we can use it.  Otherwise build one from
+	 * scratch.
 	 */
 	if (omit_detached &&
 		rel->rd_partdesc_nodetached &&
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index e88e4e918b..08ebabfe96 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -666,7 +666,7 @@ ReadRecentBuffer(RelFileNode rnode, ForkNumber forkNum, BlockNumber blockNum,
 		{
 			/*
 			 * It's now safe to pin the buffer.  We can't pin first and ask
-			 * questions later, because because it might confuse code paths
+			 * questions later, because it might confuse code paths
 			 * like InvalidateBuffer() if we pinned a random non-matching
 			 * buffer.
 			 */
diff --git a/src/backend/storage/ipc/standby.c b/src/backend/storage/ipc/standby.c
index 077251c1a6..b17326bc20 100644
--- a/src/backend/storage/ipc/standby.c
+++ b/src/backend/storage/ipc/standby.c
@@ -130,7 +130,7 @@ InitRecoveryTransactionEnvironment(void)
  *
  * This must be called even in shutdown of startup process if transaction
  * tracking has been initialized. Otherwise some locks the tracked
- * transactions were holding will not be released and and may interfere with
+ * transactions were holding will not be released and may interfere with
  * the processes still running (but will exit soon later) at the exit of
  * startup process.
  */
diff --git 

Re: 2021-09 Commitfest

[Daniel Gustafsson]

> On 4 Oct 2021, at 14:56, Magnus Hagander  wrote:

> Ugh. i missed one of the two checks. That's what I get for not testing 
> properly when the change "was so simple"...
> 
> Please try again. 

It works now, I was able to move a patch (3128) over to the 2021-11 CF.  It
does bring up the below warning(?) in a blue bar when the move was performed
which at first made me think it hadn't worked.

"The status of this patch cannot be changed in this commitfest.  You must
modify it in the one where it's open!"

--
Daniel Gustafsson   https://vmware.com/

Re: Duplicat-word typos in code comments

[Daniel Gustafsson]

> On 4 Oct 2021, at 14:56, Dagfinn Ilmari Mannsåker  wrote:

> I noticed a duplicate-word typo in a comments recently, and cooked up
> the following ripgrep command to find some more.

Pushed to master, thanks!  I avoided the reflow of the comments though to make
it the minimal change.

--
Daniel Gustafsson   https://vmware.com/

Re: Duplicat-word typos in code comments

[Dagfinn Ilmari Mannsåker]

Daniel Gustafsson  writes:

>> On 4 Oct 2021, at 14:56, Dagfinn Ilmari Mannsåker  wrote:
>
>> I noticed a duplicate-word typo in a comments recently, and cooked up
>> the following ripgrep command to find some more.
>
> Pushed to master, thanks!

Thanks!

> I avoided the reflow of the comments though to make it the minimal
> change.

Fair enough. I wasn't sure myself whether to do it or not.

- ilmari

Re: Triage on old commitfest entries

[Jesper Pedersen]

On 10/3/21 16:18, Peter Geoghegan wrote:

Index Skip Scan 16
 Last substantive discussion 2021-05, currently passing cfbot

 Seems possibly useful, but we're not making progress.

This feature is definitely useful. My pet theory is that it hasn't
made more progress because it requires expertise in two fairly
distinct areas of the system. There is a lot of B-Tree stuff here,
which is clearly my thing. But I know that I personally am much less
likely to work on a patch that requires significant changes to the
planner. Maybe this is a coordination problem.



I still believe that this is an important user-visible improvement.


However, there has been conflicting feedback on the necessary planner 
changes leading to doing double work in order to figure the best way 
forward.



Dmitry and Andy are doing a good job on keeping the patches current, but 
maybe there needs to be a firm decision from a committer on what the 
planner changes should look like before these patches can move forward.



So, is RfC the best state for that ?


Best regards,

 Jesper

Re: Duplicat-word typos in code comments

[Tom Lane]

=?utf-8?Q?Dagfinn_Ilmari_Manns=C3=A5ker?=  writes:
> Daniel Gustafsson  writes:
>> I avoided the reflow of the comments though to make it the minimal
>> change.

> Fair enough. I wasn't sure myself whether to do it or not.

The next pgindent run will do it anyway (except in comment blocks
starting in column 1).

I used to think it was better to go ahead and manually reflow, if you
use an editor that makes that easy.  That way there are fewer commits
touching any one line of code, which is good when trying to review
code history.  However, now that we've got the ability to make "git
blame" ignore pgindent commits, maybe it's better to leave that sort
of mechanical cleanup to pgindent, so that the substantive patch is
easier to review.

(But I'm not sure how well the ignore-these-commits behavior actually
works for cases like this.)

regards, tom lane

Re: Triage on old commitfest entries - JSON_PATH

[Erik Rijkers]

Op 04-10-2021 om 14:19 schreef Andrew Dunstan:


On 10/3/21 3:56 PM, Erik Rijkers wrote:

Op 03-10-2021 om 21:14 schreef Tom Lane:

As I threatened in another thread, I've looked through all of the
oldest commitfest entries to see which ones should maybe be tossed,

Patch    Age in CFs


May I add one more?

SQL/JSON: JSON_TABLE started 2018 (the commitfest page shows only 4 as
'Age in CFs' but that obviously can't be right)

Although I like the patch & new functionality and Andrew Dunstan has
worked to keep it up-to-date, there seems to be very little further
discussion. I makes me a little worried that the time I put in will
end up sunk in a dead project.





I'm working on the first piece of it, i.e. the SQL/JSON functions.



Thank you. I am glad to hear that.




cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: [PATCH] Don't block HOT update by BRIN index

[Tomas Vondra]

Hi,

I took a look at this patch again to see if I can get it polished and 
fixed. Per the discussion, I've removed the rd_indexattr list and 
replaced it with a simple flag. While doing so, I noticed a couple of 
places that should have consider (init or free) rd_hotblockingattr.


Patch 0001 is the v2, 0002 removes the rd_indexattr etc.

regards

--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
>From 9728491208b9b2a9f430483b6132930d5766255f Mon Sep 17 00:00:00 2001
From: Tomas Vondra 
Date: Mon, 4 Oct 2021 16:10:38 +0200
Subject: [PATCH 2/2] fixes and remove rd_indexattr

---
 src/backend/utils/cache/relcache.c | 28 +++-
 src/include/utils/rel.h|  2 +-
 src/include/utils/relcache.h   |  1 -
 3 files changed, 8 insertions(+), 23 deletions(-)

diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index 946e8d95f3..503c9f5bcb 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -2377,10 +2377,10 @@ RelationDestroyRelation(Relation relation, bool remember_tupdesc)
 	list_free_deep(relation->rd_fkeylist);
 	list_free(relation->rd_indexlist);
 	list_free(relation->rd_statlist);
-	bms_free(relation->rd_indexattr);
 	bms_free(relation->rd_keyattr);
 	bms_free(relation->rd_pkattr);
 	bms_free(relation->rd_idattr);
+	bms_free(relation->rd_hotblockingattr);
 	if (relation->rd_pubactions)
 		pfree(relation->rd_pubactions);
 	if (relation->rd_options)
@@ -5002,7 +5002,6 @@ RelationGetIndexPredicate(Relation relation)
 Bitmapset *
 RelationGetIndexAttrBitmap(Relation relation, IndexAttrBitmapKind attrKind)
 {
-	Bitmapset  *indexattrs;		/* indexed columns */
 	Bitmapset  *uindexattrs;	/* columns in unique indexes */
 	Bitmapset  *pkindexattrs;	/* columns in the primary index */
 	Bitmapset  *idindexattrs;	/* columns in the replica identity */
@@ -5015,12 +5014,10 @@ RelationGetIndexAttrBitmap(Relation relation, IndexAttrBitmapKind attrKind)
 	MemoryContext oldcxt;
 
 	/* Quick exit if we already computed the result. */
-	if (relation->rd_indexattr != NULL)
+	if (relation->rd_attrsvalid)
 	{
 		switch (attrKind)
 		{
-			case INDEX_ATTR_BITMAP_ALL:
-return bms_copy(relation->rd_indexattr);
 			case INDEX_ATTR_BITMAP_KEY:
 return bms_copy(relation->rd_keyattr);
 			case INDEX_ATTR_BITMAP_PRIMARY_KEY:
@@ -5059,7 +5056,7 @@ restart:
 	relreplindex = relation->rd_replidindex;
 
 	/*
-	 * For each index, add referenced attributes to indexattrs.
+	 * For each index, add referenced attributes to appropriate bitmaps.
 	 *
 	 * Note: we consider all indexes returned by RelationGetIndexList, even if
 	 * they are not indisready or indisvalid.  This is important because an
@@ -5068,7 +5065,6 @@ restart:
 	 * CONCURRENTLY is far enough along that we should ignore the index, it
 	 * won't be returned at all by RelationGetIndexList.
 	 */
-	indexattrs = NULL;
 	uindexattrs = NULL;
 	pkindexattrs = NULL;
 	idindexattrs = NULL;
@@ -5137,9 +5133,6 @@ restart:
 			 */
 			if (attrnum != 0)
 			{
-indexattrs = bms_add_member(indexattrs,
-			attrnum - FirstLowInvalidHeapAttributeNumber);
-
 if (indexDesc->rd_indam->amhotblocking)
 	hotblockingattrs = bms_add_member(hotblockingattrs,
  attrnum - FirstLowInvalidHeapAttributeNumber);
@@ -5159,12 +5152,10 @@ restart:
 		}
 
 		/* Collect all attributes used in expressions, too */
-		pull_varattnos(indexExpressions, 1, &indexattrs);
 		if (indexDesc->rd_indam->amhotblocking)
 			pull_varattnos(indexExpressions, 1, &hotblockingattrs);
 
 		/* Collect all attributes in the index predicate, too */
-		pull_varattnos(indexPredicate, 1, &indexattrs);
 		if (indexDesc->rd_indam->amhotblocking)
 			pull_varattnos(indexPredicate, 1, &hotblockingattrs);
 
@@ -5196,14 +5187,11 @@ restart:
 		bms_free(pkindexattrs);
 		bms_free(idindexattrs);
 		bms_free(hotblockingattrs);
-		bms_free(indexattrs);
 
 		goto restart;
 	}
 
 	/* Don't leak the old values of these bitmaps, if any */
-	bms_free(relation->rd_indexattr);
-	relation->rd_indexattr = NULL;
 	bms_free(relation->rd_keyattr);
 	relation->rd_keyattr = NULL;
 	bms_free(relation->rd_pkattr);
@@ -5215,8 +5203,8 @@ restart:
 
 	/*
 	 * Now save copies of the bitmaps in the relcache entry.  We intentionally
-	 * set rd_indexattr last, because that's the one that signals validity of
-	 * the values; if we run out of memory before making that copy, we won't
+	 * set rd_attrsvalid last, because that's what signals validity of the
+	 * values; if we run out of memory before making that copy, we won't
 	 * leave the relcache entry looking like the other ones are valid but
 	 * empty.
 	 */
@@ -5225,14 +5213,12 @@ restart:
 	relation->rd_pkattr = bms_copy(pkindexattrs);
 	relation->rd_idattr = bms_copy(idindexattrs);
 	relation->rd_hotblockingattr = bms_copy(hotblockingattrs);
-	relation->rd_indexattr = bms_copy(indexattrs);
+	relation->rd_attrsvalid = true;
 	MemoryContex

func.sgml

[Andrew Dunstan]

At

Tom noted:

> You have to be very careful these days when applying stale patches to
> func.sgml --- there's enough duplicate boilerplate that "patch' can easily
> be fooled into dumping an addition into the wrong place. 

This is yet another indication to me that there's probably a good case
for breaking func.sgml up into sections. It is by a very large margin
the biggest file in our document sources (the next largest is less than
half the number of lines).


thoughts?


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: func.sgml

[Tom Lane]

Andrew Dunstan  writes:
> Tom noted:
>> You have to be very careful these days when applying stale patches to
>> func.sgml --- there's enough duplicate boilerplate that "patch' can easily
>> be fooled into dumping an addition into the wrong place. 

> This is yet another indication to me that there's probably a good case
> for breaking func.sgml up into sections. It is by a very large margin
> the biggest file in our document sources (the next largest is less than
> half the number of lines).

What are you envisioning ... a file per , or something else?

I'm not sure that a split-up would really fix the problem I mentioned;
but at least it'd reduce the scope for things to go into *completely*
the wrong place.

I think to make things safer for "patch", we'd have to give up a lot
of vertical space around function-table entries.  For example,
instead of

  
   

 num_nonnulls

num_nonnulls ( VARIADIC 
"any" )
integer
...
   
  

maybe

  
num_nonnulls
num_nonnulls ( VARIADIC 
"any" )
integer
...
   

In this way, there'd be something at least a little bit unique within
the first couple of lines of an entry, so that the standard amount of
context in a diff would provide some genuine indication of where a
new entry is supposed to go.

The main problem with this formatting is that I'm not sure that
anybody's editors' SGML modes would be on board with it.

regards, tom lane

Re: func.sgml

[Dagfinn Ilmari Mannsåker]

Andrew Dunstan  writes:

> At
> 
> Tom noted:
>
>> You have to be very careful these days when applying stale patches to
>> func.sgml --- there's enough duplicate boilerplate that "patch' can easily
>> be fooled into dumping an addition into the wrong place. 
>
> This is yet another indication to me that there's probably a good case
> for breaking func.sgml up into sections. It is by a very large margin
> the biggest file in our document sources (the next largest is less than
> half the number of lines).
>
> thoughts?

It would make sense to follow a similar pattern to datatype.sgml and
break out the largest sections.  I whipped up a quick awk script to get
an idea of the sizes of the sections in the file:

$ awk '$1 == "
2506 id="functions-admin">
2463 id="functions-json">
2352 id="functions-matching">
2028 id="functions-datetime">
1672 id="functions-string">
1466 id="functions-math">
1263 id="functions-geometry">
1252 id="functions-xml">
1220 id="functions-aggregate">
1165 id="functions-formatting">
1053 id="functions-textsearch">
1049 id="functions-range">
785 id="functions-binarystring">
625 id="functions-comparison">
591 id="functions-net">
552 id="functions-array">
357 id="functions-bitstring">
350 id="functions-comparisons">
348 id="functions-subquery">
327 id="functions-event-triggers">
284 id="functions-conditional">
283 id="functions-window">
282 id="functions-srf">
181 id="functions-sequence">
145 id="functions-logical">
134 id="functions-trigger">
120 id="functions-enum">
84 id="functions-statistics">
31 id="functions-uuid">

Tangentially, running the same on datatype.sgml indicates that the
datetime section might do with splitting out:

$ awk '$1 == "
701 id="datatype-numeric">
374 id="datatype-net-types">
367 id="datatype-oid">
320 id="datatype-geometric">
310 id="datatype-pseudo">
295 id="datatype-binary">
256 id="datatype-character">
245 id="datatype-textsearch">
197 id="datatype-xml">
160 id="datatype-enum">
119 id="datatype-boolean">
81 id="datatype-money">
74 id="datatype-bit">
51 id="domains">
49 id="datatype-uuid">
30 id="datatype-pg-lsn">

The existing split-out sections of datatype.sgml are:

$ wc -l json.sgml array.sgml rowtypes.sgml rangetypes.sgml | grep -v total | 
sort -rn
  1006 json.sgml
   797 array.sgml
   592 rangetypes.sgml
   540 rowtypes.sgml

The names are also rather inconsistent and vague, especially "json" and
"array". If we split the json section out of func.sgml, we might want to
rename these datatype-foo.sgml instead of foo(types).sgml, or go the
whole hog and create subdirectories and move all the sections into
separate files in them, like with reference.sgml.

- ilmari

Re: ssl tests fail on windows / slurp_file() offset doesn't work on win

[Andrew Dunstan]

On 10/3/21 1:30 PM, Andres Freund wrote:
>
>> Why did 3c5b0685b921 choose to use setFilePointer() in the first place? At
>> this point it's a perl filehandle, so we should just use perl seek?
>>
>>
>> Leaving the concrete breakage aside, I'm somewhat unhappy that there's not a
>> single comment explaining why TestLib.pm is trying to use native windows
>> APIs.
>>
>> Isn't the code as-is also "leaking" an open IO::Handle? There's a
>> CloseHandle($fHandle), but nothing is done to $fh. But perhaps there's some
>> perl magic cleaning things up? Even if so, loks like just closing $fh will
>> close the handle as well...
> I think something roughly like the attached might be a good idea. Runs locally
> on linux, and hopefully still on windows
>
> https://cirrus-ci.com/build/4857291573821440
>

Looks sane, thanks.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: BUG #17212: pg_amcheck fails on checking temporary relations

[Mark Dilger]

> On Oct 4, 2021, at 2:00 AM, Alexander Lakhin  wrote:

Thank you, Alexander, for these bug reports.

> There is another issue, that maybe should be discussed separately (or
> this thread could be renamed to "... on checking specific relations"),
> but the solution could be similar to that.
> pg_amcheck also fails on checking invalid indexes, that could be created
> legitimately by the CREATE INDEX CONCURRENTLY command.

I believe this is a bug in amcheck's btree checking functions.  Peter, can you 
take a look?

> For example, consider the following script:
> psql -c "CREATE TABLE t(i numeric); INSERT INTO t VALUES
> (generate_series(1, 1000));"
> psql -c "CREATE INDEX CONCURRENTLY t_idx ON t(i);" &
> pg_amcheck -a --install-missing --heapallindexed --rootdescend
> --progress || echo "FAIL"
> 
> pg_amcheck fails with:
> btree index "regression.public.t_idx":
> ERROR:  cannot check index "t_idx"
> DETAIL:  Index is not valid.
> 781/781 relations (100%), 2806/2806 pages (100%)
> FAIL

Yes, I can reproduce this following your steps.  (It's always appreciated to 
have steps to reproduce.)

I can also get this failure without pg_amcheck, going directly to the btree 
checking code.  Having already built the table as you prescribe:

amcheck % psql -c "CREATE INDEX CONCURRENTLY t_idx ON t(i);" & sleep 0.1 && 
psql -c "SELECT * FROM pg_catalog.bt_index_parent_check(index := 
't_idx'::regclass, heapallindexed := true, rootdescend := true)" 
[1] 9553
ERROR:  deadlock detected
DETAIL:  Process 9555 waits for ShareLock on virtual transaction 5/11; blocked 
by process 9558.
Process 9558 waits for ShareLock on relation 16406 of database 16384; blocked 
by process 9555.
HINT:  See server log for query details.
ERROR:  cannot check index "t_idx"
DETAIL:  Index is not valid.
[1]  + exit 1 psql -c "CREATE INDEX CONCURRENTLY t_idx ON t(i);"

If Peter agrees that this is not pg_amcheck specific, then we should start a 
new thread to avoid confusing the commitfest tickets for these two items.

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: corruption of WAL page header is never reported

[Fujii Masao]

On 2021/09/13 17:21, Kyotaro Horiguchi wrote:

I wrote "while not in standby mode, we don't need to avoid retry the
entire record" but that doesn't mean the inversion "while in standby
mode, we need to do avoid that". In the first place retry doesn't
happen while not in standby mode.  I don't come up with a nice
phrasing but something like this works?

Note that we don't do this while not in standby mode because this is
required only to avoid retrying this entire record for an invalid page
header while in standby mode. Instead, ReadPageInternal() is
responsible for validating the page header in that case.


I think that it's better to comment why "retry" is not necessary
when not in standby mode.

---
When not in standby mode, an invalid page header should cause recovery
to end, not retry reading the page, so we don't need to validate the page
header here for the retry. Instead, ReadPageInternal() is responsible for
the validation.

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

Re: [PATCH] Print error when libpq-refs-stamp fails

[Anton Voloshin]

Hello,

On 28/09/2021 05:55, Rachel Heaton wrote:

Hello,

While developing I got this error and it was difficult to figure out 
what was going on.


Thanks to Jacob, I was able to learn the context of the failure, so we 
created this small patch.


-   ! nm -A -u $< 2>/dev/null | grep -v __cxa_atexit | grep exit
+   @if nm -a -u $< 2>/dev/null | grep -v __cxa_atexit | grep exit; then \
+		echo 'libpq must not be linked against any library calling exit'; 
exit 1; \

+   fi

Could you please confirm that the change from -A to -a in nm arguments 
in this patch is intentional?


--
Anton Voloshin
Postgres Professional, The Russian Postgres Company
https://postgrespro.ru

Re: preserving db/ts/relfilenode OIDs across pg_upgrade (was Re: storing an explicit nonce)

[Shruthi Gowda]

On Fri, Sep 24, 2021 at 12:44 AM Robert Haas  wrote:
>
> On Wed, Sep 22, 2021 at 3:07 PM Shruthi Gowda  wrote:
> > > - The comment in binary_upgrade_set_pg_class_oids() is still not
> > > accurate. You removed the sentence which says "Indexes cannot have
> > > toast tables, so we need not make this probe in the index code path"
> > > but the immediately preceding sentence is still inaccurate in at least
> > > two ways. First, it only talks about tables, but the code now applies
> > > to indexes. Second, it only talks about OIDs, but now also deals with
> > > refilenodes. It's really important to fully update every comment that
> > > might be affected by your changes!
> >
> > The comment is updated.
>
> Looks good.
>
> > The SQL query will not result in duplicate rows because the first join
> > filters the duplicate rows if any with the on clause ' i.indisvalid'
> > on it. The result of the first join is further left joined with pg_class and
> > pg_class will not have duplicate rows for a given oid.
>
> Oh, you're right. My mistake.
>
> > As per your suggestion, reloid and relfilenode are absorbed in the same 
> > place.
> > An additional parameter called 'suppress_storage' is passed to heap_create()
> > which indicates whether or not to create the storage when the caller
> > passed a valid relfilenode.
>
> I find it confusing to have both suppress_storage and create_storage
> with one basically as the negation of the other. To avoid that sort of
> thing I generally have a policy that variables and options should say
> whether something should happen, rather than whether it should be
> prevented from happening. Sometimes there are good reasons - such as
> strong existing precedent - to deviate from this practice but I think
> it's good to follow when possible. So my proposal is to always have
> create_storage and never suppress_storage, and if some function needs
> to adjust the value of create_storage that was passed to it then OK.

Sure, I agree. In the latest patch, only 'create_storage' is used.

> > I did not make the changes to set the oid and relfilenode in the same call.
> > I feel the uniformity w.r.t the other function signatures in
> > pg_upgrade_support.c will be lost because currently each function sets
> > only one attribute.
> > Also, renaming the applicable function names to represent that they
> > set both oid and relfilenode will make the function name even longer.
> > We may opt to not include the relfilenode in the function name instead
> > use a generic name like binary_upgrade_set_next_xxx_pg_class_oid() but
> > then
> > we will end up with some functions that set two attributes and some
> > functions that set one attribute.
>
> OK.
>
> > I have also attached the patch to preserve the DB oid. As discussed,
> > template0 will be created with a fixed oid during initdb. I am using
> > OID 2 for template0. Even though oid 2 is already in use for the
> > 'pg_am' catalog I see no harm in using it for template0 DB because oid
> > doesn’t have to be unique across the database - it has to be unique
> > for the particular catalog table. Kindly let me know if I am missing
> > something?
> > Apparently, if we did decide to pick an unused oid for template0 then
> > I see a challenge in removing that oid from the unused oid list. I
> > could not come up with a feasible solution for handling it.
>
> You are correct that there is no intrinsic reason why the same OID
> can't be used in various different catalogs. We have a policy of not
> doing that, though; I'm not clear on the reason. Maybe it'd be OK to
> deviate from that policy here, but another option would be to simply
> change the unused_oids script (and maybe some of the others). It
> already has:
>
> my $FirstGenbkiObjectId =
>   Catalog::FindDefinedSymbol('access/transam.h', '..', 'FirstGenbkiObjectId');
> push @{$oids}, $FirstGenbkiObjectId;
>
> Presumably it could be easily adapted to push the value of some other
> defined symbol into @{$oids} also, thus making that OID in effect
> used.

Thanks for the inputs, Robert. In the v4 patch, an unused OID (i.e, 4)
is fixed for the template0 and the same is removed from unused oid
list.

In addition to the review comment fixes, I have removed some code that
is no longer needed/doesn't make sense since we preserve the OIDs.

Regards,
Shruthi KC
EnterpriseDB: http://www.enterprisedb.com


v4-0001-Preserve-relfilenode-and-tablespace-OID-in-pg_upg.patch
Description: Binary data


v4-0002-Preserve-database-OIDs-in-pg_upgrade.patch
Description: Binary data

Re: 2021-09 Commitfest

[Magnus Hagander]

On Mon, Oct 4, 2021 at 3:05 PM Daniel Gustafsson  wrote:

> > On 4 Oct 2021, at 14:56, Magnus Hagander  wrote:
>
> > Ugh. i missed one of the two checks. That's what I get for not testing
> properly when the change "was so simple"...
> >
> > Please try again.
>
> It works now, I was able to move a patch (3128) over to the 2021-11 CF.  It
> does bring up the below warning(?) in a blue bar when the move was
> performed
> which at first made me think it hadn't worked.
>
> "The status of this patch cannot be changed in this commitfest.  You
> must
> modify it in the one where it's open!"
>

Did you try it with more than one patch? It could be a held back message
that got delivered late (yes, there are some such cases, sadly). I ask
because I'm failing to reproduce this one...

-- 
 Magnus Hagander
 Me: https://www.hagander.net/ 
 Work: https://www.redpill-linpro.com/ 

Re: [PATCH] Print error when libpq-refs-stamp fails

[Jacob Champion]

On Mon, 2021-10-04 at 23:40 +0700, Anton Voloshin wrote:
> 
> Could you please confirm that the change from -A to -a in nm arguments 
> in this patch is intentional?

That was not intended by us, thank you for the catch! A stray
lowercasing in vim, perhaps.

--Jacob

Bug in DefineRange() with multiranges

[Sergey Shinderuk]

Hi,

My colleague, Alex Kozhemyakin, stumbled upon a bug in DefineRange().
The problem is here:

@@ -1707,7 +1707,6 @@ DefineRange(ParseState *pstate, 
CreateRangeStmt *stmt)

/* Create cast from the range type to its multirange type */
CastCreate(typoid, multirangeOid, castFuncOid, 'e', 'f', 
DEPENDENCY_INTERNAL);


-   pfree(multirangeTypeName);
pfree(multirangeArrayName);

return address;


Given a query

create type textrange1 as range(subtype=text, 
multirange_type_name=multirange_of_text, collation="C");


the string "multirange_of_text" in the parse tree is erroneously
pfree'd.  The corrupted parse tree is then passed to event triggers.

There is another branch in DefineRange() that genereates a multirange
type name which is fine to free.

I wonder what is the proper fix.  Just drop pfree() altogether or add
pstrdup() instead?  I see that makeMultirangeTypeName() doesn't bother
freeing its buf.

Here is a gdb session demonstating the bug:

Breakpoint 1, ProcessUtilitySlow (pstate=0x5652e80c7730, 
pstmt=0x5652e80a6a40, queryString=0x5652e80a5790 "create type textrange1 
as range(subtype=text, multirange_type_name=multirange_of_text, 
collation=\"C\");",
context=PROCESS_UTILITY_TOPLEVEL, params=0x0, queryEnv=0x0, 
qc=0x7ffe835b4be0, dest=) at 
/pgwork/REL_14_STABLE/src/src/backend/tcop/utility.c:1621
1621address = DefineRange((CreateRangeStmt *) 
parsetree);
(gdb) p *(Value *)((TypeName *)((DefElem *)((CreateRangeStmt 
*)parsetree)->params->elements[1].ptr_value)->arg)->names->elements[0].ptr_value
$1 = {type = T_String, val = {ival = -401972176, str = 0x5652e80a6430 
"multirange_of_text"}}

(gdb) n
1900if (!commandCollected)
(gdb) p *(Value *)((TypeName *)((DefElem *)((CreateRangeStmt 
*)parsetree)->params->elements[1].ptr_value)->arg)->names->elements[0].ptr_value
$2 = {type = T_String, val = {ival = -401972176, str = 0x5652e80a6430 
'\177' , "\020"}}



Regards,

--
Sergey Shinderukhttps://postgrespro.com/

Re: pgsql: Document XLOG_INCLUDE_XID a little better

[Robert Haas]

On Sat, Oct 2, 2021 at 6:46 AM Dilip Kumar  wrote:
> I have written two patches, Approach1 is as you described using a
> static boolean and Approach2 as a local variable to XLogAssembleRecord
> as described by Amit, attached both of them for your reference.
> IMHO, either of these approaches looks cleaner.

I agree, and I don't have a strong preference between them. If I were
writing code like this from scratch, I would do what 0001 does. But
0002 is arguably more consistent with the existing style. It's kind of
hard to judge, at least for me, which is to be preferred.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Re: [PATCH] Print error when libpq-refs-stamp fails

[Daniel Gustafsson]

> On 4 Oct 2021, at 19:02, Jacob Champion  wrote:
> 
> On Mon, 2021-10-04 at 23:40 +0700, Anton Voloshin wrote:
>> 
>> Could you please confirm that the change from -A to -a in nm arguments 
>> in this patch is intentional?
> 
> That was not intended by us, thank you for the catch! A stray
> lowercasing in vim, perhaps.

Hmm, I will take care of this shortly.

/ Daniel

Re: BUG #17212: pg_amcheck fails on checking temporary relations

[Peter Geoghegan]

On Mon, Oct 4, 2021 at 8:10 AM Mark Dilger  wrote:
> > There is another issue, that maybe should be discussed separately (or
> > this thread could be renamed to "... on checking specific relations"),
> > but the solution could be similar to that.
> > pg_amcheck also fails on checking invalid indexes, that could be created
> > legitimately by the CREATE INDEX CONCURRENTLY command.
>
> I believe this is a bug in amcheck's btree checking functions.  Peter, can 
> you take a look?

Why do you say that? verify_nbtree.c will throw an error when called
with an invalid index -- which is what we actually see here. Obviously
that is the intended behavior, and always has been. This hasn't been a
problem before now, probably because the sample verification query in
the docs (under bt_index_check()) accounts for this directly.

Why shouldn't we expect pg_amcheck to do the same thing, at the SQL
level? It's practically the same thing as the temp table issue.
Indeed, verify_nbtree.c will throw an error on a temp table (at least
if it's from another session).

> I can also get this failure without pg_amcheck, going directly to the btree 
> checking code.  Having already built the table as you prescribe:

> ERROR:  deadlock detected
> DETAIL:  Process 9555 waits for ShareLock on virtual transaction 5/11; 
> blocked by process 9558.
> Process 9558 waits for ShareLock on relation 16406 of database 16384; blocked 
> by process 9555.
> HINT:  See server log for query details.
> ERROR:  cannot check index "t_idx"
> DETAIL:  Index is not valid.

I think that the deadlock is just another symptom of the same problem.

-- 
Peter Geoghegan

Re: improvements in Unicode tables generation code

[Peter Eisentraut]

On 28.09.21 10:25, Peter Eisentraut wrote:


On 20.07.21 13:57, Peter Eisentraut wrote:
Perhaps we should change the script or Makefile so that it doesn't 
create all the maps in one go?


I agree, either comment it better or just write one file at a time. 
I'll take another look at that.


Here is a patch that does it one file (pair) at a time.  The other 
rules besides UCS_to_most.pl actually had the same problem, since they 
produce two output files, so running in parallel called each script 
twice.  In this patch, all of that is heavily refactored and works 
correctly now. Note that UCS_to_most.pl already accepted a 
command-line argument to specify which encoding to work with.


Here is an updated patch with a thinko fix that made the previous patch 
not actually work.


I have committed this one and closed the CF entry.

Re: [PATCH] Print error when libpq-refs-stamp fails

[Daniel Gustafsson]

> On 4 Oct 2021, at 19:21, Daniel Gustafsson  wrote:
> 
>> On 4 Oct 2021, at 19:02, Jacob Champion  wrote:
>> 
>> On Mon, 2021-10-04 at 23:40 +0700, Anton Voloshin wrote:
>>> 
>>> Could you please confirm that the change from -A to -a in nm arguments 
>>> in this patch is intentional?
>> 
>> That was not intended by us, thank you for the catch! A stray
>> lowercasing in vim, perhaps.
> 
> Hmm, I will take care of this shortly.

Right, so I missed this in reviewing and testing, and I know why the latter
didn't catch it.  nm -A and -a outputs the same thing *for this input* on my
Debian and macOS boxes, with the small difference that -A prefixes the line
with the name of the input file.  -a also include debugger symbols, but for
this usage it didn't alter the results.  I will go ahead and fix this, thanks
for catching it! 

--
Daniel Gustafsson   https://vmware.com/

Re: 2021-09 Commitfest

[Daniel Gustafsson]

> On 4 Oct 2021, at 18:51, Magnus Hagander  wrote:
> 
> On Mon, Oct 4, 2021 at 3:05 PM Daniel Gustafsson  > wrote:
> > On 4 Oct 2021, at 14:56, Magnus Hagander  > > wrote:
> 
> > Ugh. i missed one of the two checks. That's what I get for not testing 
> > properly when the change "was so simple"...
> > 
> > Please try again. 
> 
> It works now, I was able to move a patch (3128) over to the 2021-11 CF.  It
> does bring up the below warning(?) in a blue bar when the move was performed
> which at first made me think it hadn't worked.
> 
> "The status of this patch cannot be changed in this commitfest.  You must
> modify it in the one where it's open!"
> 
> Did you try it with more than one patch? It could be a held back message that 
> got delivered late (yes, there are some such cases, sadly). I ask because I'm 
> failing to reproduce this one... 

It was on the very same entry and I only tested that one, so it sounds likely
that it was a message that was late to the party.

--
Daniel Gustafsson   https://vmware.com/

RfC entries in CF 2021-09

[Jaime Casanova]

Hi,

Here's the list mentioned in ${SUBJECT}, please can the committers
mention what they want to do with those?

no committer assigned:

- Bug fix for tab completion of ALTER TABLE
  This seems to have activity, last patch from is from two weeks ago.
  Any intention of committing this soom? Or should I move it to next CF?

- Extending amcheck to check toast size and compression
  Last patch from may-2021, also it seems there is no activity since
  Jul-2021. Peter Geoghegan, are you planning to look at this one?

- Parallel Hash Full Join
  cfbot says it's failing on frebsd on parallel group tests: join_hash,
  brin_bloom, brin_multi, create_table_like, async, misc_functions, 
  collate.icu.utf8, dbsize, incremental_sort, tidscan, tsrf, tidrangescan, 
  tid, sysviews, misc, alter_operator, alter_generic
  Thomas said he was working on this one, but it sounds we shouldn't
  expect this to be committed in the next days. So I will move this to
  next CF. Thomas, are you alright with that?

- Simplify some RI checks to reduce SPI overhead
  Last patch is from Jul-2021, little activity since then. Peter
  Eisentraut you're marked as reviewer here, do you intend to take the
  patch as the committer?

- enhancing plpgsql API for debugging and tracing
  Last patch is from aug-2021, which was the last activity on this.
  Suggestions?

- Identify missing publications from publisher while create/alter subscription
  Last patch is from Aug-2021, cfbot says it cannot be applied but is
  only a .sgml the one that fails. Patch actually compiles (i tried it
  myself 3 days ago but did no further tests).
  https://www.postgresql.org/message-id/20210928021944.GA18070%40ahch-to
  Suggestions?

- Minimal logical decoding on standbys (take 6)
  It seems it has activity and it's a useful improvement.
  Any one is going to take it?

- Allow providing restore_command as a command line option to pg_rewind
  Last patch is from aug-2021. Comments?

- global temporary table
  This has activity. And seems a good improvement. Comments?

- Fix pg_rewind race condition just after promotion
  Last patch is from mar-2021. Heikki, are you going to take this one?

Etsuro Fujita:

- Fast COPY FROM command for the foreign tables
  Last patch was on Jun-2021, no further activity after that.
  Etsuro-san, are you going to commit this soon?

-- 
Jaime Casanova
Director de Servicios Profesionales
SystemGuards - Consultores de PostgreSQL

Re: Duplicat-word typos in code comments

[Daniel Gustafsson]

> On 4 Oct 2021, at 15:56, Tom Lane  wrote:

> I used to think it was better to go ahead and manually reflow, if you
> use an editor that makes that easy.  That way there are fewer commits
> touching any one line of code, which is good when trying to review
> code history.  However, now that we've got the ability to make "git
> blame" ignore pgindent commits, maybe it's better to leave that sort
> of mechanical cleanup to pgindent, so that the substantive patch is
> easier to review.

Yeah, that's precisely why I did it.  Since we can skip over pgindent sweeps it
makes sense to try and minimize such changes to make code archaeology easier.
There are of course cases when the result will be such an eyesore that we'd
prefer to have it done sooner, but in cases like these where line just got one
word shorter it seemed an easy choice.

--
Daniel Gustafsson   https://vmware.com/

Re: Atomic rename feature for Windows.

[Victor Spirin]

Thank you

Thank you
In this version of patch:

1. Made function isWindows1607OrGreater() without manifest

2. To open a directory using CreateFile, have to specify the 
FILE_FLAG_BACKUP_SEMANTICS flag as part of dwFlagsAndAttributes. Checks 
that file is a directory by the GetFileAttributes function.


Victor Spirin
Postgres Professional:http://www.postgrespro.com
The Russian Postgres Company

01.10.2021 15:37, Juan José Santamaría Flecha пишет:


On Thu, Sep 30, 2021 at 11:00 PM Victor Spirin 
mailto:v.spi...@postgrespro.ru>> wrote:



IsWindowsVersionOrGreater(10,0,1607) always returns false

Only IsWindowsVersionOrGreater(10,0,0) is a valid call. (There are
no service packs in Windows 10.)

I haven't found a way to determine the Windows 10 release ID.
The RtlGetVersion function returns dwBuildNumber = 19042 on my
Windows.

I heard that Microsoft does not support older versions of Windows
10 and requires a mandatory update.

You can translate the BuildNumber to the ReleaseId, for 1607 it will 
be 14393 [1].


We might find pretty much anything in the wild, the safer the check 
the better.


[1] https://en.wikipedia.org/wiki/Windows_10_version_history 



Regards,

Juan José Santamaría Flecha
diff --git a/src/include/common/checksum_helper.h 
b/src/include/common/checksum_helper.h
index cac7570ea1..2d533c93a6 100644
--- a/src/include/common/checksum_helper.h
+++ b/src/include/common/checksum_helper.h
@@ -26,6 +26,13 @@
  * MD5 here because any new that does need a cryptographically strong checksum
  * should use something better.
  */
+
+ /*
+ * CHECKSUM_TYPE_NONE defined in the winioctl.h when _WIN32_WINNT >= 
_WIN32_WINNT_WIN10
+ */
+#ifdef CHECKSUM_TYPE_NONE
+#undef CHECKSUM_TYPE_NONE
+#endif
 typedef enum pg_checksum_type
 {
CHECKSUM_TYPE_NONE,
diff --git a/src/include/port/win32.h b/src/include/port/win32.h
index d8ae49e22d..d91555f5c0 100644
--- a/src/include/port/win32.h
+++ b/src/include/port/win32.h
@@ -12,12 +12,13 @@
 /*
  * Make sure _WIN32_WINNT has the minimum required value.
  * Leave a higher value in place. When building with at least Visual
- * Studio 2015 the minimum requirement is Windows Vista (0x0600) to
- * get support for GetLocaleInfoEx() with locales. For everything else
+ * Studio 2015 the minimum requirement is Windows 10 (0x0A00) to get support 
for SetFileInformationByHandle.
+ * The minimum requirement is Windows Vista (0x0600) get support for 
GetLocaleInfoEx() with locales.
+ * For everything else
  * the minimum version is Windows XP (0x0501).
  */
 #if defined(_MSC_VER) && _MSC_VER >= 1900
-#define MIN_WINNT 0x0600
+#define MIN_WINNT 0x0A00
 #else
 #define MIN_WINNT 0x0501
 #endif
diff --git a/src/port/dirmod.c b/src/port/dirmod.c
index 763bc5f915..12e9c35cef 100644
--- a/src/port/dirmod.c
+++ b/src/port/dirmod.c
@@ -39,6 +39,146 @@
 #endif
 #endif
 
+
+#if defined(WIN32) && !defined(__CYGWIN__) && defined(_WIN32_WINNT_WIN10) && 
_WIN32_WINNT >= _WIN32_WINNT_WIN10
+
+#include 
+
+/*
+ * Checks Windows version using RtlGetVersion
+ * Version 1607 (Build 14393) is required for SetFileInformationByHandle
+ * function with FILE_RENAME_FLAG_POSIX_SEMANTICS flag
+*/
+typedef NTSYSAPI(NTAPI * PFN_RTLGETVERSION)
+(OUT PRTL_OSVERSIONINFOEXW lpVersionInformation);
+
+static int isWin1607 = -1;
+static int isWindows1607OrGreater()
+{
+   HMODULE ntdll;
+   PFN_RTLGETVERSION _RtlGetVersion = NULL;
+   OSVERSIONINFOEXW info;
+   if (isWin1607 >= 0) return isWin1607;
+   ntdll = LoadLibraryEx("ntdll.dll", NULL, 0);
+   if (ntdll == NULL)
+   {
+   DWORD   err = GetLastError();
+
+   _dosmaperr(err);
+   return -1;
+   }
+
+   _RtlGetVersion = (PFN_RTLGETVERSION)(pg_funcptr_t)
+   GetProcAddress(ntdll, "RtlGetVersion");
+   if (_RtlGetVersion == NULL)
+   {
+   DWORD   err = GetLastError();
+
+   FreeLibrary(ntdll);
+   _dosmaperr(err);
+   return -1;
+   }
+   info.dwOSVersionInfoSize = sizeof(OSVERSIONINFOEXW);
+   if (!NT_SUCCESS(_RtlGetVersion(&info)))
+   {
+   DWORD   err = GetLastError();
+
+   FreeLibrary(ntdll);
+   _dosmaperr(err);
+   return -1;
+   }
+
+   if (info.dwMajorVersion >= 10 && info.dwBuildNumber >= 14393) isWin1607 
= 1;
+   else isWin1607 = 0;
+   FreeLibrary(ntdll);
+   return isWin1607;
+
+}
+
+typedef struct _FILE_RENAME_INFO_EXT {
+   FILE_RENAME_INFO fri;
+   WCHAR extra_space[MAX_PATH];
+} FILE_RENAME_INFO_EXT;
+
+/*
+ * pgrename_windows_posix_semantics  - uses SetFileInformationByHandle function
+ * with FILE_RENAME_FLAG_POSIX_SEMANTICS flag for atomic rename file
+ * working only on Windows 10 (1607) or later and  _WIN32_WINNT must be >= 
_WIN32_WINNT_WIN10
+ */
+static int
+pgrename_windows_posix_semant

Re: Duplicat-word typos in code comments

[Tom Lane]

Daniel Gustafsson  writes:
>> On 4 Oct 2021, at 15:56, Tom Lane  wrote:
>> I used to think it was better to go ahead and manually reflow, if you
>> use an editor that makes that easy.  That way there are fewer commits
>> touching any one line of code, which is good when trying to review
>> code history.  However, now that we've got the ability to make "git
>> blame" ignore pgindent commits, maybe it's better to leave that sort
>> of mechanical cleanup to pgindent, so that the substantive patch is
>> easier to review.

> Yeah, that's precisely why I did it.  Since we can skip over pgindent sweeps 
> it
> makes sense to try and minimize such changes to make code archaeology easier.
> There are of course cases when the result will be such an eyesore that we'd
> prefer to have it done sooner, but in cases like these where line just got one
> word shorter it seemed an easy choice.

Actually though, there's another consideration: if you leave
not-correctly-pgindented code laying around, it causes problems
for the next hacker who modifies that file and wishes to neaten
up their own work by pgindenting it.  They can either tediously
reverse out part of the delta, or commit a patch that includes
entirely-unrelated cosmetic changes, neither of which is
pleasant.

regards, tom lane

Re: BUG #17212: pg_amcheck fails on checking temporary relations

[Peter Geoghegan]

On Mon, Oct 4, 2021 at 2:00 AM Alexander Lakhin  wrote:
> There is another issue, that maybe should be discussed separately (or
> this thread could be renamed to "... on checking specific relations"),
> but the solution could be similar to that.

Thanks for the report!

I wonder if verify_heapam.c does the right thing with unlogged tables
when verification runs on a standby -- a brief glance at the code
leaves me with the impression that it's not handled there. Note that
verify_nbtree.c initially got it wrong. The issue was fixed by bugfix
commit 6754fe65. Before then nbtree verification could throw a nasty
low-level smgr error, just because we had an unlogged table in hot
standby mode.

Note that we deliberately skip indexes when this happens (we don't
error out), unlike the temp buffers (actually temp table) case. This
seems like the right set of behaviors. We really don't want to have to
throw an "invalid object type" style error just because verification
runs during recovery. Plus it just seems logical to assume that
unlogged indexes/tables don't have storage when we're in hot standby
mode, and so must simply have nothing for us to verify.

-- 
Peter Geoghegan

Re: ssl tests fail on windows / slurp_file() offset doesn't work on win

[Andres Freund]

On 2021-10-04 11:07:07 -0400, Andrew Dunstan wrote:
> Looks sane, thanks.

Thanks for looking. Pushed to all branches.

Re: storing an explicit nonce

[Bruce Momjian]

On Tue, Sep 28, 2021 at 12:30:02PM +0300, Ants Aasma wrote:
> On Mon, 27 Sept 2021 at 23:34, Bruce Momjian  wrote:
> 
> On Sun, Sep  5, 2021 at 10:51:42PM +0800, Sasasu wrote:
> > Hi, community,
> >
> > It looks like we are still considering AES-CBC, AES-XTS, and AES-GCM
> (-SIV).
> > I want to say something that we don't think about.
> >
> > For AES-CBC, the IV should be not predictable. I think LSN or HASH(LSN,
> > block number or something) is predictable. There are many CVE related to
> > AES-CBC with a predictable IV.
> 
> The LSN would change every time the page is modified, so while the LSN
> could be predicted, it would not be reused.  However, there is currently
> no work being done on page-level encryption of Postgres.
> 
> 
> We are still working on our TDE patch. Right now the focus is on refactoring
> temporary file access to make the TDE patch itself smaller. Reconsidering
> encryption mode choices given concerns expressed is next. Currently a viable
> option seems to be AES-XTS with LSN added into the IV. XTS doesn't have an
> issue with predictable IV and isn't totally broken in case of IV reuse.

Sounds great, thanks!

-- 
  Bruce Momjian  https://momjian.us
  EDB  https://enterprisedb.com

  If only the physical world exists, free will is an illusion.

Re: [PATCH] Add `verify-system` sslmode to use system CA pool for server cert

[Bruce Momjian]

On Tue, Sep 28, 2021 at 02:54:39AM -0700, tho...@habets.se wrote:
> On Tue, 28 Sep 2021 02:09:11 +0100, Bruce Momjian  said:
> > I don't think public CA's are not a good idea for complex setups since
> > they open the ability for an external party to create certificates that
> > are trusted by your server's CA, e.g., certificate authentication.
> 
> I'm not arguing for, and in fact would argue against, public CA for
> client certs.
> 
> So that's a separate issue.
> 
> Note that mTLS prevents a MITM attack that exposes server data even if
> server cert is compromised or re-issued, so if the install is using
> client certs (with private CA) then the public CA for server matters
> much less.
> 
> You can end up at the wrong server, yes, and provide data as INSERT,
> but can't steal or corrupt existing data.
> 
> And you say for complex setups. Fair enough. But currently I'd say the
> default is wrong, and what should be default is not configurable.

Agreed, I think this needs much more discussion and documentation.

-- 
  Bruce Momjian  https://momjian.us
  EDB  https://enterprisedb.com

  If only the physical world exists, free will is an illusion.

Re: Patch: Range Merge Join

[Jaime Casanova]

On Thu, Jun 10, 2021 at 07:14:32PM -0700, Jeff Davis wrote:
> On Thu, 2021-06-10 at 15:09 +1200, David Rowley wrote:
> > It shouldn't be a blocker for you, but just so you're aware, there
> > was
> > a previous proposal for this in [1] and a patch in [2].  I've include
> > Jeff here just so he's aware of this. Jeff may wish to state his
> > intentions with his own patch. It's been a few years now.
> 
> Great, thank you for working on this!
> 
> I'll start with the reason I set the work down before: it did not work
> well with multiple join keys. That might be fine, but I also started
> thinking it was specialized enough that I wanted to look into doing it
> as an extension using the CustomScan mechanism.
> 
> Do you have any solution to working better with multiple join keys? And
> do you have thoughts on whether it would be a good candidate for the
> CustomScan extension mechanism, which would make it easier to
> experiment with?
> 

Hi,

It seems this has been stalled since jun-2021. I intend mark this as
RwF unless someone speaks in the next hour or so.

-- 
Jaime Casanova
Director de Servicios Profesionales
SystemGuards - Consultores de PostgreSQL

plperl on windows

[Andres Freund]

Hi,

For https://postgr.es/m/20211001222752.wrz7erzh4cajv...@alap3.anarazel.de I
was trying to enable plperl on windows. And run into quite a few roadblocks -
enough that I gave up.

1) plperl doesn't build against a modern-ish perl. The fix for that seems easy
   enough: https://postgr.es/m/20200501134711.08750...@antares.wagner.home

2) For some reason src/tools/install.pl doesn't install plperl[u].control,
   plperl[u]--1.0.sql - But apparently the buildfarm doesn't have that issue,
   because drongo successfully ran the plperl tests?

3) When building against strawberry perl 5.32.1.1 I see errors when loading
   plperl

4) When building against strawberry perl 5.30.3.1 I see a crash during
   execution of very simple statements [1]

5) Finally when building against strawberry perl 5.28.2.1, plperl kinda
   works. But there's a lot of regression test failures, many of them
   seemingly around error trapping.


I saw that there's also active state perl, but it seems to require clicking
through some terms and conditions for every download that I don't want to
agree to.

Greetings,

Andres Freund

[1]
Exception thrown at 0x6FD75DB8 (perl530.dll) in postgres.exe: 
0xC005: Access violation reading location 0x0008.
perl530.dll!Perl_mg_get() + 56 bytesUnknown
plperl.dll!select_perl_context(bool trusted) Line 667   C
plperl.dll!plperl_inline_handler(FunctionCallInfoBaseData * fcinfo) 
Line 1941   C
plperl.dll!plperlu_inline_handler(FunctionCallInfoBaseData * fcinfo) 
Line 2064  C
postgres.exe!FunctionCall1Coll(FmgrInfo * flinfo, unsigned int 
collation, unsigned __int64 arg1) Line 1138  C
postgres.exe!OidFunctionCall1Coll(unsigned int functionId, unsigned int 
collation, unsigned __int64 arg1) Line 1417 C
postgres.exe!ExecuteDoStmt(ParseState * pstate, DoStmt * stmt, bool 
atomic) Line 2146   C
postgres.exe!standard_ProcessUtility(PlannedStmt * pstmt, const char * 
queryString, bool readOnlyTree, ProcessUtilityContext context, 
ParamListInfoData * params, QueryEnvironment * queryEnv, _DestReceiver * dest, 
QueryCompletion * qc) Line 712 C
postgres.exe!ProcessUtility(PlannedStmt * pstmt, const char * 
queryString, bool readOnlyTree, ProcessUtilityContext context, 
ParamListInfoData * params, QueryEnvironment * queryEnv, _DestReceiver * dest, 
QueryCompletion * qc) Line 530  C
postgres.exe!PortalRunUtility(PortalData * portal, PlannedStmt * pstmt, 
bool isTopLevel, bool setHoldSnapshot, _DestReceiver * dest, QueryCompletion * 
qc) Line 1157C
postgres.exe!PortalRunMulti(PortalData * portal, bool isTopLevel, bool 
setHoldSnapshot, _DestReceiver * dest, _DestReceiver * altdest, QueryCompletion 
* qc) Line 1306  C
postgres.exe!PortalRun(PortalData * portal, long count, bool 
isTopLevel, bool run_once, _DestReceiver * dest, _DestReceiver * altdest, 
QueryCompletion * qc) Line 790   C
postgres.exe!exec_simple_query(const char * query_string) Line 1222 
C
postgres.exe!PostgresMain(const char * dbname, const char * username) 
Line 4499 C
postgres.exe!BackendRun(Port * port) Line 4561  C
postgres.exe!SubPostmasterMain(int argc, char * * argv) Line 5066   
C
postgres.exe!main(int argc, char * * argv) Line 190 C
postgres.exe!invoke_main() Line 79  C++
postgres.exe!__scrt_common_main_seh() Line 288  C++
postgres.exe!__scrt_common_main() Line 331  C++
postgres.exe!mainCRTStartup(void * __formal) Line 17C++
kernel32.dll!BaseThreadInitThunk()  Unknown
ntdll.dll!RtlUserThreadStart()  Unknown

[2]
--- C:/Users/anfreund/src/postgres/src/pl/plperl/expected/plperl.out
2021-03-02 00:29:34.416742000 -0800
+++ C:/Users/anfreund/src/postgres/src/pl/plperl/results/plperl.out 
2021-10-04 14:31:45.773612500 -0700
@@ -660,8 +660,11 @@
   return $result;
 $$ LANGUAGE plperl;
 SELECT perl_spi_prepared_bad(4.35) as "double precision";
-ERROR:  type "does_not_exist" does not exist at line 2.
-CONTEXT:  PL/Perl function "perl_spi_prepared_bad"
+ double precision
+--
+
+(1 row)
+
 -- Test with a row type
 CREATE OR REPLACE FUNCTION perl_spi_prepared() RETURNS INTEGER AS $$
my $x = spi_prepare('select $1::footype AS a', 'footype');
@@ -696,37 +699,28 @@
 NOTICE:  This is a test
 -- check that restricted operations are rejected in a plperl DO block
 DO $$ system("/nonesuch"); $$ LANGUAGE plperl;
-ERROR:  'system' trapped by operation mask at line 1.
-CONTEXT:  PL/Perl anonymous code block
...

--- C:/Users/anfreund/src/postgres/src/pl/plperl/expected/plperl_plperlu.out
2021-03-02 00:29:34.425742300 -0800
+++ C:/Users/anfreund/src/postgres/src/pl/plperl/results/plperl_plperlu.out 
2021-10-04 14:31:48.065612400 -0700
@@ -10,11 +10,17 @@
 return 1;
 $$ LANGUAGE plperlu; -- compile plperlu code
 SELECT * FROM bar(); -- throws exception normally (running plpe

Re: plperl on windows

[Andres Freund]

Hi,

On 2021-10-04 14:38:16 -0700, Andres Freund wrote:
> 2) For some reason src/tools/install.pl doesn't install plperl[u].control,
>plperl[u]--1.0.sql - But apparently the buildfarm doesn't have that issue,
>because drongo successfully ran the plperl tests?

Oh, figured that one out: Install.pm checks the current directory for
config.pl - but my invocation was from the source tree root (which is
supported for most things). Because of that it skipped installing plperl, as
it though it wasn't installed.

Greetings,

Andres Freund

Re: Use simplehash.h instead of dynahash in SMgr

[David Rowley]

On Mon, 4 Oct 2021 at 20:37, Jaime Casanova
 wrote:
> Based on your comments I will mark this patch as withdrawn at midday of
> my monday unless someone objects to that.

I really think we need a hash table implementation that's faster than
dynahash and supports stable pointers to elements (simplehash does not
have stable pointers). I think withdrawing this won't help us move
towards getting that.

Thomas voiced his concerns here about having an extra hash table
implementation and then also concerns that I've coded the hash table
code to be fast to iterate over the hashed items.  To be honest, I
think both Andres and Thomas must be misunderstanding the bitmap part.
I get the impression that they both think the bitmap is solely there
to make interations faster, but in reality it's primarily there as a
compact freelist and can also be used to make iterations over sparsely
populated tables fast. For the freelist we look for 0-bits, and we
look for 1-bits during iteration.

I think I'd much rather talk about the concerns here than just
withdraw this. Even if what I have today just serves as something to
aid discussion.

It would also be good to get the points Andres raised with me off-list
on this thread.  I think his primary concern was that bitmaps are
slow, but I don't really think maintaining full pointers into freed
items is going to improve the performance of this.

David

Re: Postgresql Windows build and modern perl (>=5.28)

[Dagfinn Ilmari Mannsåker]

Victor Wagner  writes:

> Attached patch makes use of this function if PERL_VERSION >= 28. 
> It makes plperl compile with ActiveStatePerl 5.28 and StrawberryPerl
> 5.30.2.1.

I have no opinion on the substantive content of this patch, but please
don't just check against just PERL_VERSION.  Now that Perl 6 has been
renamed to Raku, Perl may bump its major version (PERL_REVISION) to 7 at
some point in th future.

The correct thing to use is the PERL_VERSION_(GT|GE|LE|LT|EQ|NE) macros,
which are provided by newer versions of perl.  We would need to update
the included copy of ppport.h to get them on older perls, but we should
do that anyway, it's not been updated since 2009.  I'll start a separate
thread for that.

- ilmari

Re: BUG #17212: pg_amcheck fails on checking temporary relations

[Mark Dilger]

> On Oct 4, 2021, at 10:58 AM, Peter Geoghegan  wrote:
> 
> On Mon, Oct 4, 2021 at 8:10 AM Mark Dilger  
> wrote:
>>> There is another issue, that maybe should be discussed separately (or
>>> this thread could be renamed to "... on checking specific relations"),
>>> but the solution could be similar to that.
>>> pg_amcheck also fails on checking invalid indexes, that could be created
>>> legitimately by the CREATE INDEX CONCURRENTLY command.
>> 
>> I believe this is a bug in amcheck's btree checking functions.  Peter, can 
>> you take a look?
> 
> Why do you say that?

Because REINDEX CONCURRENTLY and the bt_index_parent_check() function seem to 
have lock upgrade hazards that are unrelated to pg_amcheck.

> This hasn't been a
> problem before now, probably because the sample verification query in
> the docs (under bt_index_check()) accounts for this directly.

It doesn't say anything about deadlocks, but yes, it mentions errors will be 
raised unless the caller filters out indexes that are invalid or not ready.


On to pg_amcheck's behavior

I see no evidence in the OP's complaint that pg_amcheck is misbehaving.  It 
launches a worker to check each relation, prints for the user's benefit any 
errors those checks raise, and finally returns 0 if they all pass and 2 
otherwise.  Since not all relations could be checked, 2 is returned.  Returning 
0 would be misleading, as it implies everything was checked and passed, and it 
can't honestly say that.  The return value 2 does not mean that anything 
failed.  It means that not all checks passed.  When a 2 is returned, the user 
is expected to read the output and decide what, if anything, they want to do 
about it.  In this case, the user might decide to wait until the reindex 
finishes and check again, or they might decide they don't care.

It is true that pg_amcheck is calling bt_index_parent_check() on an invalid 
index, but so what?  If it chose not to do so, it would still need to print a 
message about the index being unavailable for checking, and it would still have 
to return 2.  It can't return 0, and it is unhelpful to leave the user in the 
dark about the fact that not all indexes are in the right state for checking.  
So it would still print the same error message and still return 2.

I think this bug report is really a feature request.  The OP appears to want an 
option to toggle on/off the printing of such information, perhaps with not 
printing it as the default. 

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: BUG #17212: pg_amcheck fails on checking temporary relations

[Peter Geoghegan]

On Mon, Oct 4, 2021 at 3:36 PM Mark Dilger  wrote:
> >> I believe this is a bug in amcheck's btree checking functions.  Peter, can 
> >> you take a look?
> >
> > Why do you say that?
>
> Because REINDEX CONCURRENTLY and the bt_index_parent_check() function seem to 
> have lock upgrade hazards that are unrelated to pg_amcheck.

The problem with that argument is that the bt_index_parent_check()
function isn't doing anything particularly special, apart from
dropping the lock. That has been its behavior for many years now.

> On to pg_amcheck's behavior
>
> I see no evidence in the OP's complaint that pg_amcheck is misbehaving.  It 
> launches a worker to check each relation, prints for the user's benefit any 
> errors those checks raise, and finally returns 0 if they all pass and 2 
> otherwise.  Since not all relations could be checked, 2 is returned.  
> Returning 0 would be misleading, as it implies everything was checked and 
> passed, and it can't honestly say that.  The return value 2 does not mean 
> that anything failed.  It means that not all checks passed.  When a 2 is 
> returned, the user is expected to read the output and decide what, if 
> anything, they want to do about it.  In this case, the user might decide to 
> wait until the reindex finishes and check again, or they might decide they 
> don't care.
>
> It is true that pg_amcheck is calling bt_index_parent_check() on an invalid 
> index, but so what?  If it chose not to do so, it would still need to print a 
> message about the index being unavailable for checking, and it would still 
> have to return 2.

Why would it have to print such a message? You seem to be presenting
this as if there is some authoritative, precise, relevant definition
of "the relations that pg_amcheck sees". But to me the relevant
details look arbitrary at best.

> It can't return 0, and it is unhelpful to leave the user in the dark about 
> the fact that not all indexes are in the right state for checking.

Why is that unhelpful? More to the point, *why* would this alternative
behavior constitute "leaving the user in the dark"?

What about the case where the pg_class entry isn't visible to our MVCC
snapshot? Why is "skipping" such a relation not just as unhelpful?

> I think this bug report is really a feature request.  The OP appears to want 
> an option to toggle on/off the printing of such information, perhaps with not 
> printing it as the default.

And I don't understand why you think that clearly-accidental
implementation details (really just bugs) should be treated as
axiomatic truths about how pg_amcheck must work. Should we now "fix"
pg_dump so that it matches pg_amcheck?

All of the underlying errors are cases that were clearly intended to
catch user error -- every single one. But apparently pg_amcheck is
incapable of error, by definition. Like HAL 9000.

-- 
Peter Geoghegan

Re: BUG #17212: pg_amcheck fails on checking temporary relations

[Mark Dilger]

> On Oct 4, 2021, at 4:10 PM, Peter Geoghegan  wrote:
> 
> And I don't understand why you think that clearly-accidental
> implementation details (really just bugs) should be treated as
> axiomatic truths about how pg_amcheck must work. Should we now "fix"
> pg_dump so that it matches pg_amcheck?
> 
> All of the underlying errors are cases that were clearly intended to
> catch user error -- every single one. But apparently pg_amcheck is
> incapable of error, by definition. Like HAL 9000.

On the contrary, I got all the way finished writing a patch to have pg_amcheck 
do as you suggest before it dawned on me to wonder if that was the right way to 
go.  I certainly don't assume pg_amcheck is correct by definition.  I already 
posted a patch for the temporary tables bug upthread having never argued that 
it was anything other than a bug.  I also wrote a patch for verify_heapam to 
fix the problem with unlogged tables on standbys, and was developing a test for 
that, when I got your email.  I'm not arguing against that being a bug, either. 
 Hopefully, I can get that properly tested and post it before too long.

I am concerned about giving the user the false impression that an index (or 
table) was checked when it was not.  I don't see the logic in

  pg_amcheck -i idx1 -i idx2 -i idx3

skipping all three indexes and then reporting success.  What if the user 
launches the pg_amcheck command precisely because they see error messages in 
the logs during a long running reindex command, and are curious if the index so 
generated is corrupt.  You can't assume the user knows the index is still being 
reindexed.  If the last message logged was some time ago, they might assume the 
process has finished.  So something other than a silent success is needed to 
let them know what is going on.


—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: BUG #17212: pg_amcheck fails on checking temporary relations

[Mark Dilger]

> On Oct 4, 2021, at 4:28 PM, Mark Dilger  wrote:
> 
> pg_amcheck -i idx1 -i idx2 -i idx3

I forgot to mention:  There's a continuum between `pg_amcheck -a` which checks 
everything in all databases of the cluster, and `pg_amcheck -i just_one_index`. 
 There are any number of combinations of object names, schema names, database 
names, and patterns over the same, which select anything from an empty set to a 
huge set of things to check.  I'm trying to keep the behavior the same for all 
of those, and that's why I'm trying to avoid having `pg_amcheck -a` silently 
skip indexes that are unavailable for checking while having `pg_amcheck -i 
just_one_index` give a report about the index.  I wouldn't know where to draw 
the line between reporting the issue and not, and I doubt whatever line I 
choose will be intuitive to users.

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: BUG #17212: pg_amcheck fails on checking temporary relations

[Peter Geoghegan]

On Mon, Oct 4, 2021 at 4:28 PM Mark Dilger  wrote:
> I am concerned about giving the user the false impression that an index (or 
> table) was checked when it was not.  I don't see the logic in
>
>   pg_amcheck -i idx1 -i idx2 -i idx3
>
> skipping all three indexes and then reporting success.

This is the first time that anybody mentioned the -i option on the
thread. I read your previous remarks as making a very broad statement,
about every single issue.

Anyway, the issue with -i doesn't seem like it changes that much. Why
not just behave as if there is no such "visible" index? That's the
same condition, for all practical purposes. If that approach doesn't
seem good enough, then the error message can be refined to make the
user aware of the specific issue.

> What if the user launches the pg_amcheck command precisely because they see 
> error messages in the logs during a long running reindex command, and are 
> curious if the index so generated is corrupt.

I'm guessing that you meant REINDEX CONCURRENTLY.

Since you're talking about the case where it has an error, the whole
index build must have failed. So the user would get exactly what
they'd expect -- verification of the original index, without any
hindrance from the new/failed index.

(Thinks for a moment...)

Actually, I think that we'd only verify the original index, even
before the error with CONCURRENTLY (though I've not checked that point
myself).

-- 
Peter Geoghegan

Re: [PATCH] Print error when libpq-refs-stamp fails

[Rachel Heaton]

Thanks to you both!

From: Daniel Gustafsson 
Date: Monday, October 4, 2021 at 11:36 AM
To: Jacob Champion 
Cc: Rachel Heaton , pgsql-hackers@lists.postgresql.org 
, a.volos...@postgrespro.ru 

Subject: Re: [PATCH] Print error when libpq-refs-stamp fails
> On 4 Oct 2021, at 19:21, Daniel Gustafsson  wrote:
>
>> On 4 Oct 2021, at 19:02, Jacob Champion  wrote:
>>
>> On Mon, 2021-10-04 at 23:40 +0700, Anton Voloshin wrote:
>>>
>>> Could you please confirm that the change from -A to -a in nm arguments
>>> in this patch is intentional?
>>
>> That was not intended by us, thank you for the catch! A stray
>> lowercasing in vim, perhaps.
>
> Hmm, I will take care of this shortly.

Right, so I missed this in reviewing and testing, and I know why the latter
didn't catch it.  nm -A and -a outputs the same thing *for this input* on my
Debian and macOS boxes, with the small difference that -A prefixes the line
with the name of the input file.  -a also include debugger symbols, but for
this usage it didn't alter the results.  I will go ahead and fix this, thanks
for catching it!

--
Daniel Gustafsson   https://vmware.com/

Re: Delegating superuser tasks to new security roles (Was: Granting control of SUSET gucs to non-superusers)

[Bossart, Nathan]

On 9/27/21, 11:16 AM, "Mark Dilger"  wrote:
> On Sep 21, 2021, at 12:58 PM, Andrew Dunstan  wrote:
>> I do like the basic thrust of reducing the power of CREATEROLE. There's
>> an old legal maxim I learned in my distant youth that says "nemo dat
>> quod non habet" - Nobody can give something they don't own. This seems
>> to be in that spirit, and I approve :-)
>
> Great!  I'm glad to hear the approach has some support.

I'd also like to voice my support for this effort.  I haven't been
following this thread too closely, but I did take a gander at the
latest patch set.  There is a lot to unpack here.  I think this could
easily be split into 3 or 4 threads.

The changes for adding GUC management roles seem pretty
straightforward and would likely be helpful for service providers.
However, I was kind of surprised that membership to such roles also
provided access to ALTER SYSTEM SET.  IMO there's quite a big
difference between allowing a user to set a GUC per-session versus
cluster-wide.  With these patches, if I just want to allow a user to
set a GUC like temp_file_limit or log_statement, I also have to give
them the ability to change it (and several other GUCs) for all roles
on the system.

I haven't spent too much time looking at the event trigger and logical
replication changes yet.

For the CREATEROLE changes, the main thing on my mind is how this
might impact upgrades.  IIUC roles with CREATEROLE will lose many
privileges after pg_upgrade.  I think one way to deal with this would
be to have such upgrades grant all the privileges they are losing, but
most CREATEROLE roles likely aren't using the full extent of their
powers, so that approach may be a little extreme.  Perhaps it is okay
to just add a blurb in the release notes about this backwards-
incompatible change.

Another interesting thing I found is that if a role has ownership of
a role that later obtains SUPERUSER, the owning role basically loses
all control of the role.  It makes sense to avoid letting non-
superusers mess with superusers, but this led me to wonder if there
should be a mechanism for transferring role ownership (e.g., ALTER
ROLE or REASSIGNED OWNED BY).  Presently, REASSIGNED OWNED BY fails
with an "unexpected classid" ERROR.  Such functionality might also
come in handy for the pg_dump changes for maintaining role ownership.

Nathan

Re: Patch: Range Merge Join

[Jaime Casanova]

> On Mon, Oct 04, 2021 at 04:27:54PM -0500, Jaime Casanova wrote:
>> On Thu, Jun 10, 2021 at 07:14:32PM -0700, Jeff Davis wrote:
>> > 
>> > I'll start with the reason I set the work down before: it did not work
>> > well with multiple join keys. That might be fine, but I also started
>> > thinking it was specialized enough that I wanted to look into doing it
>> > as an extension using the CustomScan mechanism.
>> > 
>> > Do you have any solution to working better with multiple join keys? And
>> > do you have thoughts on whether it would be a good candidate for the
>> > CustomScan extension mechanism, which would make it easier to
>> > experiment with?
>> > 
>> 
>> Hi,
>> 
>> It seems this has been stalled since jun-2021. I intend mark this as
>> RwF unless someone speaks in the next hour or so.
>> 

Thomas  wrote me:

> Hi,
> 
> I registered this patch for the commitfest in july. It had not been reviewed 
> and moved to the next CF. I still like to submit it.
> 
> Regards,
> Thomas
>

Just for clarification RwF doesn't imply reject of the patch.
Nevertheless, given that there has been no real review I will mark this
patch as "Waiting on Author" and move it to the next CF.

Meanwhile, cfbot (aka http://commitfest.cputube.org) says this doesn't
compile. Here is a little patch to fix the compilation errors, after
that it passes all tests in make check-world.

Also attached a rebased version of your patch with the fixes so we turn
cfbot entry green again

-- 
Jaime Casanova
Director de Servicios Profesionales
SystemGuards - Consultores de PostgreSQL
diff --git a/src/backend/executor/nodeMergejoin.c 
b/src/backend/executor/nodeMergejoin.c
index 2dccba24b3..d18b4a4605 100644
--- a/src/backend/executor/nodeMergejoin.c
+++ b/src/backend/executor/nodeMergejoin.c
@@ -294,7 +294,7 @@ MJCreateRangeData(List *rangeclause,
 {
RangeData data;
 
-   Assert(list_legth(node->rangeclause) < 3);
+   Assert(list_length(rangeclause) < 3);
 
data = (RangeData) palloc0(sizeof(RangeJoinData));
 
diff --git a/src/backend/optimizer/path/costsize.c 
b/src/backend/optimizer/path/costsize.c
index 8a2778485c..24ee41b9ed 100644
--- a/src/backend/optimizer/path/costsize.c
+++ b/src/backend/optimizer/path/costsize.c
@@ -3440,8 +3440,8 @@ final_cost_mergejoin(PlannerInfo *root, MergePath *path,
 
 
 /* Protect some assumptions below that rowcounts aren't zero */
-if (inner_path_rows <= 0)
-inner_path_rows = 1;
+   if (inner_path_rows <= 0)
+   inner_path_rows = 1;
 
/* Mark the path with the correct row estimate */
if (path->jpath.path.param_info)
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index 10644dfac4..9b7503630c 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -1206,8 +1206,16 @@ ExplainNode(PlanState *planstate, List *ancestors,
 			pname = sname = "Nested Loop";
 			break;
 		case T_MergeJoin:
-			pname = "Merge";	/* "Join" gets added by jointype switch */
-			sname = "Merge Join";
+			if(((MergeJoin *) plan)->rangeclause)
+			{
+pname = "Range Merge";	/* "Join" gets added by jointype switch */
+sname = "Range Merge Join";
+			}
+			else
+			{
+pname = "Merge";	/* "Join" gets added by jointype switch */
+sname = "Merge Join";
+			}
 			break;
 		case T_HashJoin:
 			pname = "Hash";		/* "Join" gets added by jointype switch */
@@ -1948,6 +1956,8 @@ ExplainNode(PlanState *planstate, List *ancestors,
 		case T_MergeJoin:
 			show_upper_qual(((MergeJoin *) plan)->mergeclauses,
 			"Merge Cond", planstate, ancestors, es);
+			show_upper_qual(((MergeJoin *) plan)->rangeclause,
+			"Range Cond", planstate, ancestors, es);
 			show_upper_qual(((MergeJoin *) plan)->join.joinqual,
 			"Join Filter", planstate, ancestors, es);
 			if (((MergeJoin *) plan)->join.joinqual)
diff --git a/src/backend/executor/nodeMergejoin.c b/src/backend/executor/nodeMergejoin.c
index b41454ab6d..d18b4a4605 100644
--- a/src/backend/executor/nodeMergejoin.c
+++ b/src/backend/executor/nodeMergejoin.c
@@ -93,11 +93,15 @@
 #include "postgres.h"
 
 #include "access/nbtree.h"
+#include "catalog/pg_operator.h"
 #include "executor/execdebug.h"
 #include "executor/nodeMergejoin.h"
 #include "miscadmin.h"
+#include "nodes/nodeFuncs.h"
+#include "utils/rangetypes.h"
 #include "utils/lsyscache.h"
 #include "utils/memutils.h"
+#include "utils/typcache.h"
 
 
 /*
@@ -140,6 +144,18 @@ typedef struct MergeJoinClauseData
 	SortSupportData ssup;
 }			MergeJoinClauseData;
 
+/*
+ * Runtime data for the range clause
+ */
+typedef struct RangeJoinData
+{
+	ExprState *startClause;
+	ExprState *endClause;
+	ExprState *rangeExpr;
+	ExprState *elemExpr;
+
+}			RangeJoinData;
+
 /* Result type for MJEvalOuterValues and MJEvalInnerValues */
 typedef enum
 {
@@ -269,6 +285,57 @@ MJExamineQuals(List *mergeclauses,
 	return clauses;
 }
 
+/*
+ * MJCreateRangeData
+ */
+static RangeData
+MJCreateRangeData(List *rangeclause,
+

Re: BUG #17212: pg_amcheck fails on checking temporary relations

[Mark Dilger]

> On Oct 4, 2021, at 4:45 PM, Peter Geoghegan  wrote:
> 
> I'm guessing that you meant REINDEX CONCURRENTLY.

Yes.

> Since you're talking about the case where it has an error

Sorry, I realized after hitting  that you might take it that way, but I 
mean the logs generally, not just postgres logs.  If somebody runs "reindex 
concurrently" on all tables at midnight every night, and they see one morning 
in the (non-postgres) logs from the midnight hour weird error messages about 
their RAID controller, they may well want to check all their indexes to see if 
any of them were corrupted.  This is a totally made-up example, but the idea 
that a user might want to check their indexes, tables, or both owing to errors 
of some kind is not far-fetched.  

> , the whole
> index build must have failed. So the user would get exactly what
> they'd expect -- verification of the original index, without any
> hindrance from the new/failed index.

Right, in that case, but not if hardware errors corrupted the index, and 
generated logs, without happening to trip up the reindex concurrently statement 
itself.

> (Thinks for a moment...)
> 
> Actually, I think that we'd only verify the original index, even
> before the error with CONCURRENTLY (though I've not checked that point
> myself).

To get back on track, let me say that I'm not taking the position that what 
pg_amcheck currently does is necessarily correct, but just that I'd like to be 
careful about what we change, if anything.  There are three things that seem to 
irritate people:

1) A non-zero exit code means "not everything was checked and passed" rather 
than "at least one thing is definitely corrupt".

2) Skipping of indexes is reported to the user with the word 'ERROR' in the 
report rather than, say, 'NOTICE'.

3) Deadlocks can occur

I have resisted changing #1 on the theory that `pg_amcheck --all && 
./post_all_checks_pass.sh` should only run the post_all_checks_pass.sh if 
indeed all checks have passed, and I'm interpreting skipping an index check as 
being contrary to that.  But maybe that's wrong of me.  I don't know.  There is 
already sloppiness between the time that pg_amcheck resolves which database 
relations are matched by --all, --table, --index, etc. and the time that all 
the checks are started, and again between that time and when the last one is 
complete.  Database objects could be created or dropped during those spans of 
time, in which case --all doesn't have quite so well defined a meaning.  But 
the user running pg_amcheck might also *know* that they aren't running any such 
DDL, and therefore expect --all to actually result in everything being checked.

I find it strange that I should do anything about #2 in pg_amcheck, since it's 
the function in verify_nbtree that phrases the situation as an error.  But I 
suppose I can just ignore that and have it print as a notice.  I'm genuinely 
not trying to give you grief here -- I simply don't like that pg_amcheck is 
adding commentary atop what the checking functions are doing.  I see a clean 
division between what pg_amcheck is doing and what amcheck is doing, and this 
feels to me to put that on the wrong side of the divide.  If refusing to check 
the index because it is not in the requisite state is a notice, then why 
wouldn't verify_nbtree raise it as one and return early rather than raising an 
error?

I also find it strange that #3 is being attributed to pg_amcheck's choice of 
how to call the checking function, because I can't think of any other function 
where we require the SQL caller to do anything like what you are requiring here 
in order to prevent deadlocks, and also because the docs for the functions 
don't say that a deadlock is possible, merely that the function may return with 
an error.  I was totally content to get an error back, since errors are how the 
verify_nbtree functions communicate everything else, and the handler for those 
functions is already prepared to deal with the error messages so returned.  But 
it clearly annoys you that pg_amcheck is doing this, so I'll go forward with 
the patch that I already have written which does otherwise.


—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: plperl on windows

[Andres Freund]

Hi,

On 2021-10-04 14:38:16 -0700, Andres Freund wrote:
> 3) When building against strawberry perl 5.32.1.1 I see errors when loading
>plperl
>
> 4) When building against strawberry perl 5.30.3.1 I see a crash during
>execution of very simple statements [1]
>
> 5) Finally when building against strawberry perl 5.28.2.1, plperl kinda
>works. But there's a lot of regression test failures, many of them
>seemingly around error trapping.

Here's a CI run testing various strawberry perl versions on windows. I did
apply Victor's patch to make things at least compile on newer versions of perl.

https://cirrus-ci.com/build/6290387791773696
- 5.32.1.1: fails with "src/pl/plperl/Util.c: loadable library and perl 
binaries are mismatched (got handshake key 12800080, needed 
12900080)"
- 5.30.3.1: crashes in plperl_trusted_init(), see "cat_dumps" step for backtrace
- 5.28.2.1: doesn't crash, but lots of things don't seem to work, particularly
  around error handling (to see regression diff, click on regress_diffs near
  the top, and navigate to src/pl/plperl)
- 5.24.4.1 and 5.26.3.1: pass

The 5.32.1.1 issue looks like it might actually a problem in strawberry perl
perhaps? But the rest not so much.

Greetings,

Andres Freund

RE: Transactions involving multiple postgres foreign servers, take 2

[k.jami...@fujitsu.com]

Hi Sawada-san,

I noticed that this thread and its set of patches have been marked with 
"Returned with Feedback" by yourself.
I find the feature (atomic commit for foreign transactions) very useful
and it will pave the road for having a distributed transaction management in 
Postgres.
Although we have not arrived at consensus at which approach is best,
there were significant reviews and major patch changes in the past 2 years.
By any chance, do you have any plans to continue this from where you left off?

Regards,
Kirk Jamison

Re: Split xlog.c

[Jaime Casanova]

On Fri, Sep 17, 2021 at 12:10:17PM +0900, Kyotaro Horiguchi wrote:
> Hello.
> 
> At Thu, 16 Sep 2021 11:23:46 +0300, Heikki Linnakangas  
> wrote in 
> > Here is another rebase.
> 
> I have several comments on this.
> 

Hi Heikki,

Are we waiting a rebased version? Currently this does not apply to head.
I'll mark this as WoA and move it to necxt CF.

-- 
Jaime Casanova
Director de Servicios Profesionales
SystemGuards - Consultores de PostgreSQL

Re: (LOCK TABLE options) “ONLY” and “NOWAIT” are not yet implemented

[Fujii Masao]

On 2021/10/04 14:28, shinya11.k...@nttdata.com wrote:

The patch looks good to me, too. I applied cosmetic changes to it.
Attached is the updated version of the patch. Barring any objection, I will 
commit
it.

Thank you for the patch!
It looks good to me.


Pushed. Thanks!

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

Re: BUG #17212: pg_amcheck fails on checking temporary relations

[Peter Geoghegan]

On Mon, Oct 4, 2021 at 5:32 PM Mark Dilger  wrote:
> Sorry, I realized after hitting  that you might take it that way, but I 
> mean the logs generally, not just postgres logs.  If somebody runs "reindex 
> concurrently" on all tables at midnight every night, and they see one morning 
> in the (non-postgres) logs from the midnight hour weird error messages about 
> their RAID controller, they may well want to check all their indexes to see 
> if any of them were corrupted.

I don't see what the point of this example is. Why is the REINDEX
CONCURRENTLY index special here? Presumably the user is using
pg_amcheck with its -i option in this scenario, since you've scoped it
that way. Where did they get that index name from? Presumably it's
just the original familiar index name? How did an error message that's
not from the Postgres logs (or something similar) contain any index
name?

If the overnight rebuild completed successfully then we'll verify the
newly rebuilt smgr relfilenode for the index. It if failed then we'll
just verify the original. In other words, if we treat the validity of
indexes as a "visibility concern", everything works out just fine.

If there is an orphaned index (because of the implementation issue
with CONCURRENTLY) then it is *definitely* "corrupt" -- but not in any
sense that pg_amcheck ought to concern itself with. Such an orphaned
index can never actually be used by anybody. (We should fix this wart
in the CONCURRENTLY implementation some day.)

> To get back on track, let me say that I'm not taking the position that what 
> pg_amcheck currently does is necessarily correct, but just that I'd like to 
> be careful about what we change, if anything.  There are three things that 
> seem to irritate people:
>
> 1) A non-zero exit code means "not everything was checked and passed" rather 
> than "at least one thing is definitely corrupt".

Right.

> 2) Skipping of indexes is reported to the user with the word 'ERROR' in the 
> report rather than, say, 'NOTICE'.

Right -- but it's also the specifics of the error. These are errors
that only make sense when there was specific human error. Which is
clearly not the case at all, except perhaps in the narrow -i case.

> 3) Deadlocks can occur

Right.

> I have resisted changing #1 on the theory that `pg_amcheck --all && 
> ./post_all_checks_pass.sh` should only run the post_all_checks_pass.sh if 
> indeed all checks have passed, and I'm interpreting skipping an index check 
> as being contrary to that.

You're also interpreting it as "skipping". This is a subjective
interpretation. Which is fair enough - I can see why you'd put it that
way. But that's not how I see it. Again, consider that pg_dump cares
about the "indisready" status of indexes, for a variety of reasons.

Now, the pg_dump example doesn't necessarily mean that pg_amcheck
*must* do the same thing (though it certainly suggests as much). To me
the important point is that we are perfectly entitled to define "the
indexes that pg_amcheck can see" in whatever way seems to make most
sense overall, based on practical considerations.

> But the user running pg_amcheck might also *know* that they aren't running 
> any such DDL, and therefore expect --all to actually result in everything 
> being checked.

The user would also have to know precisely how the system catalogs
work during DDL. They'd have to know that the pg_class entry might
become visible very early on, rather than at the end, in some cases.
They'd know all that, but still be surprised by the current pg_amcheck
behavior. Which is itself not consistent with pg_dump.

> I find it strange that I should do anything about #2 in pg_amcheck, since 
> it's the function in verify_nbtree that phrases the situation as an error.

I don't find it strange. It does that because it *is* an error. There
is simply no alternative.

The solution for amcheck is the same as it has always been: just write
the SQL query in a way that avoids it entirely.

> I'm genuinely not trying to give you grief here -- I simply don't like that 
> pg_amcheck is adding commentary atop what the checking functions are doing.

pg_amcheck would not be adding commentary if this was addressed in the
way that I have in mind. It would merely be dealing with the issue in
the way that the amcheck docs have recommended, for years. The problem
here (as I see it) is that pg_amcheck is already adding commentary, by
not just doing that.

> I also find it strange that #3 is being attributed to pg_amcheck's choice of 
> how to call the checking function, because I can't think of any other 
> function where we require the SQL caller to do anything like what you are 
> requiring here in order to prevent deadlocks, and also because the docs for 
> the functions don't say that a deadlock is possible, merely that the function 
> may return with an error.

I will need to study the deadlock issue separately.

-- 
Peter Geoghegan

Re: Added schema level support for publication.

[Greg Nancarrow]

On Mon, Oct 4, 2021 at 4:55 AM vignesh C  wrote:
>
> Attached v36 patch has the changes for the same.
>

I have some comments on the v36-0001 patch:

src/backend/commands/publicationcmds.c

(1)
GetPublicationSchemas()

+ /* Find all publications associated with the schema */
+ pubschsrel = table_open(PublicationNamespaceRelationId, AccessShareLock);

I think the comment is not correct. It should say:

+ /* Find all schemas associated with the publication */

(2)
AlterPublicationSchemas

I think that a comment should be added for the following lines,
something like the comment used in the similar check in
AlterPublicationTables():

+ if (!schemaidlist && stmt->action != DEFELEM_SET)
+ return;

(3)
CheckAlterPublication

Minor comment fix suggested:

BEFORE:
+ * Check if relations and schemas can be in given publication and throws
AFTER:
+ * Check if relations and schemas can be in a given publication and throw

(4)
LockSchemaList()

Suggest re-word of comment, to match imperative comment style used
elsewhere in this code.

BEFORE:
+ * The schemas specified in the schema list are locked in AccessShareLock mode
AFTER:
+ * Lock the schemas specified in the schema list in AccessShareLock mode


src/backend/commands/tablecmds.c

(5)

Code has been added to prevent a table being set (via ALTER TABLE) to
UNLOGGED if it is part of a publication, but I found that I could
still add all tables of a schema having a table that is UNLOGGED:

postgres=# create schema sch;
CREATE SCHEMA
postgres=# create unlogged table sch.test(i int);
CREATE TABLE
postgres=# create publication pub for table sch.test;
ERROR:  cannot add relation "test" to publication
DETAIL:  Temporary and unlogged relations cannot be replicated.
postgres=# create publication pub for all tables in schema sch;
CREATE PUBLICATION


Regards,
Greg Nancarrow
Fujitsu Australia

Re: Transactions involving multiple postgres foreign servers, take 2

[Masahiko Sawada]

Hi,

On Tue, Oct 5, 2021 at 9:56 AM k.jami...@fujitsu.com
 wrote:
>
> Hi Sawada-san,
>
> I noticed that this thread and its set of patches have been marked with 
> "Returned with Feedback" by yourself.
> I find the feature (atomic commit for foreign transactions) very useful
> and it will pave the road for having a distributed transaction management in 
> Postgres.
> Although we have not arrived at consensus at which approach is best,
> there were significant reviews and major patch changes in the past 2 years.
> By any chance, do you have any plans to continue this from where you left off?

As I could not reply to the review comments from Fujii-san for almost
three months, I don't have enough time to move this project forward at
least for now. That's why I marked this patch as RWF. I’d like to
continue working on this project in my spare time but I know this is
not a project that can be completed by using only my spare time. If
someone wants to work on this project, I’d appreciate it and am happy
to help.

Regards,

-- 
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

Re: corruption of WAL page header is never reported

[Kyotaro Horiguchi]

At Tue, 5 Oct 2021 00:59:46 +0900, Fujii Masao  
wrote in 
> I think that it's better to comment why "retry" is not necessary
> when not in standby mode.
> 
> ---
> When not in standby mode, an invalid page header should cause recovery
> to end, not retry reading the page, so we don't need to validate the
> page
> header here for the retry. Instead, ReadPageInternal() is responsible
> for
> the validation.

LGTM.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: storing an explicit nonce

[Stephen Frost]

Greetings,

* Ants Aasma (a...@cybertec.at) wrote:
> On Mon, 27 Sept 2021 at 23:34, Bruce Momjian  wrote:
> > On Sun, Sep  5, 2021 at 10:51:42PM +0800, Sasasu wrote:
> > > It looks like we are still considering AES-CBC, AES-XTS, and
> > AES-GCM(-SIV).
> > > I want to say something that we don't think about.
> > >
> > > For AES-CBC, the IV should be not predictable. I think LSN or HASH(LSN,
> > > block number or something) is predictable. There are many CVE related to
> > > AES-CBC with a predictable IV.
> >
> > The LSN would change every time the page is modified, so while the LSN
> > could be predicted, it would not be reused.  However, there is currently
> > no work being done on page-level encryption of Postgres.
> >
> 
> We are still working on our TDE patch. Right now the focus is on
> refactoring temporary file access to make the TDE patch itself smaller.
> Reconsidering encryption mode choices given concerns expressed is next.
> Currently a viable option seems to be AES-XTS with LSN added into the IV.
> XTS doesn't have an issue with predictable IV and isn't totally broken in
> case of IV reuse.

Probably worth a distinct thread to discuss this, just to be clear.

I do want to point out, as I think I did when we discussed this but want
to be sure it's also captured here- I don't think that temporary file
access should be forced to be block-oriented when it's naturally (in
very many cases) sequential.  To that point, I'm thinking that we need a
temp file access API through which various systems work that's
sequential and therefore relatively similar to the existing glibc, et
al, APIs, but by going through our own internal API (which more
consistently works with the glibc APIs and provides better error
reporting in the event of issues, etc) we can then extend it to work as
an encrypted stream instead.

Happy to discuss in more detail if you'd like but wanted to just bring
up this particular point, in case it got lost.

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: Delegating superuser tasks to new security roles (Was: Granting control of SUSET gucs to non-superusers)

[Stephen Frost]

Greetings,

* Bossart, Nathan (bossa...@amazon.com) wrote:
> On 9/27/21, 11:16 AM, "Mark Dilger"  wrote:
> > On Sep 21, 2021, at 12:58 PM, Andrew Dunstan  wrote:
> >> I do like the basic thrust of reducing the power of CREATEROLE. There's
> >> an old legal maxim I learned in my distant youth that says "nemo dat
> >> quod non habet" - Nobody can give something they don't own. This seems
> >> to be in that spirit, and I approve :-)
> >
> > Great!  I'm glad to hear the approach has some support.
> 
> I'd also like to voice my support for this effort.  I haven't been
> following this thread too closely, but I did take a gander at the
> latest patch set.  There is a lot to unpack here.  I think this could
> easily be split into 3 or 4 threads.

I tend to agree.  I'm also generally supportive but following everything
that's going on in this particular patch set isn't easy.

> For the CREATEROLE changes, the main thing on my mind is how this
> might impact upgrades.  IIUC roles with CREATEROLE will lose many
> privileges after pg_upgrade.  I think one way to deal with this would
> be to have such upgrades grant all the privileges they are losing, but
> most CREATEROLE roles likely aren't using the full extent of their
> powers, so that approach may be a little extreme.  Perhaps it is okay
> to just add a blurb in the release notes about this backwards-
> incompatible change.

This is definitely a pretty big change.  There needs to be a bigger and
independent discussion about the general concept of role 'self
administration' as we talk about it in the comments of the role system
and which this doesn't really address too.  I've been planning for a
while to start a specific thread about that and I'll try to do that so
that we can discuss that specifically, as it's quite relevant to all of
this, in my view.

> Another interesting thing I found is that if a role has ownership of
> a role that later obtains SUPERUSER, the owning role basically loses
> all control of the role.  It makes sense to avoid letting non-
> superusers mess with superusers, but this led me to wonder if there
> should be a mechanism for transferring role ownership (e.g., ALTER
> ROLE or REASSIGNED OWNED BY).  Presently, REASSIGNED OWNED BY fails
> with an "unexpected classid" ERROR.  Such functionality might also
> come in handy for the pg_dump changes for maintaining role ownership.

I really think we need to stop addressing roles explicitly as
'superuser' vs. 'non-superuser', because a non-superuser role can be
GRANT'd a superuser role, which makes that distinction really not
sensible.  This has continued to be a problem and we need to cleanly
address it.  Not sure exactly how to do that today but it's certainly an
issue.

Thanks,

Stephen


signature.asc
Description: PGP signature

Re: parallelizing the archiver

[Stephen Frost]

Greetings,

* Bossart, Nathan (bossa...@amazon.com) wrote:
> On 10/1/21, 12:08 PM, "Andrey Borodin"  wrote:
> > 30 сент. 2021 г., в 09:47, Bossart, Nathan  написал(а):
> >> Of course, there are drawbacks to using an extension.  Besides the
> >> obvious added complexity of building an extension in C versus writing
> >> a shell command, the patch disallows changing the libraries without
> >> restarting the server.  Also, the patch makes no effort to simplify
> >> error handling, memory management, etc.  This is left as an exercise
> >> for the extension author.
> > I think the real problem with extension is quite different than mentioned 
> > above.
> > There are many archive tools that already feature parallel archiving. 
> > PgBackrest, wal-e, wal-g, pg_probackup, pghoard, pgbarman and others. These 
> > tools by far outweight tools that don't look into archive_status to 
> > parallelize archival.
> > And we are going to ask them: add also a C extension without any feasible 
> > benefit to the user. You only get some restrictions like system restart to 
> > enable shared library.
> >
> > I think we need a design that legalises already existing de-facto standard 
> > features in archive tools. Or event better - enables these tools to be more 
> > efficient, reliable etc. Either way we will create legacy code from the 
> > scratch.
> 
> My proposal wouldn't require any changes to any of these utilities.
> This design just adds a new mechanism that would allow end users to
> set up archiving a different way with less overhead in hopes that it
> will help them keep up.  I suspect a lot of work has been put into the
> archive tools you mentioned to make sure they can keep up with high
> rates of WAL generation, so I'm skeptical that anything we do here
> will really benefit them all that much.  Ideally, we'd do something
> that improves matters for everyone, though.  I'm open to suggestions.

This has something we've contemplated quite a bit and the last thing
that I'd want to have is a requirement to configure a whole bunch of
additional parameters to enable this.  Why do we need to have some many
new GUCs?  I would have thought we'd probably be able to get away with
just having the appropriate hooks and then telling folks to load the
extension in shared_preload_libraries..

As for the hooks themselves, I'd certainly hope that they'd be designed
to handle batches of WAL rather than individual ones as that's long been
one of the main issues with the existing archive command approach.  I
appreciate that maybe that's less of an issue with a shared library but
it's still something to consider.

Admittedly, I haven't looked in depth with this patch set and am just
going off of the description of them provided in the thread, so perhaps
I missed something.

Thanks,

Stephen


signature.asc
Description: PGP signature

Re: Triage on old commitfest entries

[Stephen Frost]

Greetings,

* Peter Geoghegan (p...@bowt.ie) wrote:
> On Sun, Oct 3, 2021 at 1:30 PM Tom Lane  wrote:
> > Fair.  My concern here is mostly that we not just keep kicking the
> > can down the road.  If we see that a patch has been hanging around
> > this long without reaching commit, we should either kill it or
> > form a specific plan for how to advance it.
> 
> Also fair.
> 
> The pandemic has made the kind of coordination I refer to harder in
> practice. It's the kind of thing that face to face communication
> really helps with.

Entirely agree with this.  Index skip scan is actually *ridiculously*
useful in terms of an improvement, and we need to get the right people
together to work on it and get it implemented.  I'd love to see this
done for v15, in particular.  Who do we need to coordinate getting
together to make it happen?  I doubt that I'm alone in wanting to make
this happen and I'd be pretty surprised if we weren't able to bring the
right folks together this fall to make it a reality.

Thanks,

Stephen


signature.asc
Description: PGP signature

Re: Triage on old commitfest entries

[Tom Lane]

Stephen Frost  writes:
> Entirely agree with this.  Index skip scan is actually *ridiculously*
> useful in terms of an improvement, and we need to get the right people
> together to work on it and get it implemented.  I'd love to see this
> done for v15, in particular.  Who do we need to coordinate getting
> together to make it happen?

It sounds like Peter is willing to take point on the executor end
of things (b-tree in particular).  If he can explain what a reasonable
cost model would look like, I'm willing to see about making that happen
in the planner.

regards, tom lane

Role Self-Administration

[Stephen Frost]

Greetings,

There's been various discussions about CREATEROLE, EVENT TRIGGERs, and
other things which hinge around the general idea that we can create a
'tree' of roles where there's some root and then from that root there's
a set of created roles, or at least roles which have been GRANT'd other
roles as part of an explicit arrangement.

The issue with many of these suggestions is that roles, currently, are
able to 'administer' themselves.  That means that such role memberships
aren't suitable for such controls. 

To wit, this happens:

Superuser:

=# create user u1;
CREATE ROLE
=# create user u2;
CREATE ROLE
=# grant u2 to u1;
GRANT ROLE

...

Log in as u2:

=> revoke u2 from u1;
REVOKE ROLE

...

This is because we allow 'self administration' of roles, meaning that
they can decide what other roles they are a member of.  This is
documented as:

"A role is not considered to hold WITH ADMIN OPTION on itself, but it
may grant or revoke membership in itself from a database session where
the session user matches the role."

at: https://www.postgresql.org/docs/current/sql-grant.html

Further, we comment this in the code:

 * A role can admin itself when it matches the session user and we're
 * outside any security-restricted operation, SECURITY DEFINER or
 * similar context.  SQL-standard roles cannot self-admin.  However,
 * SQL-standard users are distinct from roles, and they are not
 * grantable like roles: PostgreSQL's role-user duality extends the
 * standard.  Checking for a session user match has the effect of
 * letting a role self-admin only when it's conspicuously behaving
 * like a user.  Note that allowing self-admin under a mere SET ROLE
 * would make WITH ADMIN OPTION largely irrelevant; any member could
 * SET ROLE to issue the otherwise-forbidden command.

in src/backend/utils/adt/acl.c

Here's the thing - having looked back through the standard, it seems
we're missing a bit that's included there and that makes a heap of
difference.  Specifically, the SQL standard basically says that to
revoke a privilege, you need to have been able to grant that privilege
in the first place (as Andrew Dunstan actually also brought up in a
recent thread about related CREATEROLE things- 
https://www.postgresql.org/message-id/837cc50a-532a-85f5-a231-9d68f2184e52%40dunslane.net
) and that isn't something we've been considering when it comes to role
'self administration' thus far, at least as it relates to the particular
field of the "grantor".

We can't possibly make things like EVENT TRIGGERs or CREATEROLE work
with role trees if a given role can basically just 'opt out' of being
part of the tree to which they were assigned by the user who created
them.  Therefore, I suggest we contemplate two changes in this area:

- Allow a user who is able to create roles decide if the role created is
  able to 'self administor' (that is- GRANT their own role to someone
  else) itself.

- Disallow roles from being able to REVOKE role membership that they
  didn't GRANT in the first place.

This isn't as big a change as it might seem as we already track which
role issued a given GRANT.  We should probably do a more thorough review
to see if there's other cases where a given role is able to REVOKE
rights that have been GRANT'd by some other role on a particular object,
as it seems like we should probably be consistent in this regard across
everything and not just for roles.  That might be a bit of a pain but it
seems likely to be worth it in the long run and feels like it'd bring us
more in-line with the SQL standard too.

So, thoughts?

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: parallelizing the archiver

[Bossart, Nathan]

On 10/4/21, 7:21 PM, "Stephen Frost"  wrote:
> This has something we've contemplated quite a bit and the last thing
> that I'd want to have is a requirement to configure a whole bunch of
> additional parameters to enable this.  Why do we need to have some many
> new GUCs?  I would have thought we'd probably be able to get away with
> just having the appropriate hooks and then telling folks to load the
> extension in shared_preload_libraries..

That would certainly simplify my patch quite a bit.  I'll do it this
way in the next revision.

> As for the hooks themselves, I'd certainly hope that they'd be designed
> to handle batches of WAL rather than individual ones as that's long been
> one of the main issues with the existing archive command approach.  I
> appreciate that maybe that's less of an issue with a shared library but
> it's still something to consider.

Will do.  This seems like it should be easier with the hook because we
can provide a way to return which files were successfully archived.

Nathan

Re: plperl: update ppport.h and fix configure version check

[Tom Lane]

=?utf-8?Q?Dagfinn_Ilmari_Manns=C3=A5ker?=  writes:
> Andres' recent post¹ about PL/Perl on Windows and linked-to² patch
> containing an erroneous version check made me realise that we haven't
> updated our copy of ppport.h since 2009.  Attached is a patch that does
> that, and applies code changes suggested by running it.  I've tested
> `make check-world` with `--with-perl` on both the oldest (5.8.9) and
> newest (5.34.0) perls I have handy.

I haven't looked at this patch's details, but I can confirm that it
also builds and passes regression on prairiedog's 5.8.3 perl.

> I also noticed that PL/Perl itself (via plc_perlboot.pl) requires Perl
> 5.8.1, but configure only checks for 5.8 (i.e. 5.8.0).  The second patch
> updates the latter to match.

Hmm ... Perl 5.8.x is old enough that probably it matters to nobody in
the real world, but if we're going to mess with this, is 5.8.1 the right
cutoff?  I wonder about this because I believe prairiedog's perl to be
the oldest that we have tested in a good long while, so that we shouldn't
assert with any confidence that 5.8.1 would actually work.  The last
time I surveyed the buildfarm's perl versions, in 2017, these were the
only 5.8.x animals:

 Animal| Surveyed build  | Configure's version report
 castoroides   | 2017-07-27 12:03:05 | configure: using perl 5.8.4
 protosciurus  | 2017-07-27 13:24:42 | configure: using perl 5.8.4
 prairiedog| 2017-07-27 22:51:11 | configure: using perl 5.8.6
 aholehole | 2017-07-27 19:31:40 | configure: using perl 5.8.8
 anole | 2017-07-28 00:27:38 | configure: using perl 5.8.8
 arapaima  | 2017-07-27 19:30:52 | configure: using perl 5.8.8
 gharial   | 2017-07-27 20:26:16 | configure: using perl 5.8.8
 locust| 2017-07-28 00:13:01 | configure: using perl 5.8.8
 narwhal   | 2017-03-17 05:00:02 | configure: using perl 5.8.8
 gaur  | 2017-07-22 21:02:43 | configure: using perl 5.8.9
 pademelon | 2017-07-22 23:56:59 | configure: using perl 5.8.9

Notice that here, prairiedog is running 5.8.6, which is Apple's
vendor-installed perl on that stone-age version of macOS.
Shortly after that, I *downgraded* it to 5.8.3.  I do not recall
exactly why I chose that precise perl version, but it seems
pretty likely that the reason was "I couldn't get anything older
to build".

In short: (a) we're not testing against anything older than 5.8.3
and (b) it seems quite unlikely that anybody cares about 5.8.x anyway.
So if we want to mess with this, maybe we should set the cutoff
to 5.8.3 not 5.8.1.

regards, tom lane

Re: parallelizing the archiver

[Stephen Frost]

Greetings,

* Bossart, Nathan (bossa...@amazon.com) wrote:
> On 10/4/21, 7:21 PM, "Stephen Frost"  wrote:
> > This has something we've contemplated quite a bit and the last thing
> > that I'd want to have is a requirement to configure a whole bunch of
> > additional parameters to enable this.  Why do we need to have some many
> > new GUCs?  I would have thought we'd probably be able to get away with
> > just having the appropriate hooks and then telling folks to load the
> > extension in shared_preload_libraries..
> 
> That would certainly simplify my patch quite a bit.  I'll do it this
> way in the next revision.
> 
> > As for the hooks themselves, I'd certainly hope that they'd be designed
> > to handle batches of WAL rather than individual ones as that's long been
> > one of the main issues with the existing archive command approach.  I
> > appreciate that maybe that's less of an issue with a shared library but
> > it's still something to consider.
> 
> Will do.  This seems like it should be easier with the hook because we
> can provide a way to return which files were successfully archived.

It's also been discussed, at least around the water cooler (as it were
in pandemic times- aka our internal slack channels..) that the existing
archive command might be reimplemented as an extension using these.  Not
sure if that's really necessary but it was a thought.  In any case,
thanks for working on this!

Stephen


signature.asc
Description: PGP signature

Re: BUG #17212: pg_amcheck fails on checking temporary relations

[Mark Dilger]

> On Oct 4, 2021, at 6:19 PM, Peter Geoghegan  wrote:
> 
> I don't see what the point of this example is.

It doesn't matter.

I am changing pg_amcheck to filter out indexes as you say.  Since the btree 
check should no longer error in these cases, the issue of pg_amcheck exit(2) 
sorts itself out without further code changes.

I am changing verify_heapam to skip unlogged tables during recovery.  In 
testing, checking such a table results in a simple notice:

  NOTICE:  cannot verify unlogged relation "u_tbl" during recovery, skipping

While testing, I also created an index on the unlogged table and checked that 
index using bt_index_parent_check, and was surprised that checking it using 
bt_index_parent_check raises an error:

  ERROR:  cannot acquire lock mode ShareLock on database objects while recovery 
is in progress
  HINT:  Only RowExclusiveLock or less can be acquired on database objects 
during recovery.

It doesn't get as far as btree_index_mainfork_expected().  So I am changing 
pg_amcheck to filter out indexes when pg_is_in_recovery() is true and 
relpersistence='u'.  Does that sound right to you?

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: Make relfile tombstone files conditional on WAL level

[Thomas Munro]

On Thu, Sep 30, 2021 at 11:32 PM Thomas Munro  wrote:
> I managed to produce a case where live data is written to an unlinked
> file and lost

I guess this must have been broken since release 9.2 moved checkpoints
out of here[1].  The connection between checkpoints, tombstone files
and file descriptor cache invalidation in auxiliary (non-sinval)
backends was not documented as far as I can see (or at least not
anywhere near the load-bearing parts).

How could it be fixed, simply and backpatchably?  If BgSyncBuffer()
did if-FirstCallSinceLastCheckpoint()-then-smgrcloseall() after
locking each individual buffer and before flushing, then I think it
might logically have the correct interlocking against relfilenode
wraparound, but that sounds a tad expensive :-(  I guess it could be
made cheaper by using atomics for the checkpoint counter instead of
spinlocks.  Better ideas?

[1] 
https://www.postgresql.org/message-id/flat/CA%2BU5nMLv2ah-HNHaQ%3D2rxhp_hDJ9jcf-LL2kW3sE4msfnUw9gA%40mail.gmail.com

Re: Delegating superuser tasks to new security roles (Was: Granting control of SUSET gucs to non-superusers)

[Bossart, Nathan]

On 10/4/21, 7:08 PM, "Stephen Frost"  wrote:
> I really think we need to stop addressing roles explicitly as
> 'superuser' vs. 'non-superuser', because a non-superuser role can be
> GRANT'd a superuser role, which makes that distinction really not
> sensible.  This has continued to be a problem and we need to cleanly
> address it.  Not sure exactly how to do that today but it's certainly an
> issue.

Agreed.  Maybe one option is to convert most of the role attributes to
be predefined roles.  Then we could just check for membership in
pg_superuser instead of trying to deal with membership in roles that
have the SUPERUSER attribute.

Nathan

Re: BUG #17212: pg_amcheck fails on checking temporary relations

[Peter Geoghegan]

On Mon, Oct 4, 2021 at 8:19 PM Mark Dilger  wrote:
> I am changing pg_amcheck to filter out indexes as you say.  Since the btree 
> check should no longer error in these cases, the issue of pg_amcheck exit(2) 
> sorts itself out without further code changes.

Cool.

> I am changing verify_heapam to skip unlogged tables during recovery.  In 
> testing, checking such a table results in a simple notice:
>
>   NOTICE:  cannot verify unlogged relation "u_tbl" during recovery, skipping

That makes sense to me.

> While testing, I also created an index on the unlogged table and checked that 
> index using bt_index_parent_check, and was surprised that checking it using 
> bt_index_parent_check raises an error:
>
>   ERROR:  cannot acquire lock mode ShareLock on database objects while 
> recovery is in progress
>   HINT:  Only RowExclusiveLock or less can be acquired on database objects 
> during recovery.

Calling bt_index_parent_check() in hot standby mode is kind of asking
for it to error-out. It requires a ShareLock on the relation, which is
inherently not possible during recovery. So I don't feel too badly
about letting it just happen.

> So I am changing pg_amcheck to filter out indexes when pg_is_in_recovery() is 
> true and relpersistence='u'.  Does that sound right to you?

Yes, that all sounds right to me.

Thanks
-- 
Peter Geoghegan

Re: parallelizing the archiver

[Bossart, Nathan]

On 10/4/21, 8:19 PM, "Stephen Frost"  wrote:
> It's also been discussed, at least around the water cooler (as it were
> in pandemic times- aka our internal slack channels..) that the existing
> archive command might be reimplemented as an extension using these.  Not
> sure if that's really necessary but it was a thought.  In any case,
> thanks for working on this!

Interesting.  I like the idea of having one code path for everything
instead of branching for the hook and non-hook paths.  Thanks for
sharing your thoughts.

Nathan

Re: [PATCH] document

[Fujii Masao]

On 2021/10/04 15:18, Fujii Masao wrote:



On 2021/08/26 1:39, Justin Pryzby wrote:

On Wed, Aug 25, 2021 at 09:50:13AM -0400, Tom Lane wrote:

Fujii Masao  writes:

When I applied the patch to the master, I found that the table entries for
those function were added into the table for aclitem functions in the docs.
I think this is not valid position and needs to be moved to the proper one
(maybe the table for system catalog information functions?).


You have to be very careful these days when applying stale patches to
func.sgml --- there's enough duplicate boilerplate that "patch' can easily
be fooled into dumping an addition into the wrong place.  I doubt that
the submitter meant the doc addition to go there.


I suppose one solution to this is to use git format-patch -U11 or similar, at
least for doc/


Yes. I moved the desriptions of the function into the table for
system catalog information functions, and made the patch by using
git diff -U6. Patch attached. Barring any objection, I'm thinking
to commit it.


Pushed. Thanks!

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

Re: Added schema level support for publication.

[Amit Kapila]

On Tue, Oct 5, 2021 at 6:57 AM Greg Nancarrow  wrote:
>
> (5)
>
> Code has been added to prevent a table being set (via ALTER TABLE) to
> UNLOGGED if it is part of a publication, but I found that I could
> still add all tables of a schema having a table that is UNLOGGED:
>
> postgres=# create schema sch;
> CREATE SCHEMA
> postgres=# create unlogged table sch.test(i int);
> CREATE TABLE
> postgres=# create publication pub for table sch.test;
> ERROR:  cannot add relation "test" to publication
> DETAIL:  Temporary and unlogged relations cannot be replicated.
> postgres=# create publication pub for all tables in schema sch;
> CREATE PUBLICATION
>

What about when you use "create publication pub for all tables;"? I
think that also works, now on similar lines shouldn't the behavior of
"all tables in schema" publication be the same? I mean if we want we
can detect and give an error but is it advisable to give an error if
there are just one or few tables in schema that are unlogged?

-- 
With Regards,
Amit Kapila.

Re: On login trigger: take three

[Greg Nancarrow]

On Wed, Sep 15, 2021 at 11:32 PM Greg Nancarrow  wrote:
>
> I've attached the updated patch.
>

Attached a rebased version of the patch, as it was broken by fairly
recent changes (only very minor change to the previous version).

Regards,
Greg Nancarrow
Fujitsu Australia


v19-0001-Add-a-new-login-event-and-login-trigger-support.patch
Description: Binary data

Re: Role Self-Administration

[Noah Misch]

On Mon, Oct 04, 2021 at 10:57:46PM -0400, Stephen Frost wrote:
> "A role is not considered to hold WITH ADMIN OPTION on itself, but it
> may grant or revoke membership in itself from a database session where
> the session user matches the role."

> Here's the thing - having looked back through the standard, it seems
> we're missing a bit that's included there and that makes a heap of
> difference.  Specifically, the SQL standard basically says that to
> revoke a privilege, you need to have been able to grant that privilege
> in the first place (as Andrew Dunstan actually also brought up in a
> recent thread about related CREATEROLE things- 
> https://www.postgresql.org/message-id/837cc50a-532a-85f5-a231-9d68f2184e52%40dunslane.net
> ) and that isn't something we've been considering when it comes to role
> 'self administration' thus far, at least as it relates to the particular
> field of the "grantor".

Which SQL standard clauses are you paraphrasing?  (A reference could take the
form of a spec version number, section number, and rule number.  Alternately,
a page number and URL to a PDF would suffice.)

> We can't possibly make things like EVENT TRIGGERs or CREATEROLE work
> with role trees if a given role can basically just 'opt out' of being
> part of the tree to which they were assigned by the user who created
> them.  Therefore, I suggest we contemplate two changes in this area:

I suspect we'll regret using the GRANT system to modify behaviors other than
whether or not one gets "permission denied".  Hence, -1 on using role
membership to control event trigger firing, whether or not $SUBJECT changes.

> - Allow a user who is able to create roles decide if the role created is
>   able to 'self administor' (that is- GRANT their own role to someone
>   else) itself.
> 
> - Disallow roles from being able to REVOKE role membership that they
>   didn't GRANT in the first place.

Either of those could be reasonable.  Does the SQL standard take a position
relevant to the decision?  A third option is to track each role's creator and
make is_admin_of_role() return true for the creator, whether or not the
creator remains a member.  That would also benefit cases where the creator is
rolinherit and wants its ambient privileges to shed the privileges of the role
it's creating.

> We should probably do a more thorough review
> to see if there's other cases where a given role is able to REVOKE
> rights that have been GRANT'd by some other role on a particular object,
> as it seems like we should probably be consistent in this regard across
> everything and not just for roles.  That might be a bit of a pain but it
> seems likely to be worth it in the long run and feels like it'd bring us
> more in-line with the SQL standard too.

Does the SQL standard take a position on whether REVOKE SELECT should work
that way?

Re: Triage on old commitfest entries

[Peter Geoghegan]

On Mon, Oct 4, 2021 at 7:45 PM Tom Lane  wrote:
> It sounds like Peter is willing to take point on the executor end
> of things (b-tree in particular).  If he can explain what a reasonable
> cost model would look like, I'm willing to see about making that happen
> in the planner.

I would be happy to work with you on this. It's clearly an important project.

Having you involved with the core planner aspects (as well as general
design questions) significantly derisks everything. That's *very*
valuable to me.

-- 
Peter Geoghegan

Re: Added schema level support for publication.

[Greg Nancarrow]

On Tue, Oct 5, 2021 at 3:11 PM Amit Kapila  wrote:
>
> > Code has been added to prevent a table being set (via ALTER TABLE) to
> > UNLOGGED if it is part of a publication, but I found that I could
> > still add all tables of a schema having a table that is UNLOGGED:
> >
> > postgres=# create schema sch;
> > CREATE SCHEMA
> > postgres=# create unlogged table sch.test(i int);
> > CREATE TABLE
> > postgres=# create publication pub for table sch.test;
> > ERROR:  cannot add relation "test" to publication
> > DETAIL:  Temporary and unlogged relations cannot be replicated.
> > postgres=# create publication pub for all tables in schema sch;
> > CREATE PUBLICATION
> >
>
> What about when you use "create publication pub for all tables;"? I
> think that also works, now on similar lines shouldn't the behavior of
> "all tables in schema" publication be the same? I mean if we want we
> can detect and give an error but is it advisable to give an error if
> there are just one or few tables in schema that are unlogged?
>

OK, it seems that for the ALL TABLES case, there is no such error
check, and it just silently skips replication of any
temporary/unlogged tables. This is intentional, right?
I couldn't see any documentation specifically related to this, so I
think perhaps it should be updated to describe this behaviour. At the
moment, the existing documentation just states FOR TABLE that
"Temporary tables, unlogged tables, foreign tables, materialized
views, and regular views cannot be part of a publication".
Yes, I agree that ALL TABLES IN SCHEMA should behave the same as the
ALL TABLES case.
Problem is, shouldn't setting UNLOGGED on a table only then be
disallowed if that table was publicised using FOR TABLE?

With the patch applied:

postgres=# create publication pub3 for all tables in schema sch;
CREATE PUBLICATION
postgres=# create table sch.test3(i int);
CREATE TABLE
postgres=# alter table sch.test3 set unlogged;
ERROR:  cannot change table "test3" to unlogged because it is part of
a publication
DETAIL:  Unlogged relations cannot be replicated.


Regards,
Greg Nancarrow
Fujitsu Australia

Re: func.sgml

[Tatsuo Ishii]

>> You have to be very careful these days when applying stale patches to
>> func.sgml --- there's enough duplicate boilerplate that "patch' can easily
>> be fooled into dumping an addition into the wrong place. 
> 
> This is yet another indication to me that there's probably a good case
> for breaking func.sgml up into sections. It is by a very large margin
> the biggest file in our document sources (the next largest is less than
> half the number of lines).

I am welcome this by a different reason. I have been involved in a
translation (to Japanese) project for long time. For this work we are
using Github. Translation works are submitted as pull requests. With
large sgml files (not only func.sgml, but config.sgml, catalogs.sgml
and libpq.sgml), Github's UI cannot handle them correctly. Sometimes
they don't show certain lines, which makes the review process
significantly hard.  Because of this, we have to split those large
sgml files into small files, typically 4 to 5 segments for each large
sgml file.

Splitting those large sgml files in upstream woudl greatly help us
because we don't need to split the large sgml files.
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp