Re: Track replica origin progress for Rollback Prepared

[Michael Paquier]

On Wed, Mar 03, 2021 at 09:06:11AM +0530, Amit Kapila wrote:
> Michael, I want to push this patch soon unless you have any concerns.

No concerns from me...

> This is required for correctness if any plugin uses 2PC and origins to
> track the progress. Now, that we have exposed the two_phase option via
> a plugin, it is good if we get this in as well.

But I find the way of doing this feature integration strange.  The
patch posted on this thread is 0002 in the patch series posted in [1],
and it seems to me that the test 020_twophase.pl in 0005 requires
0001~0004 to work properly, while 022_twophase_cascade.pl from 0005
needs 0006 on top of the rest.  So taken as a whole, this would work,
but applied independently, patches may or may not fail a portion of
the tests.  It seems to me that the tests should be grouped with the
features they apply with, in a single commit.

That's perhaps not enough to be an actual objection, but I am not
really comfortable with the concept of pushing into the tree code that
would remain untested until all the remaining pieces get done, as that
means that we have a point in the code history where some code is
around, but sits around as idle, no?  If this feature is not completed
within the dev cycle, it would mean that some code remains around
without any actual way to know if it is useful or not, released as
such.

[1]: 
https://www.postgresql.org/message-id/cafpthdz2rigof0om0obhv1yrmymo5-sqft9fclyj-jp9shx...@mail.gmail.com
--
Michael


signature.asc
Description: PGP signature

Re: PITR promote bug: Checkpointer writes to older timeline

[Kyotaro Horiguchi]

At Thu, 4 Mar 2021 14:57:13 +0900, Fujii Masao  
wrote in 
> > read_local_xlog_page() works as a part of logical decoding and has
> > responsibility to update ThisTimeLineID properly.  As the comment in
> > the function, it is the proper place to update ThisTimeLineID since we
> > miss a timeline change if we check it earlier and the function uses
> > the value just after. So we cannot change that behavior of the
> > function.  That is, neither of them doesn't seem to be the right fix.
> 
> Could you tell me what actual issue happens if read_local_xlog_page()
> resets
> ThisTimeLineID at the end? Some replication slot-related functions
> that use
> read_local_xlog_page() can be executed even during recovery. For
> example,
> you mean that, when timeline swithes during recovery, those functions
> behave incorrectly if ThisTimeLineID is reset?

The most significant point for me was I'm not fully convinced that we
can safely (or validly) remove the fucntion to maintain the variable
from read_local_xlog_page.

> * RecoveryInProgress() will update ThisTimeLineID when it first
> * notices recovery finishes, so we only have to maintain it for the
> * local process until recovery ends.

read_local_xlog_page is *designed* to maintain ThisTimeLineID.
Currently it doesn't seem utilized but I think it's sufficiently
reasonable that the function maintains ThisTimeLineID.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: OpenSSL 3.0.0 compatibility

[Michael Paquier]

On Wed, Mar 03, 2021 at 02:55:41PM +0100, Peter Eisentraut wrote:
> This thread is still in the commit fest, but I don't see any actual proposed
> patch still pending.  Most of the activity has moved into other threads.
> 
> Could you update the status of this CF entry, and perhaps also on the status
> of OpenSSL compatibility in general?

3.0.0 has released an alpha 12 on the 18th of February, so their stuff
is not quite close to GA yet.

I have not looked closely, but my guess is that it would take a couple
of extra months at least to see a release.  What if we just waited and
revisited this stuff during the next dev cycle, once there is at least
a beta, meaning mostly stable APIs?

Daniel, what do you think?
--
Michael


signature.asc
Description: PGP signature

Re: PITR promote bug: Checkpointer writes to older timeline

[Kyotaro Horiguchi]

At Thu, 04 Mar 2021 16:17:34 +0900 (JST), Kyotaro Horiguchi 
 wrote in 
> At Thu, 4 Mar 2021 11:18:42 +0900, Michael Paquier  
> wrote in 
> > I have not looked in details at the solutions proposed here, but could
> > it be possible to have a TAP test at least please?  Seeing the script
> > from the top of the thread, it should not be difficult to do so.  I
> > would put that in a file different than 009_twophase.pl, within
> > src/test/recovery/.

Isn't 004_timeline_switch.pl the place?

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: Shared memory size computation oversight?

[Michael Paquier]

On Thu, Mar 04, 2021 at 03:23:33PM +0800, Julien Rouhaud wrote:
> I was also considering adding new (add|mull)_*_size functions to avoid having
> too messy code.  I'm not terribly happy with xxx_shm_size(), as not all call 
> to
> those functions would require an alignment.  Maybe (add|mull)shmemalign_size?
> 
> But before modifying dozens of calls, should we really fix those or only
> increase a bit the "slop factor", or a mix of it?
> 
> For instance, I can see that for instance BackendStatusShmemSize() never had
> any padding consideration, while others do.
> 
> Maybe only fixing contribs, some macro like PredXactListDataSize that already
> do a MAXALIGN, SimpleLruShmemSize and hash_estimate_size() would be a short
> patch and should significantly improve the estimation.

The lack of complaints in this area looks to me like a sign that we
may not really need to backpatch something, so I would not be against
a precise chirurgy, with a separate set of {add,mul}_size() routines
that are used where adapted, so as it is easy to track down which size
estimations expect an extra padding.  I would be curious to hear more
thoughts from others here.

Saying that, calling a new routine something like add_shmem_align_size
makes it clear what's its purpose.
--
Michael


signature.asc
Description: PGP signature

Re: Shared memory size computation oversight?

[Georgios]

‐‐‐ Original Message ‐‐‐
On Thursday, March 4, 2021 9:21 AM, Michael Paquier  wrote:

> On Thu, Mar 04, 2021 at 03:23:33PM +0800, Julien Rouhaud wrote:
>
> > I was also considering adding new (add|mull)_*_size functions to avoid 
> > having
> > too messy code. I'm not terribly happy with xxx_shm_size(), as not all call 
> > to
> > those functions would require an alignment. Maybe (add|mull)shmemalign_size?
> > But before modifying dozens of calls, should we really fix those or only
> > increase a bit the "slop factor", or a mix of it?
> > For instance, I can see that for instance BackendStatusShmemSize() never had
> > any padding consideration, while others do.
> > Maybe only fixing contribs, some macro like PredXactListDataSize that 
> > already
> > do a MAXALIGN, SimpleLruShmemSize and hash_estimate_size() would be a short
> > patch and should significantly improve the estimation.
>
> The lack of complaints in this area looks to me like a sign that we
> may not really need to backpatch something, so I would not be against
> a precise chirurgy, with a separate set of {add,mul}_size() routines
> that are used where adapted, so as it is easy to track down which size
> estimations expect an extra padding. I would be curious to hear more
> thoughts from others here.
>
> Saying that, calling a new routine something like add_shmem_align_size
> makes it clear what's its purpose.

My limited opinion on the matter after spending some time yesterday through
the related code, is that with the current api it is easy to miss something
and underestimate or just be sloppy. If only from the readability point of
view, adding the proposed add_shmem_align_size will be beneficial.

I hold no opinion on backpatching.

//Georgios

>
> ---
>
> Michael

Re: Removing support for COPY FROM STDIN in protocol version 2

[Heikki Linnakangas]

On 04/03/2021 01:32, Tom Lane wrote:

Patched psql, trying to connect to a 7.3 server, reports this:

$ psql -h ...
psql: error: connection to server at "sss2" (192.168.1.3), port 5432 failed: 
FATAL:  unsupported frontend protocol

$

Conversely, 7.3 psql trying to connect to a patched server reports:

$ psql -h ...
psql: FATAL:  unsupported frontend protocol 2.0: server supports 3.0 to 3.0

$

I'm not sure where the extra newlines are coming from, and it seems
unlikely to be worth worrying over.  This behavior is good enough for me.


fe-connect.c appends a newline for any errors in pre-3.0 format:



/*
 * The postmaster typically won't end its message with a
 * newline, so add one to conform to libpq conventions.
 */
appendPQExpBufferChar(&conn->errorMessage, '\n');


That comment is wrong. The postmaster *does* end all its error messages 
with a newline. This changed in commit 9b4bfbdc2c in 7.2. Before that, 
postmaster had its own function, PacketSendError(), to send error 
messages, and it did not append a newline. Commit 9b4bfbdc2 changed 
postmaster to use elog(...) like everyone else, and elog(...) has always 
appended a newline. So I think this extra newline that libpq adds is 
needed if you try to connect to PostgreSQL 7.1 or earlier. I couldn't 
commpile a 7.1 server to verify this, though.


I changed that code in libpq to check if the message already has a 
newline, and only append one if it doesn't. This fixes the extra newline 
when connecting with new libpq to a 7.3 server (and in the fork failure 
message).



I concur that 0001 attached is committable.  I have not looked at
your 0002, though.


Removed the entry from nls.mk, and pushed 0001. Thanks!

- Heikki

Re: Parallel INSERT (INTO ... SELECT ...)

[Dilip Kumar]

On Thu, Mar 4, 2021 at 7:16 AM Greg Nancarrow  wrote:
>
> On Thu, Mar 4, 2021 at 1:07 AM Dilip Kumar  wrote:
> >
> > On Wed, Mar 3, 2021 at 5:50 PM Greg Nancarrow  wrote:
> > >
> > I agree that assert is only for debug build, but once we add and
> > assert that means we are sure that it should only be called for insert
> > and if it is called for anything else then it is a programming error
> > from the caller's side.  So after the assert, adding if check for the
> > same condition doesn't look like a good idea.  That means we think
> > that the code can hit assert in the debug mode so we need an extra
> > protection in the release mode.
> >
>
> The if-check isn't there for "extra protection".
> It's to help with future changes; inside that if-block is code only
> applicable to INSERT (and to UPDATE - sorry, before I said DELETE), as
> the code-comment indicates, whereas the rest of the function is
> generic to all command types. I don't see any problem with having this
> if-block here, to help in this way, when other command types are
> added.

If that is the case then this check should also be added along with
that future patches, I mean when we will allow UPDATE then it makes
sense of that check and that time will have to get rid of that assert
as well.  I mean complete function will be in sync.  But now this
check looks a bit odd.  I think that is my opinion but otherwise, I
don't have any strong objection to that check.

> > >
> > > > 2.
> > But the cost_modifytable is setting the number of rows to 0 in
> > ModifyTablePath whereas the cost_gather will multiply the rows from
> > the GatherPath.  I can not see the rows from GatherPath is ever set to
> > 0.
> >
>
> OK, I see the problem now.
> It works the way I described, but currently there's a problem with
> where it's getting the rows for the GatherPath, so there's a
> disconnect.
> When generating the GatherPaths, it's currently always taking the
> rel's (possibly estimated) row-count, rather than using the rows from
> the cheapest_partial_path (the subpath: ModifyTablePath). See
> generate_gather_paths().
> So when generate_useful_gather_paths() is called from the planner, for
> the added partial paths for Parallel INSERT, it should be passing
> "true" for the "override_rows" parameter, not "false".
>
> So I think that in the 0004 patch, the if-block:
>
> +   if (parallel_modify_partial_path_added)
> +   {
> +   final_rel->rows = current_rel->rows;/* ??? why
> hasn't this been
> +
>   * set above somewhere  */
> +   generate_useful_gather_paths(root, final_rel, false);
> +   }
> +
>
> can be reduced to:
>
> +   if (parallel_modify_partial_path_added)
> +   generate_useful_gather_paths(root, final_rel, true);
> +

Okay. I will check this part after you provide an updated version.  Thanks.

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

Re: Parallel INSERT (INTO ... SELECT ...)

[Dilip Kumar]

On Thu, Mar 4, 2021 at 9:03 AM Amit Kapila  wrote:
>
> I think for Update/Delete, we might not do parallel-safety checks by
> calling target_rel_max_parallel_hazard_recurse especially because
> partitions are handled differently for Updates and Deletes (see
> inheritance_planner()). I think what Dilip is telling doesn't sound
> unreasonable to me. So, even, if we want to extend it later by making
> some checks specific to Inserts/Updates, we can do it at that time.
> The comments you have at that place are sufficient to tell that in the
> future we can use those checks for Updates as well. They will need
> some adjustment if we remove that check but the intent is clear.

+1

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

Re: Extend more usecase for planning time partition pruning and init partition pruning.

[Amit Langote]

Hi Andy,

On Sun, Jan 24, 2021 at 7:34 PM Andy Fan  wrote:
>  I recently found a use case like this.  SELECT * FROM p, q WHERE p.partkey =
>  q.colx AND (q.colx = $1 OR q.colx = $2); Then we can't do either planning 
> time
>  partition prune or init partition prune.  Even though we have run-time
>  partition pruning work at last, it is too late in some cases since we have
>  to init all the plan nodes in advance.  In my case, there are 10+
>  partitioned relation in one query and the execution time is short, so the
>  init plan a lot of plan nodes cares a lot.
>
> The attached patches fix this issue. It just get the "p.partkey = q.colx"
> case in root->eq_classes or rel->joinlist (outer join), and then check if 
> there
> is some baserestrictinfo in another relation which can be used for partition
> pruning. To make the things easier, both partkey and colx must be Var
> expression in implementation.
>
> - v1-0001-Make-some-static-functions-as-extern-and-extend-C.patch
>
> Just some existing refactoring and extending ChangeVarNodes to be able
> to change var->attno.
>
> - v1-0002-Build-some-implied-pruning-quals-to-extend-the-us.patch

IIUC, your proposal is to transpose the "q.b in (1, 2)" in the
following query as "p.a in (1, 2)" and pass it down as a pruning qual
for p:

select * from p, q where p.a = q.b and q.b in (1, 2);

or "(q.b = 1 or q.b = 2)" in the following query as "(p.a = 1 or p.a = 2)":

select * from p, q where p.a = q.b and (q.b = 1 or q.b = 2);

While that transposition sounds *roughly* valid, I have some questions
about the approach:

* If the transposed quals are assumed valid to use for partition
pruning, could they also not be used by, say, the surviving
partitions' index scan paths?  So, perhaps, it doesn't seem right that
partprune.c builds the clauses on-the-fly for pruning and dump them
once done.

* On that last part, I wonder if partprune.c isn't the wrong place to
determine that "q.b in (1, 2)" and "p.a in (1, 2)" are in fact
equivalent.  That sort of thing is normally done in the phase of
planning when distribute_qual_to_rels() runs and any equivalences
found stored in PlannerInfo.eq_classes.  Have you investigated why the
process_ machinery doesn't support working with ScalarArrayOpExpr and
BoolExpr to begin with?

* Or maybe have you considered generalizing what
build_implied_pruning_quals() does so that other places like
indxpath.c can use the facility?

--
Amit Langote
EDB: http://www.enterprisedb.com

Force lookahead in COPY FROM parsing

[Heikki Linnakangas]

I posted this earlier at 
https://www.postgresql.org/message-id/9ec25819-0a8a-d51a-17dc-4150bb3cc...@iki.fi, 
and that led to removing FE/BE protocol version 2 support. That's been 
committed now, so here's COPY FROM patch again, rebased. To recap:


Previously COPY FROM could not look ahead in the COPY stream, because in 
the v2 protocol, it had to detect the end-of-copy marker correctly. With 
v2 protocol gone, that's no longer an issue, and we can simplify the 
parsing slightly. Simpler should also mean faster, but I haven't tried 
that measuring that.


- Heikki
>From 82680ff836fd5b43d1f83ed01e490d831ffa2b09 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas 
Date: Thu, 4 Mar 2021 11:10:39 +0200
Subject: [PATCH v2 1/1] Simplify COPY FROM parsing by forcing lookahead.

Now that we don't support the old-style COPY protocol anymore, we can
force four bytes of lookahead like was suggested in an existing comment,
and simplify the loop in CopyReadLineText().

Discussion: https://www.postgresql.org/message-id/9ec25819-0a8a-d51a-17dc-4150bb3cca3b%40iki.fi
---
 src/backend/commands/copyfromparse.c | 119 ---
 src/include/commands/copyfrom_internal.h |   3 +-
 2 files changed, 40 insertions(+), 82 deletions(-)

diff --git a/src/backend/commands/copyfromparse.c b/src/backend/commands/copyfromparse.c
index ce24a1528bd..c2efe7e0782 100644
--- a/src/backend/commands/copyfromparse.c
+++ b/src/backend/commands/copyfromparse.c
@@ -46,21 +46,6 @@
  * empty statements.  See http://www.cit.gu.edu.au/~anthony/info/C/C.macros.
  */
 
-/*
- * This keeps the character read at the top of the loop in the buffer
- * even if there is more than one read-ahead.
- */
-#define IF_NEED_REFILL_AND_NOT_EOF_CONTINUE(extralen) \
-if (1) \
-{ \
-	if (raw_buf_ptr + (extralen) >= copy_buf_len && !hit_eof) \
-	{ \
-		raw_buf_ptr = prev_raw_ptr; /* undo fetch */ \
-		need_data = true; \
-		continue; \
-	} \
-} else ((void) 0)
-
 /* This consumes the remainder of the buffer and breaks */
 #define IF_NEED_REFILL_AND_EOF_BREAK(extralen) \
 if (1) \
@@ -118,7 +103,7 @@ static int	CopyGetData(CopyFromState cstate, void *databuf,
 		int minread, int maxread);
 static inline bool CopyGetInt32(CopyFromState cstate, int32 *val);
 static inline bool CopyGetInt16(CopyFromState cstate, int16 *val);
-static bool CopyLoadRawBuf(CopyFromState cstate);
+static bool CopyLoadRawBuf(CopyFromState cstate, int minread);
 static int	CopyReadBinaryData(CopyFromState cstate, char *dest, int nbytes);
 
 void
@@ -209,7 +194,7 @@ CopyGetData(CopyFromState cstate, void *databuf, int minread, int maxread)
 ereport(ERROR,
 		(errcode_for_file_access(),
 		 errmsg("could not read from COPY file: %m")));
-			if (bytesread == 0)
+			if (bytesread < maxread)
 cstate->reached_eof = true;
 			break;
 		case COPY_FRONTEND:
@@ -278,6 +263,8 @@ CopyGetData(CopyFromState cstate, void *databuf, int minread, int maxread)
 			break;
 		case COPY_CALLBACK:
 			bytesread = cstate->data_source_cb(databuf, minread, maxread);
+			if (bytesread < minread)
+cstate->reached_eof = true;
 			break;
 	}
 
@@ -329,14 +316,13 @@ CopyGetInt16(CopyFromState cstate, int16 *val)
 /*
  * CopyLoadRawBuf loads some more data into raw_buf
  *
- * Returns true if able to obtain at least one more byte, else false.
+ * Returns true if able to obtain at least 'minread' bytes, else false.
  *
  * If RAW_BUF_BYTES(cstate) > 0, the unprocessed bytes are moved to the start
- * of the buffer and then we load more data after that.  This case occurs only
- * when a multibyte character crosses a bufferload boundary.
+ * of the buffer and then we load more data after that.
  */
 static bool
-CopyLoadRawBuf(CopyFromState cstate)
+CopyLoadRawBuf(CopyFromState cstate, int minread)
 {
 	int			nbytes = RAW_BUF_BYTES(cstate);
 	int			inbytes;
@@ -347,14 +333,15 @@ CopyLoadRawBuf(CopyFromState cstate)
 nbytes);
 
 	inbytes = CopyGetData(cstate, cstate->raw_buf + nbytes,
-		  1, RAW_BUF_SIZE - nbytes);
+		  minread, RAW_BUF_SIZE - nbytes);
 	nbytes += inbytes;
 	cstate->raw_buf[nbytes] = '\0';
 	cstate->raw_buf_index = 0;
 	cstate->raw_buf_len = nbytes;
 	cstate->bytes_processed += inbytes;
 	pgstat_progress_update_param(PROGRESS_COPY_BYTES_PROCESSED, cstate->bytes_processed);
-	return (inbytes > 0);
+
+	return (inbytes >= minread);
 }
 
 /*
@@ -389,7 +376,7 @@ CopyReadBinaryData(CopyFromState cstate, char *dest, int nbytes)
 			/* Load more data if buffer is empty. */
 			if (RAW_BUF_BYTES(cstate) == 0)
 			{
-if (!CopyLoadRawBuf(cstate))
+if (!CopyLoadRawBuf(cstate, 1))
 	break;		/* EOF */
 			}
 
@@ -678,7 +665,7 @@ CopyReadLine(CopyFromState cstate)
 			do
 			{
 cstate->raw_buf_index = cstate->raw_buf_len;
-			} while (CopyLoadRawBuf(cstate));
+			} while (CopyLoadRawBuf(cstate, 1));
 		}
 	}
 	else
@@ -747,7 +734,6 @@ CopyReadLineText(CopyFromState cstate)
 	char	   *copy_raw_buf;
 	int			raw_buf_ptr;
 	int			copy_buf_len;
-	bool		need_data = false

Re: [PATCH] psql : Improve code for help option

[Fujii Masao]

On 2021/03/03 17:05, Nitin Jadhav wrote:

Hi,

I have reviewed the patch and it looks good to me.


Thanks! Pushed.

Regards,

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

Re: Get memory contexts of an arbitrary backend process

[torikoshia]

On 2021-01-14 19:11, torikoshia wrote:

Since pg_get_target_backend_memory_contexts() waits to dump memory and
it could lead dead lock as below.

  - session1
  BEGIN; TRUNCATE t;

  - session2
  BEGIN; TRUNCATE t; -- wait

  - session1
  SELECT * FROM pg_get_target_backend_memory_contexts(); --wait


Thanks for notifying me, Fujii-san.


Attached v8 patch that prohibited calling the function inside 
transactions.


Regrettably, this modification could not cope with the advisory lock and
I haven't come up with a good way to deal with it.

It seems to me that the architecture of the requestor waiting for the
dumper leads to this problem and complicates things.


Considering the discussion printing backtrace discussion[1], it seems
reasonable that the requestor just sends a signal and dumper dumps to
the log file.

Since I found a past discussion that was doing exactly what I thought
reasonable[2], I'm going to continue that discussion if there are no
objections.


Any thought?


[1] 
https://www.postgresql.org/message-id/flat/CALDaNm3ZzmFS-=r7oduzj7y7bgqv+n06kqyft6c3xzdoknk...@mail.gmail.com
[2] 
https://www.postgresql.org/message-id/flat/20171212044330.3nclev2sfrab36tf%40alap3.anarazel.de#6f28be9839c74779ed6aaa75616124f5



Regards,

--
Atsushi Torikoshi
NTT DATA CORPORATION

Re: pgbench: option delaying queries till connections establishment?

[Thomas Munro]

On Wed, Mar 3, 2021 at 6:23 PM Thomas Munro  wrote:
> On Sun, Jan 31, 2021 at 1:18 AM Fabien COELHO  wrote:
> > I think it would be much more consistent to move all the thread complement
> > stuff there directly: Currently (v8) the windows implementation is in
> > pgbench and the MacOS implementation in port, which is quite messy.
>
> Hmm.  Well this is totally subjective, but here's how I see this after
> thinking about it a bit more:  macOS does actually have POSIX threads,
> except for this tiny missing piece, so it's OK to write a toy
> implementation that is reasonably conformant, and put it in there
> using the usual AC_REPLACE_FUNCS machinery.  It will go away when
> Apple eventually adds a real one.  Windows does not, and here we're
> writing a very partial toy implementation that is far from conformant.
> I think that's OK for pgbench's purposes, for now, but I'd prefer to
> keep it inside pgbench.c.  I think at some point in the (hopefully not
> too distant) future, we'll start working on thread support for the
> backend, and then I think we'll probably come up with our own
> abstraction over Windows and POSIX threads, rather than trying to use
> POSIX API wrappers from Windows, so I don't really want this stuff in
> the port library.  Does this make some kind of sense?

Here is an attempt to move things in that direction.  It compiles
tests OK on Unix including macOS with and without
--disable-thread-safety, and it compiles on Windows (via CI) but I
don't yet know if it works there.

v10-0001-Add-missing-pthread_barrier_t.patch

Same as v8.  Adds the missing pthread_barrier_t support for macOS
only.  Based on the traditional configure symbol probe for now.  It's
possible that we'll later decide to use declarations to be more
future-proof against Apple's API versioning strategy, but I don't have
one of those weird cross-version compiler setups to investigate that
(see complaints from James Hilliard about the way we deal with
pwrite()).

v10-0002-pgbench-Refactor-the-way-we-do-thread-portabilit.patch

New.  Abandons the concept of doing a fake pthread API on Windows in
pgbench.c, in favour of a couple of tiny local macros that abstract
over POSIX, Windows and threadless builds.  This results in less code,
and also fixes some minor problems I spotted in pre-existing code:
it's not OK to use (pthread_t) 0 as a special value, or to compare
pthread_t values with ==, or to assume that pthread APIs set errno.

v10-0003-pgbench-Improve-time-measurement-code.patch

Your original A patch, rebased over the above.  I haven't reviewed
this one.  It lacks a commit message.

v10-0004-pgbench-Synchronize-client-threads.patch

Adds in the barriers.
From 1459914c729e1157a932254bf7483f1ef7ac6408 Mon Sep 17 00:00:00 2001
From: Thomas Munro 
Date: Sat, 2 Jan 2021 15:05:06 +1300
Subject: [PATCH v10 1/5] Add missing pthread_barrier_t.

Supply a simple implementation of the missing pthread_barrier_t type and
functions, for macOS.

Discussion: https://postgr.es/m/20200227180100.zyvjwzcpiokfsqm2%40alap3.anarazel.de
---
 configure   | 69 +
 configure.ac|  2 +
 src/include/pg_config.h.in  |  3 ++
 src/include/port/pg_pthread.h   | 41 
 src/port/pthread_barrier_wait.c | 66 +++
 src/tools/msvc/Solution.pm  |  1 +
 6 files changed, 182 insertions(+)
 create mode 100644 src/include/port/pg_pthread.h
 create mode 100644 src/port/pthread_barrier_wait.c

diff --git a/configure b/configure
index ce9ea36999..fad817bb38 100755
--- a/configure
+++ b/configure
@@ -11635,6 +11635,62 @@ if test "$ac_res" != no; then :
 
 fi
 
+{ $as_echo "$as_me:${as_lineno-$LINENO}: checking for library containing pthread_barrier_wait" >&5
+$as_echo_n "checking for library containing pthread_barrier_wait... " >&6; }
+if ${ac_cv_search_pthread_barrier_wait+:} false; then :
+  $as_echo_n "(cached) " >&6
+else
+  ac_func_search_save_LIBS=$LIBS
+cat confdefs.h - <<_ACEOF >conftest.$ac_ext
+/* end confdefs.h.  */
+
+/* Override any GCC internal prototype to avoid an error.
+   Use char because int might match the return type of a GCC
+   builtin and then its argument prototype would still apply.  */
+#ifdef __cplusplus
+extern "C"
+#endif
+char pthread_barrier_wait ();
+int
+main ()
+{
+return pthread_barrier_wait ();
+  ;
+  return 0;
+}
+_ACEOF
+for ac_lib in '' pthread; do
+  if test -z "$ac_lib"; then
+ac_res="none required"
+  else
+ac_res=-l$ac_lib
+LIBS="-l$ac_lib  $ac_func_search_save_LIBS"
+  fi
+  if ac_fn_c_try_link "$LINENO"; then :
+  ac_cv_search_pthread_barrier_wait=$ac_res
+fi
+rm -f core conftest.err conftest.$ac_objext \
+conftest$ac_exeext
+  if ${ac_cv_search_pthread_barrier_wait+:} false; then :
+  break
+fi
+done
+if ${ac_cv_search_pthread_barrier_wait+:} false; then :
+
+else
+  ac_cv_search_pthread_barrier_wait=no
+fi
+rm conftest.$ac_ext
+LIBS=$ac_func_search_save_LIBS
+fi
+{ $as_echo "$as_me

Re: proposal - psql - use pager for \watch command

[Pavel Stehule]

Hi

čt 4. 3. 2021 v 7:37 odesílatel Pavel Stehule 
napsal:

> Hi
>
> Here is a little bit updated patch - detection of end of any child process
> cannot be used on WIN32. I am not an expert on this platform, but from what
> I read about it, there is no easy solution. The problem is in _popen
> function. We lost the handle of the created process, and it is not possible
> to find it. Writing a new implementation of _popen function looks like a
> big overkill to me. We can disable this functionality there completely (on
> win32) or we can accept the waiting time after pager has ended until we
> detect pipe error. I hope so this is acceptable, in this moment, because a)
> there are not pspg for windows (and there was only few requests for porting
> there in last 4 years), b) usage of psql on mswin platform is not too wide,
> c) in near future, there will be an possibility to use Unix psql on this
> platform.
>
>
second version - after some thinking, I think the pager for \watch command
should be controlled by option "pager" too.  When the pager is disabled on
psql level, then the pager will not be used for \watch too.

Regards

Pavel


> Regards
>
> Pavel
>
>
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index 13c1edfa4d..9a88c3f8ef 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -2993,6 +2993,11 @@ lo_import 152801
   (such as more) is used.
   
 
+  
+  When the ennvironment variable PSQL_WATCH_PAGER is set, then
+  the output of repeated execution of query is piped to the specified program.
+  
+
   
   When the pager option is off, the pager
   program is not used. When the pager option is
@@ -3004,6 +3009,13 @@ lo_import 152801
   without a value
   toggles pager use on and off.
   
+
+  
+  When an command \watch is executed, and environment
+  variable PSQL_WATCH_PAGER is defined, but the value of
+  the option pager is off, then the
+  pager is not used.
+  
   
   
 
@@ -4652,6 +4664,21 @@ PSQL_EDITOR_LINENUMBER_ARG='--line '
 

 
+   
+PSQL_WATCH_PAGER
+
+
+ 
+  When an statement is evaluated repeatedly because \watch
+  was used, then an pager is not used. This behaviour can be changed by
+  setting PSQL_WATCH_PAGER to pager with necessary options.
+  Currently only pspg pager (version 3.0+) supports this
+  functionality (with an option --stream).
+ 
+
+
+   
+

 PSQLRC
 
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index c98e3d31d0..290efed3d3 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -12,6 +12,7 @@
 #include 
 #ifndef WIN32
 #include 			/* for stat() */
+#include 			/* for waitpid() */
 #include /* open() flags */
 #include /* for geteuid(), getpid(), stat() */
 #else
@@ -4766,6 +4767,8 @@ do_watch(PQExpBuffer query_buf, double sleep)
 	const char *strftime_fmt;
 	const char *user_title;
 	char	   *title;
+	const char *pagerprog = NULL;
+	FILE	   *pagerpipe = NULL;
 	int			title_len;
 	int			res = 0;
 
@@ -4775,6 +4778,25 @@ do_watch(PQExpBuffer query_buf, double sleep)
 		return false;
 	}
 
+	/*
+	 * For usual queries, the pager can be used always, or
+	 * newer, or when then content is larger than screen. In this case,
+	 * the decision based on then content size has not sense, because the
+	 * content can be different any time, but pager (in this case) is
+	 * used longer time. So we use pager if the variable PSQL_WATCH_PAGER
+	 * is defined and if pager is not disabled.
+	 */
+	pagerprog = getenv("PSQL_WATCH_PAGER");
+	if (pagerprog && myopt.topt.pager)
+	{
+		disable_sigpipe_trap();
+		pagerpipe = popen(pagerprog, "w");
+
+		if (!pagerpipe)
+			/*silently proceed without pager */
+			restore_sigpipe_trap();
+	}
+
 	/*
 	 * Choose format for timestamps.  We might eventually make this a \pset
 	 * option.  In the meantime, using a variable for the format suppresses
@@ -4783,10 +4805,12 @@ do_watch(PQExpBuffer query_buf, double sleep)
 	strftime_fmt = "%c";
 
 	/*
-	 * Set up rendering options, in particular, disable the pager, because
-	 * nobody wants to be prompted while watching the output of 'watch'.
+	 * Set up rendering options, in particular, disable the pager, when
+	 * there an pipe to pager is not available.
 	 */
-	myopt.topt.pager = 0;
+	if (!pagerpipe)
+		myopt.topt.pager = 0;
+
 
 	/*
 	 * If there's a title in the user configuration, make sure we have room
@@ -4820,7 +4844,7 @@ do_watch(PQExpBuffer query_buf, double sleep)
 		myopt.title = title;
 
 		/* Run the query and print out the results */
-		res = PSQLexecWatch(query_buf->data, &myopt);
+		res = PSQLexecWatch(query_buf->data, &myopt, pagerpipe);
 
 		/*
 		 * PSQLexecWatch handles the case where we can no longer repeat the
@@ -4829,6 +4853,9 @@ do_watch(PQExpBuffer query_buf, double sleep)
 		

Re: [HACKERS] Custom compression methods

[Dilip Kumar]

On Thu, Mar 4, 2021 at 2:49 AM Robert Haas  wrote:
>
> Hi,
>
> Does this patch need to do something about ExtractReplicaIdentity()?
> If there are compressed fields in the tuple being built, can we rely
> on the decompression engine being available at the time we need to do
> something with the tuple?

We log the replica identity tuple in the WAL so that later walsender
can stream this to the subscriber, and before sending to the
subscriber anyway we have detoast all the data.  Said that I think the
problem you are worried about is not only with 'replica identity
tuple' but it is with any tuple.  I mean we copy the compressed field
as it is in WAL and suppose we copy some fields which are compressed
with lz4 and then we restart the server with another binary that is
compiled without lz4.  Now, the problem is the walsender can not
decompress those data.

> More generally, I think it would be good to divide up 0001 into at
> least 3 parts:
>
> - The first part would invent HeapTupleGetRawDatum() and
> HeapTupleHeaderGetRawDatum() and use them in place of the existing
> functions everywhere that it's safe to do so. The current patch just
> switches to using PointerGetDatum() but I think we should instead add
> something like static inline Datum
> HeapTupleHeaderGetRawDatum(HeapTupleHeader tuple) {
> Assert(!HeapTupleHeaderHasExternal(tup)); return
> PointerGetDatum(tuple); } This provides some type safety while being
> just as fast as a direct use of PointerGetDatum() in optimized code. I
> think that the Assert will have to be ripped out if we proceed with
> the other patches, but if we can have it at this stage, so much the
> better. I think this patch should also invent
> PG_RETURN_HEAPTUPLEHEADER_RAW and likewise use that where appropriate
> - including, I think, in place of cases that are now using
> PG_RETURN_DATUM(HeapTupleGetDatum(...)). All of these changes make
> sense from an efficiency standpoint apart from any possible
> definitional changes.
>
> - The second part would optimize code that the first part cannot
> safely convert to use the "raw" versions. For example, the changes to
> ExecEvalRow() can go in this part. You can view this part as getting
> rid of calls to HeapTupleGetDatum(), HeapTupleHeaderGetDatum(), and/or
> PG_RETURN_HEAPTUPLE() that couldn't be changed categorically, but can
> be changed if we make some other code changes. These changes, too, can
> potentially be justified on performance grounds independently of
> anything else.
>
> - Then we could maybe have some more patches that make other kinds of
> preparatory changes. I'm not too sure exactly what should go in here,
> or whether it should be 1 patch or several or maybe 0. But if there's
> preparatory stuff that's potentially separately committable and not
> the same as the stuff above, then it should go into patches here.
>
> - The last patch would actually change the rule for composite datums.
>
> One advantage of this is that if we have to revert the last patch for
> some reason we are not ripping the entire thing, churning the code
> base and widely-used APIs for everyone. Another advantage is that
> getting those first two patches committed or even just applied locally
> on a branch would, at least IMHO, make it a lot simpler to see what
> potential problem spots remain - and by "problem" I mean mostly from a
> performance point of view.

Okay, I will work on this.

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

Re: Improvements and additions to COPY progress reporting

[Michael Paquier]

On Thu, Mar 04, 2021 at 12:16:17PM +0530, Bharath Rupireddy wrote:
> IMO, the phrasing proposed by Justin upthread looks good. It's like this:
> 
> > +Each backend running ANALYZE will report its 
> > progress in
> > +the pg_stat_progress_analyze view. See

No objections to just go with that.  As a new patch set is needed, I
am switching the CF entry to "Waiting on Author".
--
Michael


signature.asc
Description: PGP signature

Re: Allow matching whole DN from a client certificate

[Ibrar Ahmed]

On Wed, Mar 3, 2021 at 3:03 AM Jacob Champion  wrote:

> On Fri, 2021-02-26 at 15:40 -0500, Andrew Dunstan wrote:
> > I think the thing that's principally outstanding w.r.t. this patch is
> > what format we should use to extract the DN.
>
> That and the warning label for sharp edges.
>
> > Should we use RFC2253,
> > which reverses the field order, as has been suggested upthread and is in
> > the latest patch? I'm slightly worried that it might be a POLA
> > violation.
>
> All I can provide is the hindsight from httpd. [1] is the thread that
> gave rise to its LegacyDNStringFormat.
>
> Since RFC 2253 isn't a canonical encoding scheme, and we've already
> established that different TLS implementations do things slightly
> differently even when providing RFC-compliant output, maybe it doesn't
> matter in the end: to get true compatibility, we need to implement a DN
> matching scheme rather than checking string equality. But using RFC2253
> for version 1 of the feature at least means that the *simplest* cases
> are the same across backends, since I doubt the NSS implementation is
> going to try to recreate OpenSSL's custom format.
>
> --Jacob
>
> [1]
> https://lists.apache.org/thread.html/2055b56985c69e7a6977151bf9817a0f982a4ad3b78a6a1984977fd0%401289507617%40%3Cusers.httpd.apache.org%3E
>


This patch set no longer applies
http://cfbot.cputube.org/patch_32_2835.log

Can we get a rebase?

I marked the patch "Waiting on Author".



-- 
Ibrar Ahmed

Re: Disallow SSL compression?

[Michael Paquier]

On Wed, Mar 03, 2021 at 03:14:01PM +0100, Peter Eisentraut wrote:
> Per your other thread, you should also remove the environment variable.
> 
> In postgres_fdw, I think commenting it out is not the right change.  The
> other commented out values are still valid settings but are omitted from the
> test for other reasons.  It's not entirely all clear, but we don't have to
> keep obsolete stuff in there forever.

Agreed on both points.  Also, it seems a bit weird to keep around
pg_stat_ssl.compression while we know that it will always be false.
Wouldn't it be better to get rid of that in PgBackendSSLStatus and
pg_stat_ssl then?
--
Michael


signature.asc
Description: PGP signature

Re: [POC] Fast COPY FROM command for the table with foreign partitions

[Ibrar Ahmed]

On Thu, Mar 4, 2021 at 12:40 PM tsunakawa.ta...@fujitsu.com <
tsunakawa.ta...@fujitsu.com> wrote:

> From: Zhihong Yu 
> > This feature enables bulk COPY into foreign table in the case of
> > multi inserts is possible
> >
> > 'is possible' -> 'if possible'
> >
> > FDWAPI was extended by next routines:
> >
> > next routines -> the following routines
>
> Thank you, fixed slightly differently.  (I feel the need for pgEnglish
> again.)
>
>
> > +   if ((!OK && PQresultStatus(res) != PGRES_FATAL_ERROR) ||
> >
> > Is PGRES_FATAL_ERROR handled somewhere else ? I don't seem to find that
> in the patch.
>
> Good catch.  ok doesn't need to be consulted here, because failure during
> row transmission causes PQputCopyEnd() to receive non-NULL for its second
> argument, which in turn makes PQgetResult() return non-COMMAND_OK.
>
>
> Regards
> Takayuki Tsunakawa
>
>
This patch set no longer applies
http://cfbot.cputube.org/patch_32_2601.log

Can we get a rebase?

I am marking the patch "Waiting on Author"

-- 
Ibrar Ahmed

Re: Parallel INSERT (INTO ... SELECT ...)

[Amit Kapila]

On Fri, Feb 26, 2021 at 10:37 AM Amit Langote  wrote:
>
> I realized that there is a race condition that will allow a concurrent
> backend to invalidate a parallel plan (for insert into a partitioned
> table) at a point in time when it's too late for plancache.c to detect
> it.  It has to do with how plancache.c locks the relations affected by
> a cached query and its cached plan.  Specifically,
> AcquireExecutorLocks(), which locks the relations that need to be
> locked before the plan could be considered safe to execute, does not
> notice the partitions that would have been checked for parallel safety
> when the plan was made.  Given that AcquireExecutorLocks() only loops
> over PlannedStmt.rtable and locks exactly the relations found there,
> partitions are not locked.  This means that a concurrent backend can,
> for example, add an unsafe trigger to a partition before a parallel
> worker inserts into it only to fail when it does.
>
> Steps to reproduce (tried with v19 set of patches):
>
> drop table if exists rp, foo;
> create table rp (a int) partition by range (a);
> create table rp1 partition of rp for values from (minvalue) to (0);
> create table rp2 partition of rp for values from (0) to (maxvalue);
> create table foo (a) as select generate_series(1, 100);
> set plan_cache_mode to force_generic_plan;
> set enable_parallel_dml to on;
> deallocate all;
> prepare q as insert into rp select * from foo where a%2 = 0;
> explain analyze execute q;
>
> At this point, attach a debugger to this backend and set a breakpoint
> in AcquireExecutorLocks() and execute q again:
>
> -- will execute the cached plan
> explain analyze execute q;
>
> Breakpoint will be hit.  Continue till return from the function and
> stop. Start another backend and execute this:
>
> -- will go through, because no lock still taken on the partition
> create or replace function make_table () returns trigger language
> plpgsql as $$ begin create table bar(); return null; end; $$ parallel
> unsafe;
> create trigger ai_rp2 after insert on rp2 for each row execute
> function make_table();
>
> Back to the debugger, hit continue to let the plan execute.  You
> should see this error:
>
> ERROR:  cannot start commands during a parallel operation
> CONTEXT:  SQL statement "create table bar()"
> PL/pgSQL function make_table() line 1 at SQL statement parallel worker
>
> The attached patch fixes this,
>

One thing I am a bit worried about this fix is that after this for
parallel-mode, we will maintain partitionOids at two places, one in
plannedstmt->relationOids and the other in plannedstmt->partitionOids.
I guess you have to do that because, in AcquireExecutorLocks, we can't
find which relationOids are corresponding to partitionOids, am I
right? If so, why not just maintain them in plannedstmt->partitionOids
and then make PlanCacheRelCallback consider it? Also, in
standard_planner, we should add these partitionOids only for
parallel-mode.

Thoughts?

-- 
With Regards,
Amit Kapila.

Re: Extending range type operators to cope with elements

[Ibrar Ahmed]

On Sun, Feb 28, 2021 at 1:36 AM Justin Pryzby  wrote:

> On Fri, Oct 30, 2020 at 11:08:19PM +0100, Tomas Vondra wrote:
> > Hi,
> >
> > +  
> > +   
> > +anyelement >>
> anyrange
> > +boolean
> > +   
> > +   
> > +Is the element strictly right of the element?
> > +   
>
> should say "of the range" ?
>
> > +++ b/src/backend/utils/adt/rangetypes.c
>
> > + /* An empty range is neither left nor right any other range */
> > + /* An empty range is neither left nor right any element */
> > + /* An empty range is neither left nor right any other range */
> > + /* An empty range is neither left nor right any element */
> > + /* An empty range is neither left nor right any element */
> > + /* An empty range is neither left nor right any element */
>
> I these comments should all say ".. left nor right OF any ..."
>
> --
> Justin
>
>
>
This patch set no longer applies.

http://cfbot.cputube.org/patch_32_2747.log

Can we get a rebase?

I am marking the patch "Waiting on Author"

--
Ibrar Ahmed

Re: Hybrid Hash/Nested Loop joins and caching results from subplans

[Ibrar Ahmed]

On Tue, Feb 23, 2021 at 10:44 AM Tom Lane  wrote:

> Andres Freund  writes:
> > We could add a wrapper node around "planner expressions" that stores
> > metadata about them during planning, without those properties leaking
> > over expressions used at other times. E.g. having
> > preprocess_expression() return a PlannerExpr that that points to the
> > expression as preprocess_expression returns it today. That'd make it
> > easy to cache information like volatility. But it also seems
> > prohibitively invasive :(.
>
> I doubt it's that bad.  We could cache such info in RestrictInfo
> for quals, or PathTarget for tlists, without much new notational
> overhead.  That doesn't cover everything the planner deals with
> of course, but it would cover enough that you'd be chasing pretty
> small returns to worry about more.
>
> regards, tom lane
>
>
>
This patch set no longer applies
http://cfbot.cputube.org/patch_32_2569.log

Can we get a rebase?

I am marking the patch "Waiting on Author"



-- 
Ibrar Ahmed

Re: [HACKERS] PATCH: Batch/pipelining support for libpq

[Ibrar Ahmed]

On Wed, Jul 15, 2020 at 12:18 AM Andres Freund  wrote:

> Hi,
>
> On 2020-07-10 19:01:49 -0400, Alvaro Herrera wrote:
> > Totally unasked for, here's a rebase of this patch series.  I didn't do
> > anything other than rebasing to current master, solving a couple of very
> > trivial conflicts, fixing some whitespace complaints by git apply, and
> > running tests to verify everthing works.
> >
> > I don't foresee working on this at all, so if anyone is interested in
> > seeing this feature in, I encourage them to read and address
> > Horiguchi-san's feedback.
>
> Nor am I planning to do so, but I do think its a pretty important
> improvement.
>
>
>
> > +/*
> > + * PQrecyclePipelinedCommand
> > + *   Push a command queue entry onto the freelist. It must be a
> dangling entry
> > + *   with null next pointer and not referenced by any other entry's
> next pointer.
> > + */
>
> Dangling sounds a bit like it's already freed.
>
>
>
> > +/*
> > + * PQbatchSendQueue
> > + *   End a batch submission by sending a protocol sync. The connection
> will
> > + *   remain in batch mode and unavailable for new synchronous command
> execution
> > + *   functions until all results from the batch are processed by the
> client.
>
> I feel like the reference to the protocol sync is a bit too low level
> for an external API. It should first document what the function does
> from a user's POV.
>
> I think it'd also be good to document whether / whether not queries can
> already have been sent before PQbatchSendQueue is called or not.
>
>
> > +/*
> > + * PQbatchProcessQueue
> > + *In batch mode, start processing the next query in the queue.
> > + *
> > + * Returns 1 if the next query was popped from the queue and can
> > + * be processed by PQconsumeInput, PQgetResult, etc.
> > + *
> > + * Returns 0 if the current query isn't done yet, the connection
> > + * is not in a batch, or there are no more queries to process.
> > + */
> > +int
> > +PQbatchProcessQueue(PGconn *conn)
> > +{
> > + PGcommandQueueEntry *next_query;
> > +
> > + if (!conn)
> > + return 0;
> > +
> > + if (conn->batch_status == PQBATCH_MODE_OFF)
> > + return 0;
> > +
> > + switch (conn->asyncStatus)
> > + {
> > + case PGASYNC_COPY_IN:
> > + case PGASYNC_COPY_OUT:
> > + case PGASYNC_COPY_BOTH:
> > + printfPQExpBuffer(&conn->errorMessage,
> > +libpq_gettext_noop("internal error,
> COPY in batch mode"));
> > + break;
>
> Shouldn't there be a return 0 here?
>
>
>
> > + if (conn->batch_status == PQBATCH_MODE_ABORTED && conn->queryclass
> != PGQUERY_SYNC)
> > + {
> > + /*
> > +  * In an aborted batch we don't get anything from the
> server for each
> > +  * result; we're just discarding input until we get to the
> next sync
> > +  * from the server. The client needs to know its queries
> got aborted
> > +  * so we create a fake PGresult to return immediately from
> > +  * PQgetResult.
> > +  */
> > + conn->result = PQmakeEmptyPGresult(conn,
> > +
> PGRES_BATCH_ABORTED);
> > + if (!conn->result)
> > + {
> > + printfPQExpBuffer(&conn->errorMessage,
> > +
>  libpq_gettext("out of memory"));
> > + pqSaveErrorResult(conn);
> > + return 0;
>
> Is there any way an application can recover at this point? ISTM we'd be
> stuck in the previous asyncStatus, no?
>
>
> > +/* pqBatchFlush
> > + * In batch mode, data will be flushed only when the out buffer reaches
> the threshold value.
> > + * In non-batch mode, data will be flushed all the time.
> > + */
> > +static int
> > +pqBatchFlush(PGconn *conn)
> > +{
> > + if ((conn->batch_status == PQBATCH_MODE_OFF)||(conn->outCount >=
> OUTBUFFER_THRESHOLD))
> > + return(pqFlush(conn));
> > + return 0; /* Just to keep compiler quiet */
> > +}
>
> unnecessarily long line.
>
>
> > +/*
> > + * Connection's outbuffer threshold is set to 64k as it is safe
> > + * in Windows as per comments in pqSendSome() API.
> > + */
> > +#define OUTBUFFER_THRESHOLD  65536
>
> I don't think the comment explains much. It's fine to send more than 64k
> with pqSendSome(), they'll just be send with separate pgsecure_write()
> invocations. And only on windows.
>
> It clearly makes sense to start sending out data at a certain
> granularity to avoid needing unnecessary amounts of memory, and to make
> more efficient use of latency / serer side compute.
>
> It's not implausible that 64k is the right amount for that, I just don't
> think the explanation above is good.
>
> > diff --git a/src/test/modules/test_libpq/testlibpqbatch.c
> b/src/test/modules/test_libpq/testlibpqbatch.c
> > new file mode 100644
> > index 00..4d6ba266e5
> > --- /dev/null
> > +++ b/src/test/modules/test_libpq/testlibpqbatch.c

Re: About to add WAL write/fsync statistics to pg_stat_wal view

[Ibrar Ahmed]

On Thu, Mar 4, 2021 at 12:14 PM Masahiro Ikeda 
wrote:

> On 2021-03-03 20:27, Masahiro Ikeda wrote:
> > On 2021-03-03 16:30, Fujii Masao wrote:
> >> On 2021/03/03 14:33, Masahiro Ikeda wrote:
> >>> On 2021-02-24 16:14, Fujii Masao wrote:
>  On 2021/02/15 11:59, Masahiro Ikeda wrote:
> > On 2021-02-10 00:51, David G. Johnston wrote:
> >> On Thu, Feb 4, 2021 at 4:45 PM Masahiro Ikeda
> >>  wrote:
> >>
> >>> I pgindented the patches.
> >>
> >> ... XLogWrite, which is invoked during an
> >> XLogFlush request (see ...).  This is also
> >> incremented by the WAL receiver during replication.
> >>
> >> ("which normally called" should be "which is normally called" or
> >> "which normally is called" if you want to keep true to the
> >> original)
> >> You missed the adding the space before an opening parenthesis here
> >> and
> >> elsewhere (probably copy-paste)
> >>
> >> is ether -> is either
> >> "This parameter is off by default as it will repeatedly query the
> >> operating system..."
> >> ", because" -> "as"
> >
> > Thanks, I fixed them.
> >
> >> wal_write_time and the sync items also need the note: "This is
> >> also
> >> incremented by the WAL receiver during replication."
> >
> > I skipped changing it since I separated the stats for the WAL
> > receiver
> > in pg_stat_wal_receiver.
> >
> >> "The number of times it happened..." -> " (the tally of this event
> >> is
> >> reported in wal_buffers_full in) This is undesirable because
> >> ..."
> >
> > Thanks, I fixed it.
> >
> >> I notice that the patch for WAL receiver doesn't require
> >> explicitly
> >> computing the sync statistics but does require computing the write
> >> statistics.  This is because of the presence of issue_xlog_fsync
> >> but
> >> absence of an equivalent pg_xlog_pwrite.  Additionally, I observe
> >> that
> >> the XLogWrite code path calls pgstat_report_wait_*() while the WAL
> >> receiver path does not.  It seems technically straight-forward to
> >> refactor here to avoid the almost-duplicated logic in the two
> >> places,
> >> though I suspect there may be a trade-off for not adding another
> >> function call to the stack given the importance of WAL processing
> >> (though that seems marginalized compared to the cost of actually
> >> writing the WAL).  Or, as Fujii noted, go the other way and don't
> >> have
> >> any shared code between the two but instead implement the WAL
> >> receiver
> >> one to use pg_stat_wal_receiver instead.  In either case, this
> >> half-and-half implementation seems undesirable.
> >
> > OK, as Fujii-san mentioned, I separated the WAL receiver stats.
> > (v10-0002-Makes-the-wal-receiver-report-WAL-statistics.patch)
> 
>  Thanks for updating the patches!
> 
> 
> > I added the infrastructure code to communicate the WAL receiver
> > stats messages between the WAL receiver and the stats collector,
> > and
> > the stats for WAL receiver is counted in pg_stat_wal_receiver.
> > What do you think?
> 
>  On second thought, this idea seems not good. Because those stats are
>  collected between multiple walreceivers, but other values in
>  pg_stat_wal_receiver is only related to the walreceiver process
>  running
>  at that moment. IOW, it seems strange that some values show dynamic
>  stats and the others show collected stats, even though they are in
>  the same view pg_stat_wal_receiver. Thought?
> >>>
> >>> OK, I fixed it.
> >>> The stats collected in the WAL receiver is exposed in pg_stat_wal
> >>> view in v11 patch.
> >>
> >> Thanks for updating the patches! I'm now reading 001 patch.
> >>
> >> +/* Check whether the WAL file was synced to disk right now */
> >> +if (enableFsync &&
> >> +(sync_method == SYNC_METHOD_FSYNC ||
> >> + sync_method == SYNC_METHOD_FSYNC_WRITETHROUGH ||
> >> + sync_method == SYNC_METHOD_FDATASYNC))
> >> +{
> >>
> >> Isn't it better to make issue_xlog_fsync() return immediately
> >> if enableFsync is off, sync_method is open_sync or open_data_sync,
> >> to simplify the code more?
> >
> > Thanks for the comments.
> > I added the above code in v12 patch.
> >
> >>
> >> +/*
> >> + * Send WAL statistics only if WalWriterDelay has elapsed
> to
> >> minimize
> >> + * the overhead in WAL-writing.
> >> + */
> >> +if (rc & WL_TIMEOUT)
> >> +pgstat_send_wal();
> >>
> >> On second thought, this change means that it always takes
> >> wal_writer_delay
> >> before walwriter's WAL stats is sent after XLogBackgroundFlush() is
> >> called.
> >> For example, if wal_writer_delay is set to several seconds, some
> >> values in
> >> pg_stat_wal would be not up-to-date mean

Re: [PATCH] Add support for ON UPDATE/DELETE actions on ALTER CONSTRAINT

[Ibrar Ahmed]

On Mon, Aug 3, 2020 at 9:31 PM Wolfgang Walther 
wrote:

> Tom Lane:
> > We don't generally act that way in other ALTER commands and I don't see
> > a strong argument to start doing so here.  [...]
> >
> > In short, I'm inclined to argue that this variant of ALTER TABLE
> > should replace *all* the fields of the constraint with the same
> > properties it'd have if you'd created it fresh using the same syntax.
> > This is by analogy to CREATE OR REPLACE commands, which don't
> > preserve any of the old properties of the replaced object.  Given
> > the interactions between these fields, I think you're going to end up
> > with a surprising mess of ad-hoc choices if you do differently.
> > Indeed, you already have, but I think it'll get worse if anyone
> > tries to extend the feature set further.
>
> I don't think the analogy to CREATE OR REPLACE holds. Semantically
> REPLACE and ALTER are very different. Using ALTER the expectation is to
> change something, keeping everything else unchanged. Looking at all the
> other ALTER TABLE actions, especially ALTER COLUMN, it looks like every
> command does exactly one thing and not more. I don't think deferrability
> and ON UPDATE / ON CASCADE should be changed together at all, neither
> implicitly nor explicitly.
>
> There seems to be a fundamental difference between deferrability and the
> ON UPDATE/ON DELETE clauses as well - the latter only apply to FOREIGN
> KEYs, while the former apply to multiple types of constraints.
>
> Matheus de Oliveira:
> > I'm still not sure if the chosen path is the best way. But I'd be glad
> > to follow any directions we all see fit.
> >
> > For now, this patch applies two methods:
> > 1. Changes full constraint definition (which keeps compatibility with
> > current ALTER CONSTRAINT):
> >  ALTER CONSTRAINT [] [] []
> > 2. Changes only the subset explicit seem in the command (a new way, I've
> > chosen to just add SET in the middle, similar to `ALTER COLUMN ... SET
> > {DEFAULT | NOT NULL}` ):
> >  ALTER CONSTRAINT SET [] [] []
> >
> > I'm OK with changing the approach, we just need to chose the color :D
>
> The `ALTER CONSTRAINT SET [] [] []`
> has the same problem about implied changes: What happens if you only do
> e.g. ALTER CONSTRAINT SET ON UPDATE xy - will the ON DELETE part be kept
> as-is or set to the default?
>
> Also, since the ON UPDATE/ON DELETE just applies to FOREIGN KEYs and no
> other constraints, there's one level of "nesting" missing in your SET
> variant, I think.
>
> I suggest to:
>
> - keep `ALTER CONSTRAINT constraint_name [ DEFERRABLE | NOT DEFERRABLE ]
> [ INITIALLY DEFERRED | INITIALLY IMMEDIATE ]` exactly as-is
>
> - add both:
>   + `ALTER CONSTRAINT constraint_name [ALTER] FOREIGN KEY ON UPDATE
> referential_action`
>   + `ALTER CONSTRAINT constraint_name [ALTER] FOREIGN KEY ON DELETE
> referential_action`
>
> This does not imply any changes, that are not in the command - very much
> analog to the ALTER COLUMN variants.
>
> This could also be extended in the future with stuff like `ALTER
> CONSTRAINT constraint_name [ALTER] FOREIGN KEY MATCH [ FULL | PARTIAL |
> SIMPLE ]`.
>
>
>

This patch set no longer applies
http://cfbot.cputube.org/patch_32_1533.log

Can we get a rebase?

I am marking the patch "Waiting on Author"

-- 
Ibrar Ahmed

Re: Improvements and additions to COPY progress reporting

[Matthias van de Meent]

On Thu, 4 Mar 2021 at 11:38, Michael Paquier  wrote:
>
> On Thu, Mar 04, 2021 at 12:16:17PM +0530, Bharath Rupireddy wrote:
> > IMO, the phrasing proposed by Justin upthread looks good. It's like this:
> >
> > > +Each backend running ANALYZE will report its 
> > > progress in
> > > +the pg_stat_progress_analyze view. See
>
> No objections to just go with that.  As a new patch set is needed, I
> am switching the CF entry to "Waiting on Author".

Thanks for all your comments, and sorry for the delayed response.
Please find attached a new version of the patch set, that is rebased
and contains the requested changes:

1/3:
Docs:
- on which the COPY command is executed
+ on which the COPY command is being executed
Reworded existing commment:
- /* Increment amount of processed tuples and update the progress */
+ /* Increment the number of processed tuples, and report the progress */

2/3:
Docs:
- ... report its progress to ...
+ ... report its progress in ...
- report its progress to the >pg_stat_progress_cluster< ...
+ report its progress in the >pg_stat_progress_cluster< view ...

3/3:
No changes

I believe that that was the extent of the not-yet-resolved comments
and suggestions.


With regards,

Matthias van de Meent.
From 7831549452b6d95c3db9060333750256675ff322 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Josef=20=C5=A0im=C3=A1nek?= 
Date: Tue, 9 Feb 2021 13:06:45 +0100
Subject: [PATCH v11 3/3] Add copy progress reporting regression tests.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This tests some basic features of copy progress reporting.

Sadly, we can only request one snapshot of progress_reporting each
transaction / statement, so we can't (easily) get intermediate results without
each result requiring a seperate statement being run.

Author: Josef Šimánek, Matthias van de Meent
---
 src/test/regress/input/copy.source  | 60 +
 src/test/regress/output/copy.source | 47 ++
 2 files changed, 107 insertions(+)

diff --git a/src/test/regress/input/copy.source b/src/test/regress/input/copy.source
index a1d529ad36..4f1cbc73d2 100644
--- a/src/test/regress/input/copy.source
+++ b/src/test/regress/input/copy.source
@@ -201,3 +201,63 @@ select * from parted_copytest where b = 1;
 select * from parted_copytest where b = 2;
 
 drop table parted_copytest;
+
+--
+-- progress reporting
+--
+
+-- setup
+-- reuse employer datatype, that has a small sized data set
+
+create table progress_reporting (
+	name 		text,
+	age			int4,
+	location 	point,
+	salary 		int4,
+	manager 	name
+);
+
+-- Add a trigger to 'raise info' the contents of pg_stat_progress_copy. This
+-- allows us to test and verify the contents of this view, which would
+-- otherwise require a concurrent session checking that table.
+create function notice_after_progress_reporting() returns trigger AS
+$$
+declare report record;
+begin
+	-- We cannot expect 'pid' nor 'relid' to be consistent over runs due to
+	-- variance in system process ids, and concurrency in runs of tests.
+	-- Additionally, due to the usage of this test in pg_regress, the 'datid'
+	-- also is not consistent between runs.
+	select into report (to_jsonb(r) - '{pid,relid,datid}'::text[]) as value
+		from pg_stat_progress_copy r
+		where pid = pg_backend_pid();
+
+	raise info 'progress: %', report.value::text;
+
+	RETURN NEW;
+END;
+$$ LANGUAGE plpgsql;
+
+create trigger check_after_progress_reporting
+	after insert on progress_reporting
+	for each statement
+	execute function notice_after_progress_reporting();
+
+-- reporting of STDIO imports, and correct bytes-processed/tuples-processed reporting
+
+copy progress_reporting from stdin;
+sharon	25	(15,12)	1000	sam
+sam	30	(10,5)	2000	bill
+bill	20	(11,10)	1000	sharon
+\.
+
+-- reporting of FILE imports, and correct reporting of tuples-excluded
+
+copy progress_reporting from '@abs_srcdir@/data/emp.data'
+	where (salary < 2000);
+
+-- cleanup progress_reporting
+
+drop trigger check_after_progress_reporting on progress_reporting;
+drop function notice_after_progress_reporting();
+drop table progress_reporting;
diff --git a/src/test/regress/output/copy.source b/src/test/regress/output/copy.source
index 938d3551da..f3596bdd15 100644
--- a/src/test/regress/output/copy.source
+++ b/src/test/regress/output/copy.source
@@ -165,3 +165,50 @@ select * from parted_copytest where b = 2;
 (1 row)
 
 drop table parted_copytest;
+--
+-- progress reporting
+--
+-- setup
+-- reuse employer datatype, that has a small sized data set
+create table progress_reporting (
+	name 		text,
+	age			int4,
+	location 	point,
+	salary 		int4,
+	manager 	name
+);
+-- Add a trigger to 'raise info' the contents of pg_stat_progress_copy. This
+-- allows us to test and verify the contents of this view, which would
+-- otherwise require a concurrent session checking that table.
+create function notice_after_progress_reporting() returns trigger AS
+$$
+declare report record;
+begi

Re: Parallel INSERT (INTO ... SELECT ...)

[Amit Kapila]

On Thu, Mar 4, 2021 at 4:37 PM Amit Kapila  wrote:
>
> On Fri, Feb 26, 2021 at 10:37 AM Amit Langote  wrote:
> >
> > I realized that there is a race condition that will allow a concurrent
> > backend to invalidate a parallel plan (for insert into a partitioned
> > table) at a point in time when it's too late for plancache.c to detect
> > it.  It has to do with how plancache.c locks the relations affected by
> > a cached query and its cached plan.  Specifically,
> > AcquireExecutorLocks(), which locks the relations that need to be
> > locked before the plan could be considered safe to execute, does not
> > notice the partitions that would have been checked for parallel safety
> > when the plan was made.  Given that AcquireExecutorLocks() only loops
> > over PlannedStmt.rtable and locks exactly the relations found there,
> > partitions are not locked.  This means that a concurrent backend can,
> > for example, add an unsafe trigger to a partition before a parallel
> > worker inserts into it only to fail when it does.
> >
> > Steps to reproduce (tried with v19 set of patches):
> >
> > drop table if exists rp, foo;
> > create table rp (a int) partition by range (a);
> > create table rp1 partition of rp for values from (minvalue) to (0);
> > create table rp2 partition of rp for values from (0) to (maxvalue);
> > create table foo (a) as select generate_series(1, 100);
> > set plan_cache_mode to force_generic_plan;
> > set enable_parallel_dml to on;
> > deallocate all;
> > prepare q as insert into rp select * from foo where a%2 = 0;
> > explain analyze execute q;
> >
> > At this point, attach a debugger to this backend and set a breakpoint
> > in AcquireExecutorLocks() and execute q again:
> >
> > -- will execute the cached plan
> > explain analyze execute q;
> >
> > Breakpoint will be hit.  Continue till return from the function and
> > stop. Start another backend and execute this:
> >
> > -- will go through, because no lock still taken on the partition
> > create or replace function make_table () returns trigger language
> > plpgsql as $$ begin create table bar(); return null; end; $$ parallel
> > unsafe;
> > create trigger ai_rp2 after insert on rp2 for each row execute
> > function make_table();
> >
> > Back to the debugger, hit continue to let the plan execute.  You
> > should see this error:
> >
> > ERROR:  cannot start commands during a parallel operation
> > CONTEXT:  SQL statement "create table bar()"
> > PL/pgSQL function make_table() line 1 at SQL statement parallel worker
> >
> > The attached patch fixes this,
> >
>
> One thing I am a bit worried about this fix is that after this for
> parallel-mode, we will maintain partitionOids at two places, one in
> plannedstmt->relationOids and the other in plannedstmt->partitionOids.
> I guess you have to do that because, in AcquireExecutorLocks, we can't
> find which relationOids are corresponding to partitionOids, am I
> right? If so, why not just maintain them in plannedstmt->partitionOids
> and then make PlanCacheRelCallback consider it? Also, in
> standard_planner, we should add these partitionOids only for
> parallel-mode.
>

One more point I was thinking about is whether we need to worry about
locking indexes during prepared query execution (similar to what we do
for AcquireExecutorLocks). I think we shouldn't be bothered to lock
those or even retain lock during parallel-safety checks because one
cannot change index expression or predicate. Is my understanding
correct or am I missing something and we should be worried about them
as well.

-- 
With Regards,
Amit Kapila.

Re: Parallel INSERT (INTO ... SELECT ...)

[Greg Nancarrow]

On Thu, Mar 4, 2021 at 10:07 PM Amit Kapila  wrote:
>
> On Fri, Feb 26, 2021 at 10:37 AM Amit Langote  wrote:
> >
> > I realized that there is a race condition that will allow a concurrent
> > backend to invalidate a parallel plan (for insert into a partitioned
> > table) at a point in time when it's too late for plancache.c to detect
> > it.  It has to do with how plancache.c locks the relations affected by
> > a cached query and its cached plan.  Specifically,
> > AcquireExecutorLocks(), which locks the relations that need to be
> > locked before the plan could be considered safe to execute, does not
> > notice the partitions that would have been checked for parallel safety
> > when the plan was made.  Given that AcquireExecutorLocks() only loops
> > over PlannedStmt.rtable and locks exactly the relations found there,
> > partitions are not locked.  This means that a concurrent backend can,
> > for example, add an unsafe trigger to a partition before a parallel
> > worker inserts into it only to fail when it does.
> >
> > Steps to reproduce (tried with v19 set of patches):
> >
> > drop table if exists rp, foo;
> > create table rp (a int) partition by range (a);
> > create table rp1 partition of rp for values from (minvalue) to (0);
> > create table rp2 partition of rp for values from (0) to (maxvalue);
> > create table foo (a) as select generate_series(1, 100);
> > set plan_cache_mode to force_generic_plan;
> > set enable_parallel_dml to on;
> > deallocate all;
> > prepare q as insert into rp select * from foo where a%2 = 0;
> > explain analyze execute q;
> >
> > At this point, attach a debugger to this backend and set a breakpoint
> > in AcquireExecutorLocks() and execute q again:
> >
> > -- will execute the cached plan
> > explain analyze execute q;
> >
> > Breakpoint will be hit.  Continue till return from the function and
> > stop. Start another backend and execute this:
> >
> > -- will go through, because no lock still taken on the partition
> > create or replace function make_table () returns trigger language
> > plpgsql as $$ begin create table bar(); return null; end; $$ parallel
> > unsafe;
> > create trigger ai_rp2 after insert on rp2 for each row execute
> > function make_table();
> >
> > Back to the debugger, hit continue to let the plan execute.  You
> > should see this error:
> >
> > ERROR:  cannot start commands during a parallel operation
> > CONTEXT:  SQL statement "create table bar()"
> > PL/pgSQL function make_table() line 1 at SQL statement parallel worker
> >
> > The attached patch fixes this,
> >
>
> One thing I am a bit worried about this fix is that after this for
> parallel-mode, we will maintain partitionOids at two places, one in
> plannedstmt->relationOids and the other in plannedstmt->partitionOids.
> I guess you have to do that because, in AcquireExecutorLocks, we can't
> find which relationOids are corresponding to partitionOids, am I
> right? If so, why not just maintain them in plannedstmt->partitionOids
> and then make PlanCacheRelCallback consider it?

Maybe Amit Langote can kindly comment on this, as it would involve
updates to his prior partition-related fixes.

>Also, in
> standard_planner, we should add these partitionOids only for
> parallel-mode.
>

It is doing that in v20 patch (what makes you think it isn't?).

Regards,
Greg Nancarrow
Fujitsu Australia

Re: Parallel INSERT (INTO ... SELECT ...)

[Amit Kapila]

On Thu, Mar 4, 2021 at 5:24 PM Greg Nancarrow  wrote:
>
> On Thu, Mar 4, 2021 at 10:07 PM Amit Kapila  wrote:
> >
>
> >Also, in
> > standard_planner, we should add these partitionOids only for
> > parallel-mode.
> >
>
> It is doing that in v20 patch (what makes you think it isn't?).
>

The below code snippet:
+ /* For AcquireExecutorLocks(). */
+ if (IsModifySupportedInParallelMode(parse->commandType))
+ result->partitionOids = glob->partitionOids;

I understand that you have a check for the parallel mode in
AcquireExecutorLocks but why can't we have it before adding that to
planned statement

-- 
With Regards,
Amit Kapila.

make coverage-html would fail within build directory separate from source tree

[walker]

Hi, hackers

During installation from source code, I created a build directory separate from 
the source tree, and execute the following command in the build directory:
/home/postgres/postgresql-13.2/configure -- enable-coverage
make
make check
make coverage-html


However, while executing make coverage-html, it failed with the following error 
messages:
/bin/lcov --gcov-tool /bin/gcov -q --no-external -c -i -d . -d 
/home/postgres/postgresql-13.2/ -o lcve_base.info
...
geninfo: ERROR: no .gcno files found in /home/postgres/postgresql-13.2/!
make: *** [lcov_base.info] Error 255
make: *** Deleting file 'lcov_base.info'




if I repeat the above steps within the source tree directory, make 
coverage-html works fine. From the official documentation, I didn't find any 
limitations for "make coverage-html", not sure if I miss something.


thanks
walker
 

Re: [PATCH] pgbench: Bug fix for the -d option

[Michael Paquier]

On Tue, Mar 02, 2021 at 11:52:33AM +0900, miyake_kouta wrote:
> I modified the patch based on other binaries.
> In this new patch, if there is a $PGUSER, then it's set to login.
> If not, get_user_name_or_exit is excuted.
> Plese let me know what you think about this change.

Your patch makes the database selection slightly better, but I think
that we can do better and simpler than that.  So please see the
attached.

One thing on HEAD that looks like a bug to me is that if one uses a
pgbench command without specifying user, port and/or name in the
command for an environment without PGDATABASE, PGPORT and PGHOST set,
then the debug log just before doConnect() prints empty strings for
all that, which is basically useless so one has no idea where the
connection happens. Like any other src/bin/ facilities, let's instead
remove all the getenv() calls at the beginning of pgbench.c and let's
libpq handle those environment variables if the parameters are NULL
(aka in the case of no values given directly in the options).  This
requires to move the debug log after doConnect(), which is no big deal
anyway as a failure results in an exit(1) immediately with a log
telling where the connection failed.

What do you think?
--
Michael
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 31a4df45f5..ab56272f2e 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -32,6 +32,7 @@
 #endif
 
 #include "postgres_fe.h"
+#include "common/username.h"
 
 #include 
 #include 
@@ -240,10 +241,10 @@ bool		is_connect;			/* establish connection for each transaction */
 bool		report_per_command; /* report per-command latencies */
 int			main_pid;			/* main process id used in log filename */
 
-char	   *pghost = "";
-char	   *pgport = "";
-char	   *login = NULL;
-char	   *dbName;
+const char *pghost = NULL;
+const char *pgport = NULL;
+const char *username = NULL;
+const char *dbName = NULL;
 char	   *logfile_prefix = NULL;
 const char *progname;
 
@@ -1191,7 +1192,7 @@ doConnect(void)
 		keywords[1] = "port";
 		values[1] = pgport;
 		keywords[2] = "user";
-		values[2] = login;
+		values[2] = username;
 		keywords[3] = "password";
 		values[3] = password;
 		keywords[4] = "dbname";
@@ -5483,13 +5484,6 @@ main(int argc, char **argv)
 		}
 	}
 
-	if ((env = getenv("PGHOST")) != NULL && *env != '\0')
-		pghost = env;
-	if ((env = getenv("PGPORT")) != NULL && *env != '\0')
-		pgport = env;
-	else if ((env = getenv("PGUSER")) != NULL && *env != '\0')
-		login = env;
-
 	state = (CState *) pg_malloc0(sizeof(CState));
 
 	/* set random seed early, because it may be used while parsing scripts. */
@@ -5610,7 +5604,7 @@ main(int argc, char **argv)
 }
 break;
 			case 'U':
-login = pg_strdup(optarg);
+username = pg_strdup(optarg);
 break;
 			case 'l':
 benchmarking_option_set = true;
@@ -5860,10 +5854,10 @@ main(int argc, char **argv)
 	{
 		if ((env = getenv("PGDATABASE")) != NULL && *env != '\0')
 			dbName = env;
-		else if (login != NULL && *login != '\0')
-			dbName = login;
+		else if ((env = getenv("PGUSER")) != NULL && *env != '\0')
+			dbName = env;
 		else
-			dbName = "";
+			dbName = get_user_name_or_exit(progname);
 	}
 
 	if (optind < argc)
@@ -6026,16 +6020,16 @@ main(int argc, char **argv)
 		initRandomState(&state[i].cs_func_rs);
 	}
 
-	pg_log_debug("pghost: %s pgport: %s nclients: %d %s: %d dbName: %s",
- pghost, pgport, nclients,
- duration <= 0 ? "nxacts" : "duration",
- duration <= 0 ? nxacts : duration, dbName);
-
 	/* opening connection... */
 	con = doConnect();
 	if (con == NULL)
 		exit(1);
 
+	pg_log_debug("pghost: %s pgport: %s nclients: %d %s: %d dbName: %s",
+ PQhost(con), PQport(con), nclients,
+ duration <= 0 ? "nxacts" : "duration",
+ duration <= 0 ? nxacts : duration, PQdb(con));
+
 	if (internal_script_used)
 		GetTableInfo(con, scale_given);
 


signature.asc
Description: PGP signature

Re: Improvements and additions to COPY progress reporting

[Bharath Rupireddy]

On Thu, Mar 4, 2021 at 5:02 PM Matthias van de Meent
 wrote:
>
> On Thu, 4 Mar 2021 at 11:38, Michael Paquier  wrote:
> >
> > On Thu, Mar 04, 2021 at 12:16:17PM +0530, Bharath Rupireddy wrote:
> > > IMO, the phrasing proposed by Justin upthread looks good. It's like this:
> > >
> > > > +Each backend running ANALYZE will report its 
> > > > progress in
> > > > +the pg_stat_progress_analyze view. See
> >
> > No objections to just go with that.  As a new patch set is needed, I
> > am switching the CF entry to "Waiting on Author".
>
> Thanks for all your comments, and sorry for the delayed response.
> Please find attached a new version of the patch set, that is rebased
> and contains the requested changes:
>
> 1/3:
> Docs:
> - on which the COPY command is executed
> + on which the COPY command is being executed
> Reworded existing commment:
> - /* Increment amount of processed tuples and update the progress */
> + /* Increment the number of processed tuples, and report the progress */

LGTM.

> 2/3:
> Docs:
> - ... report its progress to ...
> + ... report its progress in ...
> - report its progress to the >pg_stat_progress_cluster< ...
> + report its progress in the >pg_stat_progress_cluster< view ...

+   
+Each backend running VACUUM without the
+FULL option will report its progress in the
+pg_stat_progress_vacuum view. Backends running
+VACUUM with the FULL option report
+progress in the pg_stat_progress_cluster view
+instead. See  and
+ for details.
+   

I think a typo, missing "will" between option and report - it's "with
the FULL option will report"

Except the above typo, 0002 LGTM.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

Re: Improvements and additions to COPY progress reporting

[Josef Šimánek]

čt 4. 3. 2021 v 12:32 odesílatel Matthias van de Meent
 napsal:
>
> On Thu, 4 Mar 2021 at 11:38, Michael Paquier  wrote:
> >
> > On Thu, Mar 04, 2021 at 12:16:17PM +0530, Bharath Rupireddy wrote:
> > > IMO, the phrasing proposed by Justin upthread looks good. It's like this:
> > >
> > > > +Each backend running ANALYZE will report its 
> > > > progress in
> > > > +the pg_stat_progress_analyze view. See
> >
> > No objections to just go with that.  As a new patch set is needed, I
> > am switching the CF entry to "Waiting on Author".
>
> Thanks for all your comments, and sorry for the delayed response.
> Please find attached a new version of the patch set, that is rebased
> and contains the requested changes:
>
> 1/3:
> Docs:
> - on which the COPY command is executed
> + on which the COPY command is being executed
> Reworded existing commment:
> - /* Increment amount of processed tuples and update the progress */
> + /* Increment the number of processed tuples, and report the progress */
>
> 2/3:
> Docs:
> - ... report its progress to ...
> + ... report its progress in ...
> - report its progress to the >pg_stat_progress_cluster< ...
> + report its progress in the >pg_stat_progress_cluster< view ...
>
> 3/3:
> No changes
>
> I believe that that was the extent of the not-yet-resolved comments
> and suggestions.

LGTM, special thanks for taking over the work on initial COPY progress
regression tests.

>
> With regards,
>
> Matthias van de Meent.

Re: Printing backtrace of postgres processes

[Bharath Rupireddy]

On Mon, Mar 1, 2021 at 10:43 AM torikoshia  wrote:
> Since the current patch use BackendPidGetProc(), it does not
> support this feature not only postmaster, logging, and
> statistics but also checkpointer, background writer, and
> walwriter.
>
> And when I specify pid of these PostgreSQL processes, it
> says "PID  is not a PostgreSQL server process".
>
> I think it may confuse users, so it might be worth
> changing messages for those PostgreSQL processes.
> AuxiliaryPidGetProc() may help to do it.

Exactly this was the doubt I got when I initially reviewed this patch.
And I felt it should be discussed in a separate thread, you may want
to update your thoughts there [1].

[1] - 
https://www.postgresql.org/message-id/CALj2ACW7Rr-R7mBcBQiXWPp%3DJV5chajjTdudLiF5YcpW-BmHhg%40mail.gmail.com

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

Re: [PATCH] regexp_positions ( string text, pattern text, flags text ) → setof int4range[]

[Joel Jacobson]

On Tue, Mar 2, 2021, at 06:31, Tom Lane wrote:
> "Joel Jacobson"  writes:
> > Unless fixed, then the way I see it, I don't think we can use int4range[] 
> > for regexp_positions(),
> 
> Yeah.  It's a cute idea, but the semantics aren't quite right.

Having abandoned the cute idea that didn't work,
here comes a new patch with a regexp_positions() instead returning
setof record (start_pos integer[], end_pos integer[]).

Example:

SELECT * FROM regexp_positions('foobarbequebazilbarfbonk', 
$re$(b[^b]+)(b[^b]+)$re$, 'g');
start_pos | end_pos
---+-
{3,6} | {6,11}
{11,16}   | {16,20}
(2 rows)

Based on HEAD (040af779382e8e4797242c49b93a5a8f9b79c370).

I've updated docs and tests.

/Joel

0002-regexp-positions.patch
Description: Binary data

Re: make coverage-html would fail within build directory separate from source tree

[Alvaro Herrera]

On 2021-Mar-04, walker wrote:

> Hi, hackers
> 
> During installation from source code, I created a build directory separate 
> from the source tree, and execute the following command in the build 
> directory:
> /home/postgres/postgresql-13.2/configure -- enable-coverage
> make
> make check
> make coverage-html
> 
> 
> However, while executing make coverage-html, it failed with the following 
> error messages:
> /bin/lcov --gcov-tool /bin/gcov -q --no-external -c -i -d . -d 
> /home/postgres/postgresql-13.2/ -o lcve_base.info
> ...
> geninfo: ERROR: no .gcno files found in /home/postgres/postgresql-13.2/!
> make: *** [lcov_base.info] Error 255
> make: *** Deleting file 'lcov_base.info'

Hmm, it works fine for me.  config.log says I do this (in
/pgsql/build/master-coverage):

  $ /pgsql/source/REL_13_STABLE/configure --enable-debug --enable-depend 
--enable-cassert --enable-coverage 
--cache-file=/home/alvherre/run/pgconfig.master-coverage.cache 
--enable-thread-safety --enable-tap-tests --with-python --with-perl --with-tcl 
--with-openssl --with-libxml --with-tclconfig=/usr/lib/tcl8.6 
PYTHON=/usr/bin/python3 --prefix=/pgsql/install/master-coverage 
--with-pgport=55451

I do run "make install" too, though (and "make -C contrib install").
Not sure if that makes a difference.  
But for sure there are no .gcno files in the source dir -- they're all
in the build dir.


-- 
Álvaro Herrera   Valdivia, Chile
"La persona que no quería pecar / estaba obligada a sentarse
 en duras y empinadas sillas/ desprovistas, por cierto
 de blandos atenuantes"  (Patricio Vogel)

Re: make coverage-html would fail within build directory separate from source tree

[Alvaro Herrera]

On 2021-Mar-04, Alvaro Herrera wrote:

> On 2021-Mar-04, walker wrote:
> 
> > Hi, hackers
> > 
> > During installation from source code, I created a build directory separate 
> > from the source tree, and execute the following command in the build 
> > directory:
> > /home/postgres/postgresql-13.2/configure -- enable-coverage
> > make
> > make check
> > make coverage-html
> > 
> > 
> > However, while executing make coverage-html, it failed with the following 
> > error messages:
> > /bin/lcov --gcov-tool /bin/gcov -q --no-external -c -i -d . -d 
> > /home/postgres/postgresql-13.2/ -o lcve_base.info
> > ...
> > geninfo: ERROR: no .gcno files found in /home/postgres/postgresql-13.2/!

"make coverage-html" outputs this: note that I get a WARNING about the
source directory rather than an ERROR:

/usr/bin/lcov --gcov-tool /usr/bin/gcov -q --no-external -c -i -d . -d 
/pgsql/source/REL_13_STABLE/ -o lcov_base.info
geninfo: WARNING: no .gcno files found in /pgsql/source/REL_13_STABLE/ - 
skipping!
/usr/bin/lcov --gcov-tool /usr/bin/gcov -q --no-external -c -d . -d 
/pgsql/source/REL_13_STABLE/ -o lcov_test.info
geninfo: WARNING: no .gcda files found in /pgsql/source/REL_13_STABLE/ - 
skipping!
rm -rf coverage
/usr/bin/genhtml -q --legend -o coverage --title='PostgreSQL 13.2' 
--num-spaces=4  lcov_base.info lcov_test.info
touch coverage-html-stamp

(In my system, /pgsql is a symlink to /home/alvhere/Code/pgsql/)

$ geninfo --version
geninfo: LCOV version 1.13


-- 
Álvaro Herrera   Valdivia, Chile

Re: Increase value of OUTER_VAR

[Tomas Vondra]

On 3/4/21 8:43 AM, Andrey Lepikhov wrote:
> On 3/3/21 12:52, Julien Rouhaud wrote:
>> On Wed, Mar 3, 2021 at 4:57 PM Amit Langote 
>> wrote:
>>>
>>> On Wed, Mar 3, 2021 at 5:52 PM David Rowley 
>>> wrote:
 Something like 1 million seems like a more realistic limit to me.
 That might still be on the high side, but it'll likely mean we'd not
 need to revisit this for quite a while.
>>>
>>> +1
>>>
>>> Also, I got reminded of this discussion from not so long ago:
>>>
>>> https://www.postgresql.org/message-id/flat/16302-e45634e2c0e34e97%40postgresql.org
>>>
> Thank you
>>
>> +1
>>
> Ok. I changed the value to 1 million and explained this decision in the
> comment.

IMO just bumping up the constants from ~65k to 1M is a net loss, for
most users. We add this to bitmapsets, which means we're using ~8kB with
the current values, but this jumps to 128kB with this higher value. This
also means bms_next_member etc. have to walk much more memory, which is
bound to have some performance impact for everyone.

Switching to small negative values is a much better idea, but it's going
to be more invasive - we'll have to offset the values in the bitmapsets,
or we'll have to invent a new bitmapset variant that can store negative
values directly (e.g. by keeping two separate bitmaps internally, one
for negative and one for positive values). But that complicates other
stuff too (e.g. bms_next_member now returns -1 to signal "end").


regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: make coverage-html would fail within build directory separate from source tree

[Alvaro Herrera]

On 2021-Mar-04, walker wrote:

> thanks for your reply. it indeed that there are no .gcon files in source tree 
> directory, they're in build tree directory, which results in failures.
> 
> 
> That's a bit wired.
> 
> 
> Add more detailed testing steps:
> mkdir build_dir
> cd build_dir
> /home/postgres/postgresql-13.2/configure -- enable-coverage
> make
> make check
> make coverage-html

Hmm, my build dir is not inside the source dir -- is yours?  Maybe that
makes a difference?  Also, what version of lcov do you have?


-- 
Álvaro Herrera39°49'30"S 73°17'W
"Someone said that it is at least an order of magnitude more work to do
production software than a prototype. I think he is wrong by at least
an order of magnitude."  (Brian Kernighan)

Re: make coverage-html would fail within build directory separate from source tree

[walker]

thanks for your reply. it indeed that there are no .gcon files in source tree 
directory, they're in build tree directory, which results in failures.


That's a bit wired.


Add more detailed testing steps:
mkdir build_dir
cd build_dir
/home/postgres/postgresql-13.2/configure -- enable-coverage
make
make check
make coverage-html


thanks
walker


-- Original --
From:   
 "Alvaro Herrera"   
 

Re: make coverage-html would fail within build directory separate from source tree

[walker]

The same, the build directory is outside the source tree.


the version of lcov is 1.10


thanks
walker




-- Original --
From:   
 "Alvaro Herrera"   
 

Re: make coverage-html would fail within build directory separate from source tree

[Alvaro Herrera]

On 2021-Mar-04, walker wrote:

> The same, the build directory is outside the source tree.
> 
> 
> the version of lcov is 1.10

That seems *really* ancient.  Please try with a fresher version.


-- 
Álvaro Herrera39°49'30"S 73°17'W
"How amazing is that? I call it a night and come back to find that a bug has
been identified and patched while I sleep."(Robert Davidson)
   http://archives.postgresql.org/pgsql-sql/2006-03/msg00378.php

Re: [HACKERS] PATCH: Batch/pipelining support for libpq

[Alvaro Herrera]

On 2021-Mar-04, Ibrar Ahmed wrote:

> The build is failing for this patch, can you please take a look at this?
> 
> https://cirrus-ci.com/task/4568547922804736
> https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.129221
> 
> 
> I am marking the patch "Waiting on Author"

I don't know why you chose to respond to such an old message, but here's
a rebase which also includes the workaround I suggested last night for
the problem of the outdated query printed by error messages, as well as
Justin's suggested fixes.  (I also made a few tweaks to the TAP test).

-- 
Álvaro Herrera   Valdivia, Chile
"No renuncies a nada. No te aferres a nada."

Re: Track replica origin progress for Rollback Prepared

[Amit Kapila]

On Thu, Mar 4, 2021 at 1:40 PM Michael Paquier  wrote:
>
> On Wed, Mar 03, 2021 at 09:06:11AM +0530, Amit Kapila wrote:
> That's perhaps not enough to be an actual objection, but I am not
> really comfortable with the concept of pushing into the tree code that
> would remain untested until all the remaining pieces get done, as that
> means that we have a point in the code history where some code is
> around, but sits around as idle, no?  If this feature is not completed
> within the dev cycle, it would mean that some code remains around
> without any actual way to know if it is useful or not, released as
> such.
>

You are right but not sure if we should try to write the test case to
cover each and every line especially if the test case is sort of
timing dependent which I think is the case here. We don't have any
test even for the original feature committed at 1eb6d6527a, maybe, we
should write a one for that as well. Having said that, I think we use
replication origins to test it. For example:

Create Table t1(c1 int);

SELECT pg_replication_origin_create('regress');
SELECT pg_replication_origin_session_setup('regress');
SELECT pg_replication_origin_xact_setup('0/aabbccdd', '2013-01-01 00:00');
Begin;
Select txid_current();
Insert into t1 values(1);
Prepare Transaction 'foo';
SELECT pg_replication_origin_xact_setup('0/aabbccee', '2013-01-01 00:00');
Rollback Prepared 'foo';
SELECT pg_replication_origin_session_progress(true);

-- Restart the server
SELECT pg_replication_origin_session_setup('regress');
SELECT pg_replication_origin_session_progress(true);

The value before the patch will be aabbccdd and after the patch, it
will be aabbccee.

I think here we have a slight timing thing which is if the checkpoint
happens before restart then the problem won't occur, not sure but
maybe increase the checkpoint_timeout as well to make it reproducible.
I am of opinion that this area won't change much and anyway once
subscriber-side 2PC feature gets committed, we will have few tests
covering this area but I am fine if you still insist to have something
like above and think that we don't need to bother much about timing
stuff.

-- 
With Regards,
Amit Kapila.

Re: Force lookahead in COPY FROM parsing

[John Naylor]

On Thu, Mar 4, 2021 at 5:13 AM Heikki Linnakangas  wrote:
>
> I posted this earlier at
>
https://www.postgresql.org/message-id/9ec25819-0a8a-d51a-17dc-4150bb3cc...@iki.fi
,
> and that led to removing FE/BE protocol version 2 support. That's been
> committed now, so here's COPY FROM patch again, rebased.

Looks good to me. Just a couple minor things:

+ * Look at the next character.  If we're at EOF, c2 will wind
+ * up as '\0' because of the guaranteed pad of raw_buf.
  */
- IF_NEED_REFILL_AND_NOT_EOF_CONTINUE(0);
-
- /* get next char */
  c = copy_raw_buf[raw_buf_ptr];

The new comment seems copy-pasted from the c2 statements further down.

- if (raw_buf_ptr >= copy_buf_len || need_data)
+#define COPY_READ_LINE_LOOKAHEAD 4
+ if (raw_buf_ptr + COPY_READ_LINE_LOOKAHEAD >= copy_buf_len)

Is this #define deliberately put down here rather than at the top of the
file?

- * of the buffer and then we load more data after that.  This case occurs
only
- * when a multibyte character crosses a bufferload boundary.
+ * of the buffer and then we load more data after that.

Is the removed comment really invalidated by this patch? I figured it was
something not affected until the patch to do the encoding conversion in
larger chunks.

--
John Naylor
EDB: http://www.enterprisedb.com

Re: New default role- 'pg_read_all_data'

[gkokolatos]

Hi,




‐‐‐ Original Message ‐‐‐
On Monday, November 23, 2020 11:31 PM, Stephen Frost  wrote:

> Greetings,
>
> -   Anastasia Lubennikova (a.lubennik...@postgrespro.ru) wrote:
>
> > On 29.10.2020 17:19, Stephen Frost wrote:
> >
> > > -   Georgios Kokolatos (gkokola...@protonmail.com) wrote:
> > >
> > > > this patch is in "Ready for committer" state and the Cfbot is happy.
> > > > Glad that's still the case. :)
> > >
> > > > Is there any committer that is available for taking a look at it?
> > > > If there aren't any objections or further comments, I'll take another
> > > > look through it and will commit it during the upcoming CF.
> > >
> > > Thanks!
> > > Stephen
> >
> > CFM reminder. Just in case you forgot about this thread)
> > The commitfest is heading to the end. And there was a plenty of time for
> > anyone to object.
>
> Thanks, I've not forgotten, but it's a bit complicated given that I've
> another patch in progress to rename default roles to be predefined
> roles which conflicts with this one. Hopefully we'll have a few
> comments on that and I can get it committed and this one updated with
> the new naming. I'd rather not commit this one and then immediately
> commit changes over top of it.

May I enquire about the status of the current?


//Georgios

>
> Thanks again!
>
> Stephen

011_crash_recovery.pl failes using wal_block_size=16K

[walker]

Hi, hackers



cd source_dir
./configure --enable-tap-tests --with-wal-blocksize=16
make world
make install-world
cd source_dir/src/test/recovery
make check PROVE_TESTS='t/011_crash_recovery.pl' PROVE_FLAGS='--verbose'


the output of the last command is:
011_crash_recovery.pl ..
1..3
ok 1 - own xid is in-progress
not ok 2 - new xid after restart is greater


#    Failed test 'new xid after restart is greater'
#    at t/011_crash_recovery.pl line 61
#       '485'
#             >
#       '485'
not ok 3 - xid is aborted after crash


#    Failed test 'xid is aborted after crash'
#    at t/011_crash_recovery.pl line 65.
#                got: 'committed'
#        expected: 'aborted'
# Looks like you failed 2 tests of 3.
Dubious test returned 2(stat 512, 0x200)
Failed 2/3 subtests
..




But if I modified something in t/011_crash_recovery.pl, this perl script works 
fine, as follows:
is($node->safe_psql('postgres'), qq[SELECT pg_xact_status('$xid');]),
             'in progress', 'own xid is 
in-progress');


sleep(1);  # here new added, just make sure the CREATE TABLE XLOG can 
be flushed into WAL segment file on disk.


# Crash and restart the postmaster
$node->stop('immediate');
$node->start;




I think the problem is that before crash(simulated by stop with immediate 
mode), the XLOG of "create table mine" didn't get flushed into wal file on 
disk. Instead, if delay some time, e.g. 200ms, or more after issue create 
table, in theory, the data in wal buffer should be written to disk by wal 
writer.


However, I'm not sure the root cause. what's the difference between 
wal_blocksize=8k and wal_blocksize=16k while flushing wal buffer data to disk?


thanks
walker

Re: Feedback on table expansion hook (including patch)

[Peter Eisentraut]

On 07.05.20 10:11, Erik Nordström wrote:
I am looking for feedback on the possibility of adding a table expansion 
hook to PostgreSQL (see attached patch). The motivation for this is to 
allow extensions to optimize table expansion. In particular, TimescaleDB 
does its own table expansion in order to apply a number of 
optimizations, including partition pruning (note that TimescaleDB uses 
inheritance since PostgreSQL 9.6 rather than declarative partitioning ). 
There's currently no official hook for table expansion, but TimescaleDB 
has been using the get_relation_info hook for this purpose. 
Unfortunately, PostgreSQL 12 broke this for us since it moved expansion 
to a later stage where we can no longer control it without some pretty 
bad hacks.


Unlike the get_relation_info_hook, your proposed hook would *replace* 
expand_inherited_rtentry() rather than just tack on additional actions. 
That seems awfully fragile.  Could you do with a hook that does 
additional things rather than replace a whole chunk of built-in code?

Re: WIP: document the hook system

[Peter Eisentraut]

On 15.01.21 08:28, Peter Eisentraut wrote:

On 2020-12-31 04:28, David Fetter wrote:

This could probably use a lot of filling in, but having it in the
actual documentation beats needing to know folklore even to know
that the capability is there.


This patch seems quite short of a state where one could begin to 
evaluate it.  Documenting the hooks better seems a worthwhile goal.   I 
think the question is whether we can develop documentation that is 
genuinely useful by itself without studying the relevant source code. 
This submission does not address that question.


There hasn't been any meaningful progress on this, and no new patch to 
look at, so I'm proposing to set this as returned with feedback.

Re: [HACKERS] PATCH: Batch/pipelining support for libpq

[Alvaro Herrera]

Apparently, the archives system or the commitfest system is not picking
up new messages to the thread, so the CF app is trying to apply a
very old patch version.  I'm not sure what's up with that.  Thomas, any
clues on where to look?

-- 
Álvaro Herrera   Valdivia, Chile
"Oh, great altar of passive entertainment, bestow upon me thy discordant images
at such speed as to render linear thought impossible" (Calvin a la TV)

Re: make coverage-html would fail within build directory separate from source tree

[walker]

install a newer version of lcov 1.13, it works fine with WARNING just same as 
yours.


much appreciated


thanks
walker


-- Original --
From:   
 "Alvaro Herrera"   
 
http://archives.postgresql.org/pgsql-sql/2006-03/msg00378.php

Re: authtype parameter in libpq

[Daniel Gustafsson]

> On 3 Mar 2021, at 14:47, Peter Eisentraut  wrote:
> 
> On 26.02.21 21:02, Daniel Gustafsson wrote:
>> When looking at disallowing SSL compression I found the parameter "authtype"
>> which was deprecated in commit d5bbe2aca55bc8 on January 26 1998.  While I do
>> think there is a case to be made for the backwards compatibility having run 
>> its
>> course on this one, shouldn't we at least remove the environment variable and
>> default compiled fallback for it to save us a getenv call when filling in the
>> option defaults?
> 
> The argument of avoiding unnecessary getenv() calls is sensible.  PGTTY 
> should get the same treatment.

The reason I left PGTTY alone is that we still have a way to extract the value
set via PQtty(), so removing one or two ways of setting it while at the same
time allowing the value to be read back seemed inconsistent regardless of its
obsolescence.

authtype is completely dead in terms of reading back the value, to the point of
it being a memleak if it indeed was found in as an environment variable.

> But I tend to think we should remove them both altogether (modulo ABI and API 
> preservation).

No disagreement from me, the attached takes a stab at that to get an idea what
it would look like.  PQtty is left to maintain API stability but the parameters
are removed from the conn object as thats internal to libpq.

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



v2-0001-Remove-deprecated-parameters-authtype-and-pqtty.patch
Description: Binary data

Re: Increase value of OUTER_VAR

[Tom Lane]

Tomas Vondra  writes:
> IMO just bumping up the constants from ~65k to 1M is a net loss, for
> most users. We add this to bitmapsets, which means we're using ~8kB with
> the current values, but this jumps to 128kB with this higher value. This
> also means bms_next_member etc. have to walk much more memory, which is
> bound to have some performance impact for everyone.

Hmm, do we really have any places that include OUTER_VAR etc in
bitmapsets?  They shouldn't appear in relid sets, for sure.
I agree though that if they did, this would have bad performance
consequences.

I still think the negative-special-values approach is better.
If there are any places that that would break, we'd find out about
it in short order, rather than having a silent performance lossage.

regards, tom lane

Re: make coverage-html would fail within build directory separate from source tree

[Tom Lane]

Alvaro Herrera  writes:
> Hmm, my build dir is not inside the source dir -- is yours?

I recall that the gcc build instructions strongly warn against that
sort of setup.  Maybe we should too?

Actually, our build instructions already say this specifically:

You can also run configure in a directory outside the source tree, and
then build there, if you want to keep the build directory separate
from the original source files. This procedure is called a VPATH
build ...

Maybe "outside the source tree" needs to be emphasized a bit more.

regards, tom lane

Re: [PATCH] Support empty ranges with bounds information

[Isaac Morland]

On Thu, 4 Mar 2021 at 01:25, Joel Jacobson  wrote:

Suggestion #1: Use [] as the canonical form for discrete types.
> This would allow creating ranges for all values for discrete types.
>

I won't reiterate here, but there are fundamental reasons why [) is
definitely the right default and canonical form.

In any case, you can create a range containing the last value:

odyssey=> select 2147483647 <@ int4range (0, null);
 ?column?
--
 t
(1 row)

odyssey=>

It does seem reasonable to me to change it so that specifying the last
value as the right end with ] would use a null endpoint instead of
producing an error when it tries to increment the bound.

Re: pg_amcheck contrib application

[Robert Haas]

Most of these changes sound good. I'll go through the whole patch
again today, or as much of it as I can. But before I do that, I want
to comment on this point specifically.

On Thu, Mar 4, 2021 at 1:25 AM Mark Dilger  wrote:
> I think this is fixed up now.  There is an interaction with amcheck's 
> verify_heapam(), where that function raises an error if the startblock or 
> endblock arguments are out of bounds for the relation in question.  Rather 
> than aborting the entire pg_amcheck run, it avoids passing inappropriate 
> block ranges to verify_heapam() and outputs a warning, so:
>
> % pg_amcheck mark.dilger -t foo -t pg_class --progress -v --startblock=35 
> --endblock=77
> pg_amcheck: in database "mark.dilger": using amcheck version "1.3" in schema 
> "public"
> 0/6 relations (0%)  0/55 pages (0%)
> pg_amcheck: checking table "mark.dilger"."public"."foo" (oid 16385) (10/45 
> pages)
> pg_amcheck: warning: ignoring endblock option 77 beyond end of table 
> "mark.dilger"."public"."foo"
> pg_amcheck: checking btree index "mark.dilger"."public"."foo_idx" (oid 16388) 
> (30/30 pages)
> pg_amcheck: checking table "mark.dilger"."pg_catalog"."pg_class" (oid 1259) 
> (0/13 pages)
> pg_amcheck: warning: ignoring startblock option 35 beyond end of table 
> "mark.dilger"."pg_catalog"."pg_class"
> pg_amcheck: warning: ignoring endblock option 77 beyond end of table 
> "mark.dilger"."pg_catalog"."pg_class"
> pg_amcheck: checking btree index 
> "mark.dilger"."pg_catalog"."pg_class_relname_nsp_index" (oid 2663) (6/6 pages)
> pg_amcheck: checking btree index 
> "mark.dilger"."pg_catalog"."pg_class_tblspc_relfilenode_index" (oid 3455) 
> (5/5 pages)
> pg_amcheck: checking btree index 
> "mark.dilger"."pg_catalog"."pg_class_oid_index" (oid 2662) (4/4 pages)
> 6/6 relations (100%) 55/55 pages (100%)
>
> The way the (x/y pages) is printed takes into account that the 
> [startblock..endblock] range may reduce the number of pages to check (x) to 
> something less than the number of pages in the relation (y), but the 
> reporting is a bit of a lie when the startblock is beyond the end of the 
> table, as it doesn't get passed to verify_heapam and so the number of blocks 
> checked may be more than the zero blocks reported. I think I might need to 
> fix this up tomorrow, but I want to get what I have in this patch set posted 
> tonight, so it's not fixed here.  Also, there are multiple ways of addressing 
> this, and I'm having trouble deciding which way is best.  I can exclude the 
> relation from being checked at all, or realize earlier that I'm not going to 
> honor the startblock argument and compute the blocks to check correctly.  
> Thoughts?

I think this whole approach is pretty suspect because the number of
blocks in the relation can increase (by relation extension) or
decrease (by VACUUM or TRUNCATE) between the time when we query for
the list of target relations and the time we get around to executing
any queries against them. I think it's OK to use the number of
relation pages for progress reporting because progress reporting is
only approximate anyway, but I wouldn't print them out in the progress
messages, and I wouldn't try to fix up the startblock and endblock
arguments on the basis of how long you think that relation is going to
be. You seem to view the fact that the server reported the error as
the reason for the problem, but I don't agree. I think having the
server report the error here is right, and the problem is that the
error reporting sucked because it was long-winded and didn't
necessarily tell you which table had the problem.

There are a LOT of things that can go wrong when we go try to run
verify_heapam on a table. The table might have been dropped; in fact,
on a busy production system, such cases are likely to occur routinely
if DDL is common, which for many users it is. The system catalog
entries might be screwed up, so that the relation can't be opened.
There might be an unreadable page in the relation, either because the
OS reports an I/O error or something like that, or because checksum
verification fails. There are various other possibilities. We
shouldn't view such errors as low-level things that occur only in
fringe cases; this is a corruption-checking tool, and we should expect
that running it against messed-up databases will be common. We
shouldn't try to interpret the errors we get or make any big decisions
about them, but we should have a clear way of reporting them so that
the user can decide what to do.

Just as an experiment, I suggest creating a database with 100 tables
in it, each with 1 index, and then deleting a single pg_attribute
entry for 10 of the tables, and then running pg_amcheck. I think you
will get 20 errors -  one for each messed-up table and one for the
corresponding index. Maybe you'll get errors for the TOAST tables
checks too, if the tables have TOAST tables, although that seems like
it should be avoidable. Now, now matter what you do, the tool is going
to produc

Re: Increase value of OUTER_VAR

[Tomas Vondra]

On 3/4/21 4:16 PM, Tom Lane wrote:
> Tomas Vondra  writes:
>> IMO just bumping up the constants from ~65k to 1M is a net loss, for
>> most users. We add this to bitmapsets, which means we're using ~8kB with
>> the current values, but this jumps to 128kB with this higher value. This
>> also means bms_next_member etc. have to walk much more memory, which is
>> bound to have some performance impact for everyone.
> 
> Hmm, do we really have any places that include OUTER_VAR etc in
> bitmapsets?  They shouldn't appear in relid sets, for sure.
> I agree though that if they did, this would have bad performance
> consequences.
> 

Hmmm, I don't know. I mostly assumed that if I do pull_varnos() it would
include those values. But maybe that's not supposed to happen.

> I still think the negative-special-values approach is better.
> If there are any places that that would break, we'd find out about
> it in short order, rather than having a silent performance lossage.
> 

OK

regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: Re: [PATCH] regexp_positions ( string text, pattern text, flags text ) → setof int4range[]

[Tom Lane]

"Joel Jacobson"  writes:
> Having abandoned the cute idea that didn't work,
> here comes a new patch with a regexp_positions() instead returning
> setof record (start_pos integer[], end_pos integer[]).

I wonder if a 2-D integer array wouldn't be a better idea,
ie {{startpos1,length1},{startpos2,length2},...}.  My experience
with working with parallel arrays in SQL has been unpleasant.

Also, did you see

https://www.postgresql.org/message-id/fc160ee0-c843-b024-29bb-97b5da61971f%40darold.net

Seems like there may be some overlap in these proposals.

regards, tom lane

Re: Keep notnullattrs in RelOptInfo (Was part of UniqueKey patch series)

[Dmitry Dolgov]

> On Thu, Feb 18, 2021 at 08:58:13PM +0800, Andy Fan wrote:

Thanks for continuing work on this patch!

> On Tue, Feb 16, 2021 at 12:01 PM David Rowley  wrote:
>
> > On Fri, 12 Feb 2021 at 15:18, Andy Fan  wrote:
> > >
> > > On Fri, Feb 12, 2021 at 9:02 AM David Rowley 
> > wrote:
> > >> The reason I don't really like this is that it really depends where
> > >> you want to use RelOptInfo.notnullattrs.  If someone wants to use it
> > >> to optimise something before the base quals are evaluated then they
> > >> might be unhappy that they found some NULLs.
> > >>
> > >
> > > Do you mean the notnullattrs is not set correctly before the base quals
> > are
> > > evaluated?  I think we have lots of data structures which are set just
> > after some
> > > stage.  but notnullattrs is special because it is set at more than 1
> > stage.  However
> > > I'm doubtful it is unacceptable, Some fields ever change their meaning
> > at different
> > > stages like Var->varno.  If a user has a misunderstanding on it, it
> > probably will find it
> > > at the testing stage.
> >
> > You're maybe focusing too much on your use case for notnullattrs. It
> > only cares about NULLs in the result for each query level.
> >
> >  thinks of an example...
> >
> > OK, let's say I decided that COUNT(*) is faster than COUNT(id) so
> > decided that I might like to write a patch which rewrite the query to
> > use COUNT(*) when it was certain that "id" could not contain NULLs.
> >
> > The query is:
> >
> > SELECT p.partid, p.partdesc,COUNT(s.saleid) FROM part p LEFT OUTER
> > JOIN sales s ON p.partid = s.partid GROUP BY p.partid;
> >
> > sale.saleid is marked as NOT NULL in pg_attribute.  As the writer of
> > the patch, I checked the comment for notnullattrs and it says "Not
> > null attrs, start from -FirstLowInvalidHeapAttributeNumber", so I
> > should be ok to assume since sales.saleid is marked in notnullattrs
> > that I can rewrite the query?!
> >
> > The documentation about the RelOptInfo.notnullattrs needs to be clear
> > what exactly it means. I'm not saying your representation of how to
> > record NOT NULL in incorrect. I'm saying that you need to be clear
> > what exactly is being recorded in that field.
> >
> > If you want it to mean "attribute marked here cannot output NULL
> > values at this query level", then you should say something along those
> > lines.
> >
> > However, having said that, because this is a Bitmapset of
> > pg_attribute.attnums, it's only possible to record Vars from base
> > relations.  It does not seem like you have any means to record
> > attributes that are normally NULLable, but cannot produce NULL values
> > due to a strict join qual.
> >
> > e.g: SELECT t.nullable FROM t INNER JOIN j ON t.nullable = j.something;
> >
> > I'd expect the RelOptInfo for t not to contain a bit for the
> > "nullable" column, but there's no way to record the fact that the join
> > RelOptInfo for {t,j} cannot produce a NULL for that column. It might
> > be quite useful to know that for the UniqueKeys patch.
> >
>
> I checked again and found I do miss the check on JoinExpr->quals.  I have
> fixed it in v3 patch. Thanks for the review!
>
> In the attached v3,  commit 1 is the real patch, and commit 2 is just add
> some logs to help local testing.  notnull.sql/notnull.out is the test case
> for
> this patch, both commit 2 and notnull.* are not intended to be committed
> at last.

Just to clarify, this version of notnullattrs here is the latest one,
and another one from "UniqueKey on Partitioned table" thread should be
disregarded?

> Besides the above fix in v3, I changed the comments alongs the notnullattrs
> as below and added a true positive helper function is_var_nullable.

With "true positive" you mean it will always correctly say if a Var is
nullable or not? I'm not sure about this, but couldn't be there still
some cases when a Var belongs to nullable_baserels, but still has some
constraints preventing it from being nullable (e.g. a silly example when
the not nullable column belong to the table, and the query does full
join of this table on itself using this column)?

Is this function necessary for the following patches? I've got an
impression that the discussion in this thread was mostly evolving about
correct description when notnullattrs could be used, not making it
bullet proof.

>   Bitmapset   *notnullattrs;

It looks like RelOptInfo has its own out function _outRelOptInfo,
probably the notnullattrs should be also present there as BITMAPSET_FIELD?

As a side note, I've attached those two new threads to CF item [1],
hopefully it's correct.

[1]: https://commitfest.postgresql.org/32/2433/

Re: About to add WAL write/fsync statistics to pg_stat_wal view

[Fujii Masao]

On 2021/03/04 16:14, Masahiro Ikeda wrote:

On 2021-03-03 20:27, Masahiro Ikeda wrote:

On 2021-03-03 16:30, Fujii Masao wrote:

On 2021/03/03 14:33, Masahiro Ikeda wrote:

On 2021-02-24 16:14, Fujii Masao wrote:

On 2021/02/15 11:59, Masahiro Ikeda wrote:

On 2021-02-10 00:51, David G. Johnston wrote:

On Thu, Feb 4, 2021 at 4:45 PM Masahiro Ikeda
 wrote:


I pgindented the patches.


... XLogWrite, which is invoked during an
XLogFlush request (see ...).  This is also
incremented by the WAL receiver during replication.

("which normally called" should be "which is normally called" or
"which normally is called" if you want to keep true to the original)
You missed the adding the space before an opening parenthesis here and
elsewhere (probably copy-paste)

is ether -> is either
"This parameter is off by default as it will repeatedly query the
operating system..."
", because" -> "as"


Thanks, I fixed them.


wal_write_time and the sync items also need the note: "This is also
incremented by the WAL receiver during replication."


I skipped changing it since I separated the stats for the WAL receiver
in pg_stat_wal_receiver.


"The number of times it happened..." -> " (the tally of this event is
reported in wal_buffers_full in) This is undesirable because ..."


Thanks, I fixed it.


I notice that the patch for WAL receiver doesn't require explicitly
computing the sync statistics but does require computing the write
statistics.  This is because of the presence of issue_xlog_fsync but
absence of an equivalent pg_xlog_pwrite.  Additionally, I observe that
the XLogWrite code path calls pgstat_report_wait_*() while the WAL
receiver path does not.  It seems technically straight-forward to
refactor here to avoid the almost-duplicated logic in the two places,
though I suspect there may be a trade-off for not adding another
function call to the stack given the importance of WAL processing
(though that seems marginalized compared to the cost of actually
writing the WAL).  Or, as Fujii noted, go the other way and don't have
any shared code between the two but instead implement the WAL receiver
one to use pg_stat_wal_receiver instead.  In either case, this
half-and-half implementation seems undesirable.


OK, as Fujii-san mentioned, I separated the WAL receiver stats.
(v10-0002-Makes-the-wal-receiver-report-WAL-statistics.patch)


Thanks for updating the patches!



I added the infrastructure code to communicate the WAL receiver stats messages 
between the WAL receiver and the stats collector, and
the stats for WAL receiver is counted in pg_stat_wal_receiver.
What do you think?


On second thought, this idea seems not good. Because those stats are
collected between multiple walreceivers, but other values in
pg_stat_wal_receiver is only related to the walreceiver process running
at that moment. IOW, it seems strange that some values show dynamic
stats and the others show collected stats, even though they are in
the same view pg_stat_wal_receiver. Thought?


OK, I fixed it.
The stats collected in the WAL receiver is exposed in pg_stat_wal view in v11 
patch.


Thanks for updating the patches! I'm now reading 001 patch.

+    /* Check whether the WAL file was synced to disk right now */
+    if (enableFsync &&
+    (sync_method == SYNC_METHOD_FSYNC ||
+ sync_method == SYNC_METHOD_FSYNC_WRITETHROUGH ||
+ sync_method == SYNC_METHOD_FDATASYNC))
+    {

Isn't it better to make issue_xlog_fsync() return immediately
if enableFsync is off, sync_method is open_sync or open_data_sync,
to simplify the code more?


Thanks for the comments.
I added the above code in v12 patch.



+    /*
+ * Send WAL statistics only if WalWriterDelay has elapsed to minimize
+ * the overhead in WAL-writing.
+ */
+    if (rc & WL_TIMEOUT)
+    pgstat_send_wal();

On second thought, this change means that it always takes wal_writer_delay
before walwriter's WAL stats is sent after XLogBackgroundFlush() is called.
For example, if wal_writer_delay is set to several seconds, some values in
pg_stat_wal would be not up-to-date meaninglessly for those seconds.
So I'm thinking to withdraw my previous comment and it's ok to send
the stats every after XLogBackgroundFlush() is called. Thought?


Thanks, I didn't notice that.

Although PGSTAT_STAT_INTERVAL is 500msec, wal_writer_delay's
default value is 200msec and it may be set shorter time.


Yeah, if wal_writer_delay is set to very small value, there is a risk
that the WAL stats are sent too frequently. I agree that's a problem.



Why don't to make another way to check the timestamp?

+   /*
+    * Don't send a message unless it's been at least
PGSTAT_STAT_INTERVAL
+    * msec since we last sent one
+    */
+   now = GetCurrentTimestamp();
+   if (TimestampDifferenceExceeds(last_report, now,
PGSTAT_STAT_INTERVAL))
+   {
+   pgstat_send_wal();
+ 

Re: buildfarm windows checks / tap tests on windows

[Andrew Dunstan]

On 3/3/21 7:21 PM, Andrew Dunstan wrote:
> On 3/3/21 5:32 PM, Andrew Dunstan wrote:
>> On 3/3/21 4:42 PM, Andres Freund wrote:
>>> Hi,
>>>
>>> On 2021-03-03 16:07:13 -0500, Andrew Dunstan wrote:
 Here's what I actually got working. Rip out the calls to kill_kill and
 replace them with:


 $psql_primary{stdin} .= "\\q\n";
 $psql_primary{run}->pump_nb();
 $psql_standby{stdin} .= "\\q\n";
 $psql_standby{run}->pump_nb();
 sleep 2; # give them time to quit


 No hang or signal now.
>>> Hm. I wonder if we can avoid the sleep 2 by doing something like
>>> ->pump(); ->finish(); instead of one pump_nb()? One pump() is needed to
>>> send the \q to psql, and then we need to wait for the process to finish?
>>> I'll try that, but given that I can't reproduce any problems...
>> Looking at the examples in the IPC:Run docco, it looks like we might
>> just be able to replace the pump_nb above with finish, and leave the
>> sleep out. I'll try that.
>
> OK, this worked fine. I'll try it s a recipe in the other places where
> kill_kill is forcing us to skip tests under MSwin32 perl, and see how we go.
>
>


Here's the patch. I didn't see a convenient way of handling the
pg_recvlogical case, so that's unchanged.


cheers


andrew

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

diff --git a/src/test/recovery/t/011_crash_recovery.pl b/src/test/recovery/t/011_crash_recovery.pl
index 5fe917978c..10cd98f70a 100644
--- a/src/test/recovery/t/011_crash_recovery.pl
+++ b/src/test/recovery/t/011_crash_recovery.pl
@@ -7,16 +7,8 @@ use PostgresNode;
 use TestLib;
 use Test::More;
 use Config;
-if ($Config{osname} eq 'MSWin32')
-{
 
-	# some Windows Perls at least don't like IPC::Run's start/kill_kill regime.
-	plan skip_all => "Test fails on Windows perl";
-}
-else
-{
-	plan tests => 3;
-}
+plan tests => 3;
 
 my $node = get_new_node('primary');
 $node->init(allows_streaming => 1);
@@ -65,4 +57,5 @@ cmp_ok($node->safe_psql('postgres', 'SELECT pg_current_xact_id()'),
 is($node->safe_psql('postgres', qq[SELECT pg_xact_status('$xid');]),
 	'aborted', 'xid is aborted after crash');
 
-$tx->kill_kill;
+$stdin .= "\\q\n";
+$tx->finish; # wait for psql to quit gracefully
diff --git a/src/test/recovery/t/021_row_visibility.pl b/src/test/recovery/t/021_row_visibility.pl
index b76990dfe0..f6a486bb88 100644
--- a/src/test/recovery/t/021_row_visibility.pl
+++ b/src/test/recovery/t/021_row_visibility.pl
@@ -151,9 +151,12 @@ ok(send_query_and_wait(\%psql_standby,
 	   qr/will_commit.*\n\(1 row\)$/m),
'finished prepared visible');
 
-# explicitly shut down psql instances - they cause hangs on windows
-$psql_primary{run}->kill_kill;
-$psql_standby{run}->kill_kill;
+# explicitly shut down psql instances gracefully - to avoid hangs
+# or worse on windows
+$psql_primary{stdin}  .= "\\q\n";
+$psql_primary{run}->finish;
+$psql_standby{stdin} .= "\\q\n";
+$psql_standby{run}->finish;
 
 $node_primary->stop;
 $node_standby->stop;

Re: Removing support for COPY FROM STDIN in protocol version 2

[Tom Lane]

Heikki Linnakangas  writes:
> On 04/03/2021 01:32, Tom Lane wrote:
>> I'm not sure where the extra newlines are coming from, and it seems
>> unlikely to be worth worrying over.  This behavior is good enough for me.

> fe-connect.c appends a newline for any errors in pre-3.0 format:

>>  /*
>>   * The postmaster typically won't end its message with a
>>   * newline, so add one to conform to libpq conventions.
>>   */
>>  appendPQExpBufferChar(&conn->errorMessage, '\n');

> That comment is wrong. The postmaster *does* end all its error messages 
> with a newline. This changed in commit 9b4bfbdc2c in 7.2.

Ah-hah, and the bit you show here came in with 2af360ed1, in 7.0.
I'm surprised though that we didn't notice that the newline was now
usually redundant.  This was a commonly taken code path until 7.4.

Anyway, your fix seems fine ... I wonder if we should back-patch it?

regards, tom lane

Re: Improvements and additions to COPY progress reporting

[Matthias van de Meent]

On Thu, 4 Mar 2021 at 13:36, Bharath Rupireddy
 wrote:
>
> +   
> +Each backend running VACUUM without the
> +FULL option will report its progress in the
> +pg_stat_progress_vacuum view. Backends running
> +VACUUM with the FULL option report
> +progress in the pg_stat_progress_cluster view
> +instead. See  and
> + for details.
> +   
>
> I think a typo, missing "will" between option and report - it's "with
> the FULL option will report"

"Backends running [...] report progress to [...] instead" is,
a.f.a.i.k., correct English. Adding 'will' would indeed still be
correct, but it would in my opinion also be decremental to the
readability of the section due to the repeated use of the same
template sentence structure. I think that keeping it as-is is just
fine.



With regards,

Matthias van de Meent.

Re: WIP: document the hook system

[David Steele]

On 3/4/21 10:00 AM, Peter Eisentraut wrote:

On 15.01.21 08:28, Peter Eisentraut wrote:

On 2020-12-31 04:28, David Fetter wrote:

This could probably use a lot of filling in, but having it in the
actual documentation beats needing to know folklore even to know
that the capability is there.


This patch seems quite short of a state where one could begin to 
evaluate it.  Documenting the hooks better seems a worthwhile goal.   
I think the question is whether we can develop documentation that is 
genuinely useful by itself without studying the relevant source code. 
This submission does not address that question.


There hasn't been any meaningful progress on this, and no new patch to 
look at, so I'm proposing to set this as returned with feedback.


+1. I'll close it on March 9 unless there are objections.

Regards,
--
-David
da...@pgmasters.net

Re: Improvements and additions to COPY progress reporting

[Justin Pryzby]

On Thu, Mar 04, 2021 at 05:19:18PM +0100, Matthias van de Meent wrote:
> On Thu, 4 Mar 2021 at 13:36, Bharath Rupireddy 
>  wrote:
> >
> > +   
> > +Each backend running VACUUM without the
> > +FULL option will report its progress in the
> > +pg_stat_progress_vacuum view. Backends running
> > +VACUUM with the FULL option 
> > report
> > +progress in the pg_stat_progress_cluster view
> > +instead. See  and
> > + for details.
> > +   
> >
> > I think a typo, missing "will" between option and report - it's "with
> > the FULL option will report"
> 
> "Backends running [...] report progress to [...] instead" is,
> a.f.a.i.k., correct English. Adding 'will' would indeed still be
> correct, but it would in my opinion also be decremental to the
> readability of the section due to the repeated use of the same
> template sentence structure. I think that keeping it as-is is just
> fine.

I'd prefer to see the same thing repeated, since then it's easy to compare, for
readers, and also future doc authors.  That's normal in technical documentation
to have redundancy.  It's easy to read.

I'd suggest to move "instead" into the middle of the sentence,
and combine VACUUM+FULL, and add "their":

> > +... Backends running VACUUM FULL will instead report
> > +their progress in the 
> > pg_stat_progress_cluster view.

-- 
Justin

Re: WIP: BRIN multi-range indexes

[John Naylor]

On Tue, Mar 2, 2021 at 7:32 PM Tomas Vondra 
wrote:
> Ummm, in brin_minmax_multi_distance_timetz()? I don't think timetz can
> be infinite, no? I think brin_minmax_multi_distance_date you meant?

In timestamp.c there are plenty of checks for TIMESTAMP_NOT_FINITE
for TimestampTz types and I assumed that was applicable here.

--
John Naylor
EDB: http://www.enterprisedb.com

Re: Confusing behavior of psql's \e

[Laurenz Albe]

On Wed, 2021-03-03 at 17:12 +, Jacob Champion wrote:
> On Wed, 2021-03-03 at 17:08 +0100, Laurenz Albe wrote:
> > This is no new behavior, and I think this is rare enough that we don't
> > have to bother.
> 
> I agree that it's not new behavior, but this patch exposes that
> behavior for the temporary file case, because you've fixed the bug that
> caused the timestamp check to not matter. As far as I can tell, you
> can't run into that race on the master branch for temporary files, and
> you won't run into it in practice for explicit filenames.

Actually, the timestamp check *did* matter before.
The code in "do_edit" has:

[after the editor has been called]
if (!error && before.st_mtime != after.st_mtime)
{
[read file back into query_buf]
}

This is pre-existing code.  I just added an else branch:

else
{
[discard query_buf if we were editing a script, function or view]
}

So if you do your "modify and save the file in less than a second"
trick with the old code, you would end up with the old, unmodified
data in the query buffer.

I would say that the old behavior is worse in that case, and
discarding the query buffer is better.

I am not against fixing or improving the behavior, but given the
fact that we can't portably get less than second precision, it
seems impossible.  For good measure, I have added a check if the
file size has changed.

I don't think we can or have to do better than that.
Few people are skilled enough to modify and save a file in less
than a second, and I don't think there have been complaints
about that behavior so far.

Attached is version 2 of the patch, with the added file size
check and a pgindent run.

Yours,
Laurenz Albe
From 8dc37cc9e775032f8f95cb837760dfe5463fc343 Mon Sep 17 00:00:00 2001
From: Laurenz Albe 
Date: Thu, 4 Mar 2021 17:33:59 +0100
Subject: [PATCH] Improve \e, \ef and \ev if the editor is quit

Before, quitting the editor without saving would cause
the *previous query* to be executed, which is certainly not
what anyone would expect.

It is better to execute nothing and clear the query buffer
in this case.

The exception is if the current query buffer is editied:
in this case, the behavior is unclanged, and the query buffer
is retained.

Author: Laurenz Albe 
Reviewed-by: Chapman Flack, Jacob Champion
Discussion: https://postgr.es/m/0ba3f2a658bac6546d9934ab6ba63a805d46a49b.camel%40cybertec.at
---
 doc/src/sgml/ref/psql-ref.sgml | 10 ---
 src/bin/psql/command.c | 49 +++---
 2 files changed, 41 insertions(+), 18 deletions(-)

diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index 13c1edfa4d..0bf69174ff 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -1970,7 +1970,9 @@ testdb=>
 
 
 
-The new contents of the query buffer are then re-parsed according to
+If you edit a file or the previous query, and you quit the editor without
+modifying the file, the query buffer is cleared.
+Otherwise, the new contents of the query buffer are re-parsed according to
 the normal rules of psql, treating the
 whole buffer as a single line.  Any complete queries are immediately
 executed; that is, if the query buffer contains or ends with a
@@ -2039,7 +2041,8 @@ Tue Oct 26 21:40:57 CEST 1999
  in the form of a CREATE OR REPLACE FUNCTION or
  CREATE OR REPLACE PROCEDURE command.
  Editing is done in the same way as for \edit.
- After the editor exits, the updated command is executed immediately
+ If you quit the editor without saving, the statement is discarded.
+ If you save and exit the editor, the updated command is executed immediately
  if you added a semicolon to it.  Otherwise it is redisplayed;
  type semicolon or \g to send it, or \r
  to cancel.
@@ -2115,7 +2118,8 @@ Tue Oct 26 21:40:57 CEST 1999
  This command fetches and edits the definition of the named view,
  in the form of a CREATE OR REPLACE VIEW command.
  Editing is done in the same way as for \edit.
- After the editor exits, the updated command is executed immediately
+ If you quit the editor without saving, the statement is discarded.
+ If you save and exit the editor, the updated command is executed immediately
  if you added a semicolon to it.  Otherwise it is redisplayed;
  type semicolon or \g to send it, or \r
  to cancel.
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index c98e3d31d0..2264ed5bab 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -151,7 +151,7 @@ static void copy_previous_query(PQExpBuffer query_buf, PQExpBuffer previous_buf)
 static bool do_connect(enum trivalue reuse_previous_specification,
 	   char *dbname, char *user, char *host, char *port);
 static bool do_edit(const char *filename_arg, PQExpBuffer query_buf,
-	int lineno, 

Re: [PATCH] regexp_positions ( string text, pattern text, flags text ) → setof int4range[]

[Chapman Flack]

On 03/04/21 10:40, Tom Lane wrote:
> Also, did you see
> 
> https://www.postgresql.org/message-id/fc160ee0-c843-b024-29bb-97b5da61971f%40darold.net
> 
> Seems like there may be some overlap in these proposals.

Not only that, the functions in that other proposal are very similar
to the standard's own functions that are specified to use XML Query
regular expression syntax (sample implementations in [1]).

These differently-named (which is good) functions seem to be a de facto
standard where the regexp syntax and semantics are those native to the
DBMS, the correspondence being

de facto  ISO XQuery-based
  -- --
  regexp_likelike_regex
  regexp_count   occurrences_regex
  regexp_instr   position_regex
  regexp_substr  substring_regex
  regexp_replace translate_regex


The regexp_positions proposal highlights an interesting apparent gap in
both the de facto and the ISO specs: the provided functions allow you
to specify which occurrence you're talking about, and get the corresponding
positions or the corresponding substring, but neither set of functions
includes one to just give you all the matching positions at once as
a SETOF something.

What the proposed regexp_positions() returns is pretty much exactly
the notional "list of match vectors" that appears internally throughout
the specs of the ISO functions, but is never directly exposed.

In the LOMV as described in the standard, the position/length arrays
are indexed from zero, and the start and length at index 0 are those
for the overall match as a whole.

Right now, if you have a query that involves, say,

  substring_regex('(b[^b]+)(b[^b]+)' IN str GROUP 1) and also
  substring_regex('(b[^b]+)(b[^b]+)' IN str GROUP 2),

a naïve implementation like [1] will of course compile and evaluate
the regexp twice and return one group each time. It makes me wonder
whether the standards committee was picturing a clever parse analyzer
and planner that would say "aha! you want group 1 and group 2 from
a single evaluation of this regex!", and that might even explain the
curious rule in the standard that the regex must be an actual literal,
not any other expression. (Still, that strikes me as an awkward way to
have to write it, spelling the regex out as a literal, twice.)

It has also made my idly wonder how close we could get to behaving
that way, perhaps with planner support functions and other available
parse analysis/planning hooks. Would any of those mechanisms get a
sufficiently global view of the query to do that kind of rewriting?

Regards,
-Chap


[1]
https://tada.github.io/pljava/pljava-examples/apidocs/org/postgresql/pljava/example/saxon/S9.html#method.summary

Re: [HACKERS] PATCH: Batch/pipelining support for libpq

[Ibrar Ahmed]

On Thu, Mar 4, 2021 at 8:01 PM Alvaro Herrera 
wrote:

> Apparently, the archives system or the commitfest system is not picking
> up new messages to the thread, so the CF app is trying to apply a
> very old patch version.  I'm not sure what's up with that.  Thomas, any
> clues on where to look?
>
> --
> Álvaro Herrera   Valdivia, Chile
> "Oh, great altar of passive entertainment, bestow upon me thy discordant
> images
> at such speed as to render linear thought impossible" (Calvin a la TV)
>

Hi Alvaro,

The thread splits and CF app still has the previous thread. The old thread
has the v18 as the latest patch which is failing. The new thread which  (
https://www.postgresql.org/message-id/CAJkzx4T5E-2cQe3dtv2R78dYFvz%2Bin8PY7A8MArvLhs_pg75gg%40mail.gmail.com
)
has the new patch.


-- 
Ibrar Ahmed

Re: Improvements and additions to COPY progress reporting

[Matthias van de Meent]

On Thu, 4 Mar 2021 at 17:29, Justin Pryzby  wrote:
>
> On Thu, Mar 04, 2021 at 05:19:18PM +0100, Matthias van de Meent wrote:
> >
> > "Backends running [...] report progress to [...] instead" is,
> > a.f.a.i.k., correct English. Adding 'will' would indeed still be
> > correct, but it would in my opinion also be decremental to the
> > readability of the section due to the repeated use of the same
> > template sentence structure. I think that keeping it as-is is just
> > fine.
>
> I'd prefer to see the same thing repeated, since then it's easy to compare, 
> for
> readers, and also future doc authors.  That's normal in technical 
> documentation
> to have redundancy.  It's easy to read.
>
> I'd suggest to move "instead" into the middle of the sentence,
> and combine VACUUM+FULL, and add "their":
>
> > > +... Backends running VACUUM FULL will instead 
> > > report
> > > +their progress in the 
> > > pg_stat_progress_cluster view.

Sure, I'm convinced. PFA the patchset with this change applied.

With regards,

Matthias van de Meent
From af85e442b5f61e145e6214d1fbfc4269af3bc9f1 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Josef=20=C5=A0im=C3=A1nek?= 
Date: Tue, 9 Feb 2021 13:06:45 +0100
Subject: [PATCH v12 3/3] Add copy progress reporting regression tests.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This tests some basic features of copy progress reporting.

Sadly, we can only request one snapshot of progress_reporting each
transaction / statement, so we can't (easily) get intermediate results without
each result requiring a seperate statement being run.

Author: Josef Šimánek, Matthias van de Meent
---
 src/test/regress/input/copy.source  | 60 +
 src/test/regress/output/copy.source | 47 ++
 2 files changed, 107 insertions(+)

diff --git a/src/test/regress/input/copy.source b/src/test/regress/input/copy.source
index a1d529ad36..4f1cbc73d2 100644
--- a/src/test/regress/input/copy.source
+++ b/src/test/regress/input/copy.source
@@ -201,3 +201,63 @@ select * from parted_copytest where b = 1;
 select * from parted_copytest where b = 2;
 
 drop table parted_copytest;
+
+--
+-- progress reporting
+--
+
+-- setup
+-- reuse employer datatype, that has a small sized data set
+
+create table progress_reporting (
+	name 		text,
+	age			int4,
+	location 	point,
+	salary 		int4,
+	manager 	name
+);
+
+-- Add a trigger to 'raise info' the contents of pg_stat_progress_copy. This
+-- allows us to test and verify the contents of this view, which would
+-- otherwise require a concurrent session checking that table.
+create function notice_after_progress_reporting() returns trigger AS
+$$
+declare report record;
+begin
+	-- We cannot expect 'pid' nor 'relid' to be consistent over runs due to
+	-- variance in system process ids, and concurrency in runs of tests.
+	-- Additionally, due to the usage of this test in pg_regress, the 'datid'
+	-- also is not consistent between runs.
+	select into report (to_jsonb(r) - '{pid,relid,datid}'::text[]) as value
+		from pg_stat_progress_copy r
+		where pid = pg_backend_pid();
+
+	raise info 'progress: %', report.value::text;
+
+	RETURN NEW;
+END;
+$$ LANGUAGE plpgsql;
+
+create trigger check_after_progress_reporting
+	after insert on progress_reporting
+	for each statement
+	execute function notice_after_progress_reporting();
+
+-- reporting of STDIO imports, and correct bytes-processed/tuples-processed reporting
+
+copy progress_reporting from stdin;
+sharon	25	(15,12)	1000	sam
+sam	30	(10,5)	2000	bill
+bill	20	(11,10)	1000	sharon
+\.
+
+-- reporting of FILE imports, and correct reporting of tuples-excluded
+
+copy progress_reporting from '@abs_srcdir@/data/emp.data'
+	where (salary < 2000);
+
+-- cleanup progress_reporting
+
+drop trigger check_after_progress_reporting on progress_reporting;
+drop function notice_after_progress_reporting();
+drop table progress_reporting;
diff --git a/src/test/regress/output/copy.source b/src/test/regress/output/copy.source
index 938d3551da..f3596bdd15 100644
--- a/src/test/regress/output/copy.source
+++ b/src/test/regress/output/copy.source
@@ -165,3 +165,50 @@ select * from parted_copytest where b = 2;
 (1 row)
 
 drop table parted_copytest;
+--
+-- progress reporting
+--
+-- setup
+-- reuse employer datatype, that has a small sized data set
+create table progress_reporting (
+	name 		text,
+	age			int4,
+	location 	point,
+	salary 		int4,
+	manager 	name
+);
+-- Add a trigger to 'raise info' the contents of pg_stat_progress_copy. This
+-- allows us to test and verify the contents of this view, which would
+-- otherwise require a concurrent session checking that table.
+create function notice_after_progress_reporting() returns trigger AS
+$$
+declare report record;
+begin
+	-- We cannot expect 'pid' nor 'relid' to be consistent over runs due to
+	-- variance in system process ids, and concurrency in runs of tests.
+	-- Additionally, due to the usage of this 

Re: [HACKERS] PATCH: Batch/pipelining support for libpq

[Justin Pryzby]

On Thu, Mar 04, 2021 at 12:01:37PM -0300, Alvaro Herrera wrote:
> Apparently, the archives system or the commitfest system is not picking
> up new messages to the thread, so the CF app is trying to apply a
> very old patch version.  I'm not sure what's up with that.  Thomas, any
> clues on where to look?

I think it's just because you forgot the patch.
https://www.postgresql.org/message-id/20210304142627.GA5978%40alvherre.pgsql

-- 
Justin

Re: [PATCH] postgres-fdw: column option to override foreign types

[David Steele]

On 3/4/21 9:28 AM, Georgios Kokolatos wrote:


I am afraid I will have to :-1: this patch. Of course it is possible that I am 
wrong,
so please correct me if you, or any other reviewers, think so.


I'm inclined to agree and it seems like a view on the source server is a 
good compromise and eliminates the maintenance concerns.


I'm going to mark this as Waiting on Author for now, but will close it 
on March 11 if there are no arguments in support.


Dian, perhaps you have another angle you'd like to try?

Regards,
--
-David
da...@pgmasters.net

Re: Confusing behavior of psql's \e

[Jacob Champion]

On Thu, 2021-03-04 at 17:41 +0100, Laurenz Albe wrote:
> So if you do your "modify and save the file in less than a second"
> trick with the old code, you would end up with the old, unmodified
> data in the query buffer.

Sorry, I was unclear in my first post -- I'm not modifying the
temporary file. Just saving and quitting with :wq, which is much easier
to do in less than a second.

> I would say that the old behavior is worse in that case, and
> discarding the query buffer is better.

For the case where you've modified the buffer, I agree, and I'm not
arguing otherwise.

> I am not against fixing or improving the behavior, but given the
> fact that we can't portably get less than second precision, it
> seems impossible.

You could backdate the temporary file, so that any save is guaranteed
to move the timestamp forward. That should work even if the filesystem
has extremely poor precision.

--Jacob

Re: [PATCH] regexp_positions ( string text, pattern text, flags text ) → setof int4range[]

[Joel Jacobson]

On Thu, Mar 4, 2021, at 16:40, Tom Lane wrote:
> "Joel Jacobson"  writes:
> > Having abandoned the cute idea that didn't work,
> > here comes a new patch with a regexp_positions() instead returning
> > setof record (start_pos integer[], end_pos integer[]).
> 
> I wonder if a 2-D integer array wouldn't be a better idea,
> ie {{startpos1,length1},{startpos2,length2},...}.  My experience
> with working with parallel arrays in SQL has been unpleasant.

I considered it, but I prefer two separate simple arrays for two reasons:

a) more pedagogic, it's at least then obvious what values are start and end 
positions,
then you only have to understand what the values means.

b) 2-D doesn't arrays don't work well with unnest().
If you would unnest() the 2-D array you couldn't separate the start positions 
from the end positions,
whereas with two separate, you could do:

SELECT unnest(start_pos) AS start_pos, unnest(end_pos) AS end_pos FROM 
regexp_positions('foobarbequebazilbarfbonk', $re$(b[^b]+)(b[^b]+)$re$, 'g');
start_pos | end_pos
---+-
 3 |   6
 6 |  11
11 |  16
16 |  20
(4 rows)

Can give some details on your unpleasant experiences of parallel arrays?


> 
> Also, did you see
> 
> https://www.postgresql.org/message-id/fc160ee0-c843-b024-29bb-97b5da61971f%40darold.net
> 
> 
> Seems like there may be some overlap in these proposals.

Yes, I saw it, it was sent shortly after my proposal, so I couldn't take it 
into account.
Seems useful, except regexp_instr() seems redundant, I would rather have 
regexp_positions(),
but maybe regexp_instr() should also be added for compatibility reasons.

/Joel

Re: [PATCH] regexp_positions ( string text, pattern text, flags text ) → setof int4range[]

[Gilles Darold]

Le 04/03/2021 à 16:40, Tom Lane a écrit :

"Joel Jacobson"  writes:

Having abandoned the cute idea that didn't work,
here comes a new patch with a regexp_positions() instead returning
setof record (start_pos integer[], end_pos integer[]).

I wonder if a 2-D integer array wouldn't be a better idea,
ie {{startpos1,length1},{startpos2,length2},...}.  My experience
with working with parallel arrays in SQL has been unpleasant.

Also, did you see

https://www.postgresql.org/message-id/fc160ee0-c843-b024-29bb-97b5da61971f%40darold.net

Seems like there may be some overlap in these proposals.



The object of regexp_position() is to return all start+end of captured 
substrings, it overlaps a little with regexp_instr() in the way that 
this function returns the start or end position of a specific captured 
substring. I think it is a good idea to have a function that returns all 
positions instead of a single one like regexp_instr(), this is not the 
same usage. Actually regexp_position() is exactly the same as 
regexp_matches() except that it return positions instead of substrings.



I also think that it should return a setof 2-D integer array, an other 
solution is to return all start/end positions of an occurrence chained 
in an integer array {start1,end1,start2,end2,..}.



Regards,

--
Gilles Darold

Re: [HACKERS] PATCH: Batch/pipelining support for libpq

[Alvaro Herrera]

> I think it's just because you forgot the patch.
> https://www.postgresql.org/message-id/20210304142627.GA5978%40alvherre.pgsql


-- 
Álvaro Herrera39°49'30"S 73°17'W
diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index 0553279314..c16befa314 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -3173,6 +3173,33 @@ ExecStatusType PQresultStatus(const PGresult *res);

   
  
+
+ 
+  PGRES_PIPELINE_SYNC
+  
+   
+The PGresult represents a
+synchronization point in pipeline mode, requested by 
+.
+This status occurs only when pipeline mode has been selected.
+   
+  
+ 
+
+ 
+  PGRES_PIPELINE_ABORTED
+  
+   
+The PGresult represents a pipeline that has
+received an error from the server.  PQgetResult
+must be called repeatedly, and each time it will return this status code
+until the end of the current pipeline, at which point it will return
+PGRES_PIPELINE_SYNC and normal processing can
+resume.
+   
+  
+ 
+
 
 
 If the result status is PGRES_TUPLES_OK or
@@ -4919,6 +4946,498 @@ int PQflush(PGconn *conn);
 
  
 
+ 
+  Pipeline Mode
+
+  
+   libpq
+   pipeline mode
+  
+
+  
+   pipelining
+   in libpq
+  
+
+  
+   batch mode
+   in libpq
+  
+
+  
+   libpq pipeline mode allows applications to
+   send a query without having to read the result of the previously
+   sent query.  Taking advantage of the pipeline mode, a client will wait
+   less for the server, since multiple queries/results can be sent/
+   received in a single network transaction.
+  
+
+  
+   While pipeline mode provides a significant performance boost, writing
+   clients using the pipeline mode is more complex because it involves
+   managing a queue of pending queries and finding which result
+   corresponds to which query in the queue.
+  
+
+  
+   Pipeline mode also generally consumes more memory on both the client and server,
+   though careful and aggressive management of the send/receive queue can mitigate
+   this.  This applies whether or not the connection is in blocking or non-blocking
+   mode.
+  
+
+  
+   Using Pipeline Mode
+
+   
+To issue pipelines, the application must switch a connection into pipeline mode.
+Enter pipeline mode with 
+or test whether pipeline mode is active with
+.
+In pipeline mode, only asynchronous operations
+are permitted, and COPY is disallowed.
+Using any synchronous command execution functions,
+such as PQfn, or PQexec
+and its sibling functions, is an error condition.
+   
+
+   
+
+ It is best to use pipeline mode with libpq in
+ non-blocking mode. If used
+ in blocking mode it is possible for a client/server deadlock to occur.
+  
+   
+The client will block trying to send queries to the server, but the
+server will block trying to send results to the client from queries
+it has already processed. This only occurs when the client sends
+enough queries to fill both its output buffer and the server's receive
+buffer before it switches to processing input from the server,
+but it's hard to predict exactly when that will happen.
+   
+  
+
+   
+
+   
+Issuing Queries
+
+
+ After entering pipeline mode, the application dispatches requests using
+ , 
+ or its prepared-query sibling
+ .
+ These requests are queued on the client-side until flushed to the server;
+ this occurs when  is used to
+ establish a synchronization point in the pipeline,
+ or when  is called.
+ The functions ,
+ , and
+  also work in pipeline mode.
+ Result processing is described below.
+
+
+
+ The server executes statements, and returns results, in the order the
+ client sends them.  The server will begin executing the commands in the
+ pipeline immediately, not waiting for the end of the pipeline.
+ If any statement encounters an error, the server aborts the current
+ transaction and skips processing commands in the pipeline until the
+ next synchronization point established by PQsendPipeline.
+ (This remains true even if the commands in the pipeline would rollback
+ the transaction.)
+ Query processing resumes after the synchronization point.
+
+
+
+ It's fine for one operation to depend on the results of a
+ prior one; for example, one query may define a table that the next
+ query in the same pipeline uses. Similarly, an application may
+ create a named prepared statement and execute it with later
+ statements in the same pipeline.
+
+   
+
+   
+Processing Results
+
+
+ To process the result of one query in a pipeline, the 

Re: [patch] bit XOR aggregate functions

[Ibrar Ahmed]

On Wed, Mar 3, 2021 at 7:30 PM Peter Eisentraut <
peter.eisentr...@enterprisedb.com> wrote:

> On 10.02.21 06:42, Kyotaro Horiguchi wrote:
> > We already had CREATE AGGREATE at the time, so BIT_XOR can be thought
> > as it falls into the same category with BIT_AND and BIT_OR, that is,
> > we may have BIT_XOR as an intrinsic aggregation?
>
> I think the use of BIT_XOR is quite separate from BIT_AND and BIT_OR.
> The latter give you an "all" or "any" result of the bits set.  BIT_XOR
> will return 1 or true if an odd number of inputs are 1 or true, which
> isn't useful by itself.  But it can be used as a checksum, so it seems
> pretty reasonable to me to add it.  Perhaps the use case could be
> pointed out in the documentation.
>
>
>
>
Hi Alex,


The patch is failing just because of a comment, which is already changed by
another patch

-/* Define to build with OpenSSL support. (--with-ssl=openssl) */

+/* Define to 1 if you have OpenSSL support. */

Do you mind sending an updated patch?

http://cfbot.cputube.org/patch_32_2980.log.

I am changing the status to "Waiting for Author"


In my opinion that change no more requires so I removed that and attached
the patch.

-- 
Ibrar Ahmed


bit-xor-agg-v002.diff
Description: Binary data

Re: pg_amcheck contrib application

[Robert Haas]

On Thu, Mar 4, 2021 at 10:29 AM Robert Haas  wrote:
> Most of these changes sound good. I'll go through the whole patch
> again today, or as much of it as I can. But before I do that, I want
> to comment on this point specifically.

Just a thought - I don't feel strongly about this - but you want want
to consider storing your list of patterns in an array that gets
resized as necessary rather than a list. Then the pattern ID would
just be pattern_ptr - pattern_array, and finding the pattern by ID
would just be pattern_ptr = &pattern_array[pattern_id]. I don't think
there's a real efficiency issue here because the list of patterns is
almost always going to be short, and even if somebody decides to
provide a very long list of patterns (e.g. by using xargs) it's
probably still not that big a deal. A sufficiently obstinate user
running an operating system where argument lists can be extremely long
could probably make this the dominant cost by providing a gigantic
number of patterns that don't match anything, but such a person is
trying to prove a point, rather than accomplish anything useful, so I
don't care. But, the code might be more elegant the other way.

This patch increases the number of cases where we use ^ to assert that
exactly one of two things is true from 4 to 5. I think it might be
better to just write out (a && !b) || (b && !a), but there is some
precedent for the way you did it so perhaps it's fine.

The name prepare_table_commmand() is oddly non-parallel with
verify_heapam_slot_handler(). Seems better to call it either a table
throughout, or a heapam throughout. Actually I think I would prefer
"heap" to either of those, but I definitely think we shouldn't switch
terminology. Note that prepare_btree_command() doesn't have this
issue, since it matches verify_btree_slot_handler(). On a related
note, "c.relam = 2" is really a test for is_heap, not is_table. We
might have other table AMs in the future, but only one of those AMs
will be called heap, and only one will have OID 2.

You've got some weird round-tripping stuff where you sent literal
values to the server so that you can turn around and get them back
from the server. For example, you've got prepare_table_command()
select rel->nspname and rel->relname back from the server as literals,
which seems silly because we have to already have that information or
we couldn't ask the server to give it to us ... and if we already have
it, then why do we need to get it again? The reason it's like this
seems to be that after calling prepare_table_command(), we use
ParallelSlotSetHandler() to set verify_heapam_slot_handler() as the
callback, and we set sql.data as the callback, so we don't have access
to the RelationInfo object when we're handling the slot result. But
that's easy to fix: just store the sql as a field inside the
RelationInfo, and then pass a pointer to the whole RelationInfo to the
slot handler. Then you don't need to round-trip the table and schema
names; and you have the values available even if an error happens.

On a somewhat related note, I think it might make sense to have the
slot handlers try to free memory. It seems hard to make pg_amcheck
leak enough memory to matter, but I guess it's not entirely
implausible that someone could be checking let's say 10 million
relations. Freeing the query strings could probably prevent a half a
GB or so of accumulated memory usage under those circumstances. I
suppose freeing nspname and relname would save a bit more, but it's
hardly worth doing since they are a lot shorter and you've got to have
all that information in memory at once at some point anyway; similarly
with the RelationInfo structures, which have the further complexity of
being part of a linked list you might not want to corrupt. But you
don't need to have every query string in memory at the same time, just
as many as are running at one in time.

Also, maybe compile_relation_list_one_db() should keep the result set
around so that you don't need to pstrdup() the nspname and relname in
the first place. Right now, just before compile_relation_list_one_db()
calls PQclear() you have two copies of every nspname and relname
allocated. If you just kept the result sets around forever, the peak
memory usage would be lower than it is currently. If you really wanted
to get fancy you could arrange to free each result set when you've
finished that database, but that seems annoying to code and I'm pretty
sure it doesn't matter.

The CTEs called "include_raw" and "exclude_raw" which are used as part
of the query to construct a list of tables. The regexes are fished
through there, and the pattern IDs, which makes sense, but the raw
patterns are also fished through, and I don't see a reason for that.
We don't seem to need that for anything. The same seems to apply to
the query used to resolve database patterns.

I see that most of the queries have now been adjusted to be spread
across fewer lines, which is good, but please make sure to do that
everywhe

Re: Corruption during WAL replay

[Ibrar Ahmed]

On Wed, Jan 6, 2021 at 1:33 PM Kyotaro Horiguchi 
wrote:

> At Mon, 17 Aug 2020 11:22:15 -0700, Andres Freund 
> wrote in
> > Hi,
> >
> > On 2020-08-17 14:05:37 +0300, Heikki Linnakangas wrote:
> > > On 14/04/2020 22:04, Teja Mupparti wrote:
> > > > Thanks Kyotaro and Masahiko for the feedback. I think there is a
> > > > consensus on the critical-section around truncate,
> > >
> > > +1
> >
> > I'm inclined to think that we should do that independent of the far more
> > complicated fix for other related issues.
> ...
> > > Perhaps a better approach would be to prevent the checkpoint from
> > > completing, until all in-progress truncations have completed. We have a
> > > mechanism to wait out in-progress commits at the beginning of a
> checkpoint,
> > > right after the redo point has been established. See comments around
> the
> > > GetVirtualXIDsDelayingChkpt() function call in CreateCheckPoint(). We
> could
> > > have a similar mechanism to wait out the truncations before
> *completing* a
> > > checkpoint.
> >
> > What I outlined earlier *is* essentially a way to do so, by preventing
> > checkpointing from finishing the buffer scan while a dangerous state
> > exists.
>
> Seems reasonable. The attached does that. It actually works for the
> initial case.
>
> regards.
>
> --
> Kyotaro Horiguchi
> NTT Open Source Software Center
>

The regression is failing for this patch, do you mind look at that and send
the updated patch?

https://api.cirrus-ci.com/v1/task/6313174510075904/logs/test.log

...
t/006_logical_decoding.pl  ok
t/007_sync_rep.pl  ok
Bailout called.  Further testing stopped:  system pg_ctl failed
FAILED--Further testing stopped: system pg_ctl failed
make[2]: *** [Makefile:19: check] Error 255
make[1]: *** [Makefile:49: check-recovery-recurse] Error 2
make: *** [GNUmakefile:71: check-world-src/test-recurse] Error 2
...

-- 
Ibrar Ahmed

Re: Add support for PROVE_FLAGS and PROVE_TESTS in MSVC scripts

[Juan José Santamaría Flecha]

On Thu, Mar 4, 2021 at 3:11 AM Michael Paquier  wrote:

> On Wed, Mar 03, 2021 at 08:59:30PM +0100, Juan José Santamaría Flecha
> wrote:
> > This seems to me as if setting the variables in the shell is the proposed
> > way to do so. In the previous doc point we do the same with the
> buildenv.pl
> > file. It looks inconsistent, as if it was one way or the other, when it
> > could be either.
>
> Okay, that makes sense.  PROVE_TESTS is a runtime-only dependency so
> my guess is that most people would set that directly on the command
> prompt like I do, still it can be useful to enforce that in build.pl.
> PROVE_FLAGS can be both, but you are right that setting it in build.pl
> would be the most common approach.  So let's document both.  Attached
> is a proposal for that, what do you think?  I have extended the
> example of PROVE_FLAGS to show how to set up more --jobs.
>

LGTM.

Regards,

Juan José Santamaría Flecha

Re: Shared memory size computation oversight?

[Julien Rouhaud]

On Thu, Mar 04, 2021 at 08:43:51AM +, Georgios wrote:
> 
> ‐‐‐ Original Message ‐‐‐
> On Thursday, March 4, 2021 9:21 AM, Michael Paquier  
> wrote:
> 
> > On Thu, Mar 04, 2021 at 03:23:33PM +0800, Julien Rouhaud wrote:
> >
> > > I was also considering adding new (add|mull)_*_size functions to avoid 
> > > having
> > > too messy code. I'm not terribly happy with xxx_shm_size(), as not all 
> > > call to
> > > those functions would require an alignment. Maybe 
> > > (add|mull)shmemalign_size?
> > > But before modifying dozens of calls, should we really fix those or only
> > > increase a bit the "slop factor", or a mix of it?
> > > For instance, I can see that for instance BackendStatusShmemSize() never 
> > > had
> > > any padding consideration, while others do.
> > > Maybe only fixing contribs, some macro like PredXactListDataSize that 
> > > already
> > > do a MAXALIGN, SimpleLruShmemSize and hash_estimate_size() would be a 
> > > short
> > > patch and should significantly improve the estimation.
> >
> > The lack of complaints in this area looks to me like a sign that we
> > may not really need to backpatch something, so I would not be against
> > a precise chirurgy, with a separate set of {add,mul}_size() routines
> > that are used where adapted, so as it is easy to track down which size
> > estimations expect an extra padding. I would be curious to hear more
> > thoughts from others here.
> >
> > Saying that, calling a new routine something like add_shmem_align_size
> > makes it clear what's its purpose.
> 
> My limited opinion on the matter after spending some time yesterday through
> the related code, is that with the current api it is easy to miss something
> and underestimate or just be sloppy. If only from the readability point of
> view, adding the proposed add_shmem_align_size will be beneficial.
> 
> I hold no opinion on backpatching.

So here's a v2 patch which hopefully account for all unaccounted alignment
padding.  There's also a significant change in the shared hash table size
estimation.  AFAICT, allocation will be done this way:

- num_freelists allocs of init_size / num_freelists entries (on average) for
  partitioned htab, num_freelist being 1 for non partitioned table,
  NUM_FREELISTS (32) otherwise.

- then the rest of the entries, if any, are alloced in groups of
  choose_nelem_alloc() entries

With this patch applied, I see an extra 16KB of shared memory being requested.
>From ddc03fbbebb8486d10b0fb0718bf6e4ed1291759 Mon Sep 17 00:00:00 2001
From: Julien Rouhaud 
Date: Sat, 27 Feb 2021 15:45:29 +0800
Subject: [PATCH v2] Fix various shared memory estimates.

---
 contrib/pg_prewarm/autoprewarm.c  |  2 +-
 .../pg_stat_statements/pg_stat_statements.c   |  2 +-
 src/backend/access/common/syncscan.c  |  2 +-
 src/backend/access/nbtree/nbtutils.c  |  3 +-
 src/backend/access/transam/multixact.c|  2 +-
 src/backend/access/transam/slru.c |  2 +-
 src/backend/access/transam/twophase.c |  2 +-
 src/backend/access/transam/xlog.c |  2 +-
 src/backend/commands/async.c  |  1 +
 src/backend/postmaster/autovacuum.c   |  3 +-
 src/backend/postmaster/bgworker.c |  2 +-
 src/backend/postmaster/checkpointer.c |  2 +-
 src/backend/postmaster/pgstat.c   | 17 ++---
 src/backend/postmaster/postmaster.c   |  2 +-
 src/backend/replication/logical/launcher.c|  3 +-
 src/backend/replication/logical/origin.c  |  3 +-
 src/backend/replication/slot.c|  2 +-
 src/backend/replication/walreceiverfuncs.c|  2 +-
 src/backend/replication/walsender.c   |  2 +-
 src/backend/storage/buffer/buf_init.c | 14 ++--
 src/backend/storage/buffer/buf_table.c|  3 +-
 src/backend/storage/buffer/freelist.c |  2 +-
 src/backend/storage/ipc/dsm.c |  2 +-
 src/backend/storage/ipc/pmsignal.c|  2 +-
 src/backend/storage/ipc/procarray.c   |  8 +--
 src/backend/storage/ipc/procsignal.c  |  2 +-
 src/backend/storage/ipc/sinvaladt.c   |  2 +-
 src/backend/storage/lmgr/lock.c   | 16 -
 src/backend/storage/lmgr/lwlock.c |  2 +-
 src/backend/storage/lmgr/predicate.c  | 22 +++---
 src/backend/storage/lmgr/proc.c   | 12 ++--
 src/backend/utils/hash/dynahash.c | 69 ++-
 src/backend/utils/time/snapmgr.c  |  2 +-
 src/include/storage/predicate_internals.h |  4 +-
 src/include/storage/shmem.h   | 13 
 src/include/utils/hsearch.h   |  2 +
 36 files changed, 146 insertions(+), 87 deletions(-)

diff --git a/contrib/pg_prewarm/autoprewarm.c b/contrib/pg_prewarm/autoprewarm.c
index b3f73ea92d..887e68b288 100644
--- a/contrib/pg_prewarm/autoprewarm.c
+++ b/contrib/pg_prewarm/autoprewarm.c
@@ -138,7 +138,7 @@ _PG_init(void)
 
 	EmitWarningsOnPlaceholders("pg_prewarm");
 
-	RequestA

Re: [Patch] ALTER SYSTEM READ ONLY

[Amul Sul]

On Wed, Mar 3, 2021 at 7:56 PM Dilip Kumar  wrote:
>
> On Wed, Mar 3, 2021 at 4:50 PM Amul Sul  wrote:
> >
> > On Wed, Mar 3, 2021 at 12:08 PM Dilip Kumar  wrote:
> > >
> > Yes, it is possible to allow wal temporarily for itself by setting
> > LocalXLogInsertAllowed, but when we request Checkpointer for the 
> > end-of-recovery
> > checkpoint, the first thing it will do is that wal prohibit state transition
> > then recovery-end-checkpoint.
> >
> > Also, allowing WAL write in read-only (WAL prohibited state) mode is against
> > this feature principle.
>
> So IIUC before the checkpoint change the state in the control file we
> anyway inform other backend and then they are allowed to write the WAL
> is the right?  If that is true then what is the problem in first doing
> the pending post-recovery process and then informing the backend about
> the state change.  I mean we are in a process of changing the state to
> read-write so why it is necessary to inform all the backend before we
> can write WAL?  Are we afraid that after we write the WAL and if there
> is some failure before we make the system read-write then it will
> break the principle of the feature, I mean eventually system will stay
> read-only but we wrote the WAL?  If so then currently, are we assuring
> that once we inform the backend and backend are allowed to write the
> WAL there are no chances of failure and the system state is guaranteed
> to be changed.  If all that is true then I will take my point back.
>

The wal prohibit state transition handling code is integrated into various
places of the checkpointer process so that it can pick state changes as soon as
possible. Before informing other backends we can do UpdateFullPageWrites() but
when we try to next the end-of-recovery checkpoint write operation, the
Checkpointer will hit ProcessWALProhibitStateChangeRequest() first which will
try for the wal prohibit transition state completion and then  write
the checkpoint
record.

Regards,
Amul

Re: Shared memory size computation oversight?

[Tom Lane]

I spent a bit of time looking into this.  Using current HEAD, I
instrumented CreateSharedMemoryAndSemaphores to log the size estimates
returned by the various estimation subroutines, plus the shared memory
space actually consumed (i.e, the change in ShmemSegHdr->freeoffset)
by the various shared memory initialization functions.  There were only
two estimates that were way off: LockShmemSize requested 1651771 more
bytes than InitLocks actually consumed, and PredicateLockShmemSize
requested 167058 more bytes than InitPredicateLocks consumed.  I believe
both of those are intentional, reflecting space that may be eaten by the
lock tables later.

Meanwhile, looking at ShmemSegHdr->freeoffset vs ShmemSegHdr->totalsize,
the actual remaining shmem space after postmaster startup is 1919488
bytes.  (Running the core regression tests doesn't consume any of that
remaining space, btw.)  Subtracting the extra lock-table space, we have
100659 bytes to spare, which is as near as makes no difference to the
intended slop of 10 bytes.

My conclusion is that we don't need to do anything, indeed the proposed
changes will probably just lead to overestimation.

It's certainly possible that there's something amiss somewhere.  These
numbers were all taken with out-of-the-box configuration, so it could be
that changing some postgresql.conf entries would expose that some module
is not scaling its request correctly.  Also, I don't have any extensions
loaded, so this proves nothing about the correctness of any of those.
But it appears to me that the general scheme is working perfectly fine,
so we do not need to complicate it.

regards, tom lane

ApplyLauncherShmemInit  used512
ApplyLauncherShmemSize  est 424
AsyncShmemInit  used69376
AsyncShmemSize  est 69308
AutoVacuumShmemInit used5376
AutoVacuumShmemSize est 5368
BTreeShmemInit  used1536
BTreeShmemSize  est 1476
CreateSharedBackendStatus   used203520
BackendStatusShmemSize  est 203304
BackgroundWorkerShmemInit   used4608
BackgroundWorkerShmemSize   est 4496
InitBufferPool  used137047040
BufferShmemSize est 137044560
CLOGShmemInit   used529152
CLOGShmemSize   est 529152
CheckpointerShmemInit   used393344
CheckpointerShmemSize   est 393280
CommitTsShmemInit   used133760
CommitTsShmemSize   est 133600
CreateLWLocks   used26880
LWLockShmemSize est 26756
InitLocks   used1394560
LockShmemSize   est 3046331 <-- 1651771 not consumed
MultiXactShmemInit  used201600
MultiXactShmemSize  est 201412
PGReserveSemaphores used16128
PGSemaphoreShmemSizeest 16128
PMSignalShmemInit   used1024
PMSignalShmemSize   est 1024
InitPredicateLocks  used2396160
PredicateLockShmemSize  est 2563218 <-- 167058 not consumed
CreateSharedProcArray   used40320
ProcArrayShmemSize  est 40178
InitProcGlobal  used112128
ProcGlobalShmemSize est 111883
ProcSignalShmemInit used9344
ProcSignalShmemSize est 9296
ReplicationOriginShmemInit  used640
ReplicationOriginShmemSize  est 568
ReplicationSlotsShmemInit   used2688
ReplicationSlotsShmemSize   est 2640
InitShmemIndex  used10624
SHMEM_INDEX hashest 13040
CreateSharedInvalidationState   used69504
SInvalShmemSize est 69464
SUBTRANSShmemInit   used267008
SUBTRANSShmemSize   est 267008
SnapMgrInit used128
SnapMgrShmemSizeest 68
SyncScanShmemInit   used768
SyncScanShmemSize   est 656
TwoPhaseShmemInit   used128
TwoPhaseShmemSize   est 16
WalRcvShmemInit used2304
WalRcvShmemSize est 2248
WalSndShmemInit used1152
WalSndShmemSize est 1040
XLOGShmemInit   used4208768
XLOGShmemSize   est 4208280
dsm_postmaster_startup  used0
dsm_estimate_size   est 0
dsm_shmem_init  used0
final roundup   est 3602
total_addin_request est 0

Re: buildfarm windows checks / tap tests on windows

[Andres Freund]

Hi,

On 2021-03-04 11:10:19 -0500, Andrew Dunstan wrote:
> Here's the patch.

Awesome. Will you commit it?


> I didn't see a convenient way of handling the pg_recvlogical case, so
> that's unchanged.

Is the problem actually the kill_kill() itself, or just doing
->kill_kill() without a subsequent ->finish()?

But anyway, that seems like a less critical test...

Greetings,

Andres Freund

Re: [PATCH] Support empty ranges with bounds information

[Joel Jacobson]

On Thu, Mar 4, 2021, at 16:21, Isaac Morland wrote:
> On Thu, 4 Mar 2021 at 01:25, Joel Jacobson  wrote:
> 
>> __
>> Suggestion #1: Use [] as the canonical form for discrete types.
>> This would allow creating ranges for all values for discrete types.
> 
> I won't reiterate here, but there are fundamental reasons why [) is 
> definitely the right default and canonical form.

It would be interesting to hear the reasons.

For discrete types, there are only exact values, there is nothing in between 
two adjacent discrete values.
So if we mean a range covering only the integer 5, why can't we just say [5,5] 
which simply means "5"?
Why is it necessary to express it as [5,6) which I interpret as the much more 
complex "all integers from 5 up until just before the integer 6".

We know for sure nothing can exist after 5 before 6, it's void, so why is it 
necessary to be explicit about including this space which we know can't contain 
any values?

For discrete types, we wouldn't even need the inclusive/exclusive features at 
all.

> 
> In any case, you can create a range containing the last value:
> 
> odyssey=> select 2147483647 <@ int4range (0, null);
>  ?column? 
> --
>  t
> (1 row)
> 
> odyssey=> 
> 
> It does seem reasonable to me to change it so that specifying the last value 
> as the right end with ] would use a null endpoint instead of producing an 
> error when it tries to increment the bound.

Neat hack, thanks.

/Joel

Re: Increase value of OUTER_VAR

[Tom Lane]

Tomas Vondra  writes:
> On 3/4/21 4:16 PM, Tom Lane wrote:
>> Hmm, do we really have any places that include OUTER_VAR etc in
>> bitmapsets?  They shouldn't appear in relid sets, for sure.
>> I agree though that if they did, this would have bad performance
>> consequences.

> Hmmm, I don't know. I mostly assumed that if I do pull_varnos() it would
> include those values. But maybe that's not supposed to happen.

But (IIRC) those varnos are never used till setrefs.c fixes up the plan
to replace normal Vars with references to lower plan nodes' outputs.
I'm not sure why anyone would be doing pull_varnos() after that;
it would not give very meaningful results.

regards, tom lane

Re: [PATCH] postgres-fdw: column option to override foreign types

[Georgios Kokolatos]

The following review has been posted through the commitfest application:
make installcheck-world:  not tested
Implements feature:   not tested
Spec compliant:   not tested
Documentation:not tested

Hi,

Thanks for the patch.

I am afraid I will have to :-1: this patch. Of course it is possible that I am 
wrong,
so please correct me if you, or any other reviewers, think so.

The problem that is intended to be solved, upon closer inspection seems to be a
conscious design decision rather than a problem. Even if I am wrong there, I am
not certain that the proposed patch covers all the bases with respect to 
collations,
build-in types, shipability etc for simple expressions, and covers any of more
complicated expressions all together. 

As for the scenario which prompted the patch, you wrote, quote:

The real scenario that prompted it is a
tickets table with status, priority, category, etc. enums. We don't have
plans to modify them any time soon, but if we do it's got to be
coordinated and deployed across two databases, all so we can use what
might as well be a text column in a single WHERE clause. Since foreign
tables can be defined over subsets of columns, reordered, and names
changed, a little opt-in flexibility with types too doesn't seem
misplaced. 

end quote.

I will add that creating a view on the remote server with type flexibility that
you wish and then create foreign tables against the view, might address your
problem.

Respectfully,
//Georgios

Re: buildfarm windows checks / tap tests on windows

[Andrew Dunstan]

On 3/4/21 12:56 PM, Andres Freund wrote:
> Hi,
>
> On 2021-03-04 11:10:19 -0500, Andrew Dunstan wrote:
>> Here's the patch.
> Awesome. Will you commit it?



Done


>
>
>> I didn't see a convenient way of handling the pg_recvlogical case, so
>> that's unchanged.
> Is the problem actually the kill_kill() itself, or just doing
> ->kill_kill() without a subsequent ->finish()?


Pretty sure it's the kill_kill that causes the awful crash Michael and I
have seen.



>
> But anyway, that seems like a less critical test...



right


cheers


andrew

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

Re: Increase value of OUTER_VAR

[Tom Lane]

Just as a proof of concept, I tried the attached, and it passes
check-world.  So if there's anyplace trying to stuff OUTER_VAR and
friends into bitmapsets, it's pretty far off the beaten track.

The main loose ends that'd have to be settled seem to be:

(1) What data type do we want Var.varno to be declared as?  In the
previous thread, Robert opined that plain "int" isn't a good choice,
but I'm not sure I agree.  There's enough "int" for rangetable indexes
all over the place that it'd be a fool's errand to try to make it
uniformly something different.

(2) Does that datatype change need to propagate anywhere besides
what I touched here?  I did not make any effort to search for
other places.

regards, tom lane

diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index 8fc432bfe1..d44d84804e 100644
--- a/src/backend/nodes/outfuncs.c
+++ b/src/backend/nodes/outfuncs.c
@@ -1098,7 +1098,7 @@ _outVar(StringInfo str, const Var *node)
 {
 	WRITE_NODE_TYPE("VAR");
 
-	WRITE_UINT_FIELD(varno);
+	WRITE_INT_FIELD(varno);
 	WRITE_INT_FIELD(varattno);
 	WRITE_OID_FIELD(vartype);
 	WRITE_INT_FIELD(vartypmod);
diff --git a/src/backend/nodes/readfuncs.c b/src/backend/nodes/readfuncs.c
index 718fb58e86..ff94c10b8d 100644
--- a/src/backend/nodes/readfuncs.c
+++ b/src/backend/nodes/readfuncs.c
@@ -575,7 +575,7 @@ _readVar(void)
 {
 	READ_LOCALS(Var);
 
-	READ_UINT_FIELD(varno);
+	READ_INT_FIELD(varno);
 	READ_INT_FIELD(varattno);
 	READ_OID_FIELD(vartype);
 	READ_INT_FIELD(vartypmod);
diff --git a/src/backend/optimizer/plan/setrefs.c b/src/backend/optimizer/plan/setrefs.c
index 42f088ad71..f9267d329e 100644
--- a/src/backend/optimizer/plan/setrefs.c
+++ b/src/backend/optimizer/plan/setrefs.c
@@ -467,16 +467,6 @@ add_rte_to_flat_rtable(PlannerGlobal *glob, RangeTblEntry *rte)
 
 	glob->finalrtable = lappend(glob->finalrtable, newrte);
 
-	/*
-	 * Check for RT index overflow; it's very unlikely, but if it did happen,
-	 * the executor would get confused by varnos that match the special varno
-	 * values.
-	 */
-	if (IS_SPECIAL_VARNO(list_length(glob->finalrtable)))
-		ereport(ERROR,
-(errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
- errmsg("too many range table entries")));
-
 	/*
 	 * If it's a plain relation RTE, add the table to relationOids.
 	 *
diff --git a/src/include/nodes/primnodes.h b/src/include/nodes/primnodes.h
index d4ce037088..43d8100424 100644
--- a/src/include/nodes/primnodes.h
+++ b/src/include/nodes/primnodes.h
@@ -168,11 +168,11 @@ typedef struct Expr
  * in the planner and doesn't correspond to any simple relation column may
  * have varnosyn = varattnosyn = 0.
  */
-#defineINNER_VAR		65000	/* reference to inner subplan */
-#defineOUTER_VAR		65001	/* reference to outer subplan */
-#defineINDEX_VAR		65002	/* reference to index column */
+#defineINNER_VAR		(-1)	/* reference to inner subplan */
+#defineOUTER_VAR		(-2)	/* reference to outer subplan */
+#defineINDEX_VAR		(-3)	/* reference to index column */
 
-#define IS_SPECIAL_VARNO(varno)		((varno) >= INNER_VAR)
+#define IS_SPECIAL_VARNO(varno)		((varno) < 0)
 
 /* Symbols for the indexes of the special RTE entries in rules */
 #definePRS2_OLD_VARNO			1
@@ -181,7 +181,7 @@ typedef struct Expr
 typedef struct Var
 {
 	Expr		xpr;
-	Index		varno;			/* index of this var's relation in the range
+	int			varno;			/* index of this var's relation in the range
  * table, or INNER_VAR/OUTER_VAR/INDEX_VAR */
 	AttrNumber	varattno;		/* attribute number of this var, or zero for
  * all attrs ("whole-row Var") */

CI/windows docker vs "am a service" autodetection on windows

[Andres Freund]

Hi,

When one tests postgres in a some of the popular CI systems (all that
use docker for windows), some of the tests fail in weird ways. Like

https://www.postgresql.org/message-id/20210303052011.ybplxw6q4tafwogk%40alap3.anarazel.de

> t/003_recovery_targets.pl  7/9
> #   Failed test 'multiple conflicting settings'
> #   at t/003_recovery_targets.pl line 151.
> 
> #   Failed test 'recovery end before target reached is a fatal error'
> #   at t/003_recovery_targets.pl line 177.
> t/003_recovery_targets.pl  9/9 # Looks like you failed 2 tests of 
> 9.
> t/003_recovery_targets.pl  Dubious, test returned 2 (wstat 512, 
> 0x200)
> Failed 2/9 subtests
> 
> I think it's pretty dangerous if we have a substantial number of tests
> that aren't run on windows - I think a lot of us just assume that the
> BF would catch windows specific problems...

A lot of debugging later I figured out that the problem is that postgres
decides not to write anything to stderr, but send everything to the
windows event log instead.  This includes error messages when starting
postgres with wrong parameters or such...

The reason for that elog.c and pg_ctl.c use
src/port/win32security.c:pgwin32_is_service() to detect whether they're
running as a service:

static void
send_message_to_server_log(ErrorData *edata)
...
/*
 * In a win32 service environment, there is no usable stderr. 
Capture
 * anything going there and write it to the eventlog instead.
 *
 * If stderr redirection is active, it was OK to write to 
stderr above
 * because that's really a pipe to the syslogger process.
 */
else if (pgwin32_is_service())
write_eventlog(edata->elevel, buf.data, buf.len);
..
void
write_stderr(const char *fmt,...)
...
/*
 * On Win32, we print to stderr if running on a console, or write to
 * eventlog if running as a service
 */
if (pgwin32_is_service())   /* Running as a service */
{
write_eventlog(ERROR, errbuf, strlen(errbuf));


but pgwin32_is_service() doesn't actually reliably detect if running as
a service - it's a heuristic that also triggers when running postgres
within a windows docker container (presumably because that itself is run
from within a service?).


ISTM that that's a problem, and is likely to become more of a problem
going forward (assuming that docker on windows will become more
popular).


My opinion is that the whole attempt at guessing whether we are running
as a service is a bad idea. This isn't the first time to be a problem,
see e.g. [1].

Why don't we instead have pgwin32_doRegister() include a parameter that
indicates we're running as a service and remove all the heuristics?


I tried to look around to see if there's a simple way to drop the
problematic memberships that trigger pgwin32_is_service() - but there
seem to be no commandline tools doing so (but there are C APIs).

Does anybody have an alternative way of fixing this?


Greetings,

Andres Freund

[1]
commit ff30aec759bdc4de78912d91f650ec8fd95ff6bc
Author: Heikki Linnakangas 
Date:   2017-03-17 11:14:01 +0200

Fix and simplify check for whether we're running as Windows service.

Re: Allow matching whole DN from a client certificate

[Joel Jacobson]

On Sat, Jan 30, 2021, at 22:18, Andrew Dunstan wrote:
> ssl-match-client-cert-dn-v3.patch

Spelling error of "conjunction":
+   This option is probably best used in comjunction with a username map. 

/Joel

Add .log file ending to tap test log files

[Andres Freund]

Hi,

Right now it's harder than necessary to capture the log output from tap
tests because the the regression tests files don't end with a common
file ending with other types of logs.  They're
# Open the test log file, whose name depends on the test name.
$test_logfile = basename($0);
$test_logfile =~ s/\.[^.]+$//;
$test_logfile = "$log_path/regress_log_$test_logfile";

This was essentially introduced in 1ea06203b82: "Improve logging of TAP tests."

Would anybody object to replacing _logfile with .log? I realize that'd
potentially would cause some short-term pain on the buildfarm, but I
think it'd improve things longer term.

Greetings,

Andres Freund

Re: [HACKERS] PATCH: Batch/pipelining support for libpq

[Alvaro Herrera]

v30 contains changes to hopefully make it build on MSVC.

-- 
Álvaro Herrera   Valdivia, Chile
diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index 0553279314..c16befa314 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -3173,6 +3173,33 @@ ExecStatusType PQresultStatus(const PGresult *res);

   
  
+
+ 
+  PGRES_PIPELINE_SYNC
+  
+   
+The PGresult represents a
+synchronization point in pipeline mode, requested by 
+.
+This status occurs only when pipeline mode has been selected.
+   
+  
+ 
+
+ 
+  PGRES_PIPELINE_ABORTED
+  
+   
+The PGresult represents a pipeline that has
+received an error from the server.  PQgetResult
+must be called repeatedly, and each time it will return this status code
+until the end of the current pipeline, at which point it will return
+PGRES_PIPELINE_SYNC and normal processing can
+resume.
+   
+  
+ 
+
 
 
 If the result status is PGRES_TUPLES_OK or
@@ -4919,6 +4946,498 @@ int PQflush(PGconn *conn);
 
  
 
+ 
+  Pipeline Mode
+
+  
+   libpq
+   pipeline mode
+  
+
+  
+   pipelining
+   in libpq
+  
+
+  
+   batch mode
+   in libpq
+  
+
+  
+   libpq pipeline mode allows applications to
+   send a query without having to read the result of the previously
+   sent query.  Taking advantage of the pipeline mode, a client will wait
+   less for the server, since multiple queries/results can be sent/
+   received in a single network transaction.
+  
+
+  
+   While pipeline mode provides a significant performance boost, writing
+   clients using the pipeline mode is more complex because it involves
+   managing a queue of pending queries and finding which result
+   corresponds to which query in the queue.
+  
+
+  
+   Pipeline mode also generally consumes more memory on both the client and server,
+   though careful and aggressive management of the send/receive queue can mitigate
+   this.  This applies whether or not the connection is in blocking or non-blocking
+   mode.
+  
+
+  
+   Using Pipeline Mode
+
+   
+To issue pipelines, the application must switch a connection into pipeline mode.
+Enter pipeline mode with 
+or test whether pipeline mode is active with
+.
+In pipeline mode, only asynchronous operations
+are permitted, and COPY is disallowed.
+Using any synchronous command execution functions,
+such as PQfn, or PQexec
+and its sibling functions, is an error condition.
+   
+
+   
+
+ It is best to use pipeline mode with libpq in
+ non-blocking mode. If used
+ in blocking mode it is possible for a client/server deadlock to occur.
+  
+   
+The client will block trying to send queries to the server, but the
+server will block trying to send results to the client from queries
+it has already processed. This only occurs when the client sends
+enough queries to fill both its output buffer and the server's receive
+buffer before it switches to processing input from the server,
+but it's hard to predict exactly when that will happen.
+   
+  
+
+   
+
+   
+Issuing Queries
+
+
+ After entering pipeline mode, the application dispatches requests using
+ , 
+ or its prepared-query sibling
+ .
+ These requests are queued on the client-side until flushed to the server;
+ this occurs when  is used to
+ establish a synchronization point in the pipeline,
+ or when  is called.
+ The functions ,
+ , and
+  also work in pipeline mode.
+ Result processing is described below.
+
+
+
+ The server executes statements, and returns results, in the order the
+ client sends them.  The server will begin executing the commands in the
+ pipeline immediately, not waiting for the end of the pipeline.
+ If any statement encounters an error, the server aborts the current
+ transaction and skips processing commands in the pipeline until the
+ next synchronization point established by PQsendPipeline.
+ (This remains true even if the commands in the pipeline would rollback
+ the transaction.)
+ Query processing resumes after the synchronization point.
+
+
+
+ It's fine for one operation to depend on the results of a
+ prior one; for example, one query may define a table that the next
+ query in the same pipeline uses. Similarly, an application may
+ create a named prepared statement and execute it with later
+ statements in the same pipeline.
+
+   
+
+   
+Processing Results
+
+
+ To process the result of one query in a pipeline, the application calls
+ PQgetResult repeatedly and handles each result
+ until PQgetResult re

Re: [PATCH] postgres-fdw: column option to override foreign types

[Dian M Fay]

On Thu Mar 4, 2021 at 9:28 AM EST, Georgios Kokolatos wrote:
> I am afraid I will have to :-1: this patch. Of course it is possible
> that I am wrong,
> so please correct me if you, or any other reviewers, think so.
>
> The problem that is intended to be solved, upon closer inspection
> seems
> to be a
> conscious design decision rather than a problem. Even if I am wrong
> there, I am
> not certain that the proposed patch covers all the bases with respect
> to
> collations,
> build-in types, shipability etc for simple expressions, and covers any
> of more
> complicated expressions all together.

Thanks for reviewing it!

I see room for interpretation in the design here, although I have
admittedly not been looking at it for very long. `CREATE/ALTER FOREIGN
TABLE` take the user at their word about types, which only map 1:1 for a
foreign Postgres server anyway. In make_tuple_from_result_row, incoming
values start as strings until they're converted to their target types --
again, with no guarantee that those types match those on the remote
server. The docs recommend types match exactly and note the sorts of
things that can go wrong, but there's no enforcement; either what you've
cooked up works or it doesn't. And in fact, declaring local text for a
remote enum seems to work quite well right up until you try to
reference it in the `WHERE` clause.

Enum::text seems like a safe and potentially standardizable case for
postgres_fdw. As implemented, the patch goes beyond that, but it's
opt-in and the docs already warn about consequences. I haven't tested it
across collations, but right now that seems like something to look into
if the idea survives the next few messages.

> I will add that creating a view on the remote server with type
> flexibility that
> you wish and then create foreign tables against the view, might
> address
> your
> problem.

A view would address the immediate issue of the types, but itself
requires additional maintenance if/when the underlying table's schema
changes (even `SELECT *` is expanded into the current column definitions
at creation). I think it's better than copying the types, because it
moves the extra work of keeping local and remote synchronized to a
*table* modification as opposed to a *type* modification, in which
latter case it's much easier to forget about dependents. But I'd prefer
to avoid extra work anywhere!

Re: CI/windows docker vs "am a service" autodetection on windows

[Andrew Dunstan]

On 3/4/21 2:08 PM, Andres Freund wrote:
> [...] pgwin32_is_service() doesn't actually reliably detect if running as
> a service - it's a heuristic that also triggers when running postgres
> within a windows docker container (presumably because that itself is run
> from within a service?).
>
>
> ISTM that that's a problem, and is likely to become more of a problem
> going forward (assuming that docker on windows will become more
> popular).
>
>
> My opinion is that the whole attempt at guessing whether we are running
> as a service is a bad idea. This isn't the first time to be a problem,
> see e.g. [1].
>
> Why don't we instead have pgwin32_doRegister() include a parameter that
> indicates we're running as a service and remove all the heuristics?



I assume you mean a postmaster parameter, that would be set via pg_ctl?
Seems reasonable.



cheers


andrew

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

Re: PROXY protocol support

[Jacob Champion]

On Thu, 2021-03-04 at 10:42 +0900, Tatsuo Ishii wrote:
> Is there any formal specification for the "a protocol common and very
> light weight in proxies"?

See

https://www.haproxy.org/download/1.8/doc/proxy-protocol.txt

which is maintained by HAProxy Technologies.

--Jacob

Re: [HACKERS] PATCH: Batch/pipelining support for libpq

[Alvaro Herrera]

On 2021-Mar-04, Alvaro Herrera wrote:

> v30 contains changes to hopefully make it build on MSVC.

Hm, that didn't work -- appveyor still says:

Project "C:\projects\postgresql\pgsql.sln" (1) is building 
"C:\projects\postgresql\pipeline.vcxproj" (75) on node 1 (default targets).
PrepareForBuild:
  Creating directory ".\Release\pipeline\".
  Creating directory ".\Release\pipeline\pipeline.tlog\".
InitializeBuildStatus:
  Creating ".\Release\pipeline\pipeline.tlog\unsuccessfulbuild" because 
"AlwaysCreate" was specified.
ClCompile:
  C:\Program Files (x86)\Microsoft Visual Studio 12.0\VC\bin\x86_amd64\CL.exe 
/c /Isrc/include /Isrc/include/port/win32 /Isrc/include/port/win32_msvc /Zi 
/nologo /W3 /WX- /Ox /D WIN32 /D _WINDOWS /D __WINDOWS__ /D __WIN32__ /D 
WIN32_STACK_RLIMIT=4194304 /D _CRT_SECURE_NO_DEPRECATE /D 
_CRT_NONSTDC_NO_DEPRECATE /D _MBCS /GF /Gm- /EHsc /MD /GS /fp:precise 
/Zc:wchar_t /Zc:forScope /Fo".\Release\pipeline\\" 
/Fd".\Release\pipeline\vc120.pdb" /Gd /TC /wd4018 /wd4244 /wd4273 /wd4102 
/wd4090 /wd4267 /errorReport:queue /MP src/test/modules/test_libpq/pipeline.c
  pipeline.c
src/test/modules/test_libpq/pipeline.c(11): fatal error C1083: Cannot open 
include file: 'libpq-fe.h': No such file or directory 
[C:\projects\postgresql\pipeline.vcxproj]
Done Building Project "C:\projects\postgresql\pipeline.vcxproj" (default 
targets) -- FAILED.
Project "C:\projects\postgresql\pgsql.sln" (1) is building 
"C:\projects\postgresql\test_parser.vcxproj" (76) on node 1 (default targets).

I think the problem is that the project is called pipeline and not test_libpq,
so there's no match in the name.  I'm going to rename the whole thing to
src/test/modules/libpq_pipeline/ and see if the msvc tooling likes that
better.


-- 
Álvaro Herrera39°49'30"S 73°17'W

Re: [HACKERS] PATCH: Batch/pipelining support for libpq

[Alvaro Herrera]

On 2021-Mar-04, Alvaro Herrera wrote:

> I think the problem is that the project is called pipeline and not
> test_libpq, so there's no match in the name.  I'm going to rename the
> whole thing to src/test/modules/libpq_pipeline/ and see if the msvc
> tooling likes that better.

v31.

-- 
Álvaro Herrera   Valdivia, Chile
"I'm impressed how quickly you are fixing this obscure issue. I came from 
MS SQL and it would be hard for me to put into words how much of a better job
you all are doing on [PostgreSQL]."
 Steve Midgley, http://archives.postgresql.org/pgsql-sql/2008-08/msg0.php
diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index 0553279314..c87b0ce911 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -3173,6 +3173,33 @@ ExecStatusType PQresultStatus(const PGresult *res);

   
  
+
+ 
+  PGRES_PIPELINE_SYNC
+  
+   
+The PGresult represents a
+synchronization point in pipeline mode, requested by 
+.
+This status occurs only when pipeline mode has been selected.
+   
+  
+ 
+
+ 
+  PGRES_PIPELINE_ABORTED
+  
+   
+The PGresult represents a pipeline that has
+received an error from the server.  PQgetResult
+must be called repeatedly, and each time it will return this status code
+until the end of the current pipeline, at which point it will return
+PGRES_PIPELINE_SYNC and normal processing can
+resume.
+   
+  
+ 
+
 
 
 If the result status is PGRES_TUPLES_OK or
@@ -4919,6 +4946,498 @@ int PQflush(PGconn *conn);
 
  
 
+ 
+  Pipeline Mode
+
+  
+   libpq
+   pipeline mode
+  
+
+  
+   pipelining
+   in libpq
+  
+
+  
+   batch mode
+   in libpq
+  
+
+  
+   libpq pipeline mode allows applications to
+   send a query without having to read the result of the previously
+   sent query.  Taking advantage of the pipeline mode, a client will wait
+   less for the server, since multiple queries/results can be sent/
+   received in a single network transaction.
+  
+
+  
+   While pipeline mode provides a significant performance boost, writing
+   clients using the pipeline mode is more complex because it involves
+   managing a queue of pending queries and finding which result
+   corresponds to which query in the queue.
+  
+
+  
+   Pipeline mode also generally consumes more memory on both the client and server,
+   though careful and aggressive management of the send/receive queue can mitigate
+   this.  This applies whether or not the connection is in blocking or non-blocking
+   mode.
+  
+
+  
+   Using Pipeline Mode
+
+   
+To issue pipelines, the application must switch a connection into pipeline mode.
+Enter pipeline mode with 
+or test whether pipeline mode is active with
+.
+In pipeline mode, only asynchronous operations
+are permitted, and COPY is disallowed.
+Using any synchronous command execution functions,
+such as PQfn, or PQexec
+and its sibling functions, is an error condition.
+   
+
+   
+
+ It is best to use pipeline mode with libpq in
+ non-blocking mode. If used
+ in blocking mode it is possible for a client/server deadlock to occur.
+  
+   
+The client will block trying to send queries to the server, but the
+server will block trying to send results to the client from queries
+it has already processed. This only occurs when the client sends
+enough queries to fill both its output buffer and the server's receive
+buffer before it switches to processing input from the server,
+but it's hard to predict exactly when that will happen.
+   
+  
+
+   
+
+   
+Issuing Queries
+
+
+ After entering pipeline mode, the application dispatches requests using
+ , 
+ or its prepared-query sibling
+ .
+ These requests are queued on the client-side until flushed to the server;
+ this occurs when  is used to
+ establish a synchronization point in the pipeline,
+ or when  is called.
+ The functions ,
+ , and
+  also work in pipeline mode.
+ Result processing is described below.
+
+
+
+ The server executes statements, and returns results, in the order the
+ client sends them.  The server will begin executing the commands in the
+ pipeline immediately, not waiting for the end of the pipeline.
+ If any statement encounters an error, the server aborts the current
+ transaction and skips processing commands in the pipeline until the
+ next synchronization point established by PQsendPipeline.
+ (This remains true even if the commands in the pipeline would rollback
+ the transaction.)
+ Query processing resumes after the synchronization point.
+
+
+
+ It's fine for one

Re: Removing support for COPY FROM STDIN in protocol version 2

[Tom Lane]

Heikki Linnakangas  writes:
>> I concur that 0001 attached is committable.  I have not looked at
>> your 0002, though.

> Removed the entry from nls.mk, and pushed 0001. Thanks!

It seems that buildfarm member walleye doesn't like this.
Since nothing else is complaining, I confess bafflement
as to why.  walleye seems to be our only active mingw animal,
so maybe there's a platform dependency somewhere ... but
how would (mostly) removal of code expose that?

regards, tom lane