Re: Logical replication - schema change not invalidating the relation cache

[Dilip Kumar]

On Fri, Jul 2, 2021 at 12:03 PM Dilip Kumar  wrote:
>
> Yeah, this looks like a bug.  I will look at the patch.
>

While looking into this, I think the main cause of the problem is that
schema rename does not invalidate the relation cache right?  I also
tried other cases e.g. if there is an open cursor and we rename the
schema

CREATE SCHEMA sch1;
CREATE TABLE sch1.t1(c1 int);
insert into sch1.t1 values(1);
insert into sch1.t1 values(2);
insert into sch1.t1 values(3);
BEGIN;
DECLARE mycur CURSOR FOR SELECT * FROM sch1.t1;
FETCH NEXT FROM mycur ;
--At this point rename sch1 to sch2 from another session--
FETCH NEXT FROM mycur ;
UPDATE sch2.t1 SET c1 = 20 WHERE CURRENT OF mycur;
select * from sch2.t1 ;

So even after the schema rename the cursor is able to fetch and its
also able to update on the same table in the new schema, ideally using
CURRENT OF CUR, you can update the same table for which you have
declared the cursor.  I am giving this example because this behavior
also looks somewhat similar.

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

Re: logical replication worker accesses catalogs in error context callback

[Bharath Rupireddy]

On Sat, Jul 3, 2021 at 2:19 AM Tom Lane  wrote:
>
> I wrote:
> > Didn't look at 0002 yet.
>
> ... and now that I have, I don't like it much.  It adds a lot of
> complexity, plus some lookup cycles that might be wasted.  I experimented
> with looking into the range table as I suggested previously, and
> that seems to work; see attached.  (This includes a little bit of
> code cleanup along with the bug fix proper.)
>
> An interesting point here is that the range table data will represent
> table and column aliases, not necessarily their true names.  I don't
> find that wrong, it's just different from what the code presently
> does.  If we go with this, likely we should change the plain-relation
> code path so that it also prints aliases from the RTE instead of
> the actual names.

Thanks. This patch is way simpler than what I proposed. Also, I like
the generic message "processing expression at position %d in select
list" when relname or attname is not available.

The patch basically looks good to me except a minor comment to have a
local variable for var->varattno which makes the code shorter.
if (IsA(tle->expr, Var))
{
-   RangeTblEntry *rte;
Var*var = (Var *) tle->expr;
+   AttrNumber  attno = var->varattno;
+   RangeTblEntry *rte = exec_rt_fetch(var->varno, estate);

-   rte = exec_rt_fetch(var->varno, estate);
+   relname = rte->eref->aliasname;

-   if (var->varattno == 0)
+   if (attno == 0)
is_wholerow = true;
-   else
-   attname = get_attname(rte->relid,
var->varattno, false);
-
-   relname = get_rel_name(rte->relid);
+   else if (attno > 0 && attno <=
list_length(rte->eref->colnames))
+   attname =
strVal(list_nth(rte->eref->colnames, attno - 1));
}

Regards,
Bharath Rupireddy.

Re: logical replication worker accesses catalogs in error context callback

[Bharath Rupireddy]

On Sat, Jul 3, 2021 at 1:37 AM Tom Lane  wrote:
>
> Bharath Rupireddy  writes:
> >> << Attaching v5-0001 here again for completion >>
> >> I'm attaching v5-0001 patch that avoids printing the column type names
> >> in the context message thus no cache lookups have to be done in the
> >> error context callback. I think the column name is enough to know on
> >> which column the error occurred and if required it's type can be known
> >> by the user. This patch gets rid of printing local and remote type
> >> names in slot_store_error_callback and also
> >> logicalrep_typmap_gettypname because it's unnecessary. Thoughts?
>
> I've now pushed this patch.  I noted that once we drop
> logicalrep_typmap_gettypname, there's really nothing using
> the LogicalRepTypMap table at all, so I nuked that too.
> (We can always recover the code from the git archives if
> some reason to use it emerges.)

Isn't it better to have the below comment before
slot_store_error_callback, similar to what's there before
conversion_error_callback in v7-0002.
 * Note that this function mustn't do any catalog lookups, since we are in
 * an already-failed transaction.

Maybe it's worth considering
avoid_sys_cache_lookup_in_error_callback_v2.patch from [1] as another
way to enforce the above statement.

[1] - 
https://www.postgresql.org/message-id/CAD21AoAwxbd-zXXUAeJ2FBRHr%2B%3DbfMUHoN7xJuXcwu1sFi1-sQ%40mail.gmail.com

Regards,
Bharath Rupireddy.

Re: Preventing abort() and exit() calls in libpq

[Noah Misch]

On Fri, Jul 02, 2021 at 11:20:17AM -0400, Tom Lane wrote:
> Alvaro Herrera  writes:
> > Now it's hoverfly:
> > ! nm -A -u libpq.so.5 2>/dev/null | grep -v __cxa_atexit | grep exit
> > libpq.so.5: atexit   U   -
> > libpq.so.5: pthread_exit U   -
> 
> Ugh.  What in the world is producing those references?

Those come from a statically-linked libldap_r:

$ nm -A -u /home/nm/sw/nopath/openldap-64/lib/libldap_r.a|grep exit
/home/nm/sw/nopath/openldap-64/lib/libldap_r.a[tpool.o]: .ldap_pvt_thread_exit 
U   -
/home/nm/sw/nopath/openldap-64/lib/libldap_r.a[thr_posix.o]: .pthread_exit  
  U   -
/home/nm/sw/nopath/openldap-64/lib/libldap_r.a[init.o]: .atexit  U  
 -

Re: Preventing abort() and exit() calls in libpq

[Tom Lane]

"alvhe...@alvh.no-ip.org"  writes:
> gcc here is 8.3.0.

Hmmm ... mine is 8.4.1.

I'm about to go out to dinner, but will check into this with some
newer gcc versions later.

regards, tom lane

Re: Numeric multiplication overflow errors

[Dean Rasheed]

On Fri, 2 Jul 2021 at 12:56, David Rowley  wrote:
>
> On Fri, 2 Jul 2021 at 22:55, Dean Rasheed  wrote:
> > Here's an update with the
> > last set of changes discussed.
>
> Looks good to me.

Thanks for the review and testing!

> Just the question of if we have any problems changing the serialized
> format in the back branches.  I'm not sure if that's something we've
> done before. I only had a quick look of git blame in the
> serial/deserial functions and the only changes I really see apart from
> a few cosmetic ones were a57d312a7 and 9cca11c91. Both of which just
> went into master.

Thinking about this more, I think it's best not to risk back-patching.
It *might* be safe, but it's difficult to really be sure of that. The
bug itself is pretty unlikely to ever happen in practice, hence the
lack of prior complaints, and in fact I only found it by an
examination of the code. So it doesn't seem to be worth the risk.

OTOH, the original bug, with numeric *, is one I have hit in practice,
and the fix is trivial and low risk, so I would like to backpatch that
fix.

Regards,
Dean

Re: Preventing abort() and exit() calls in libpq

[alvhe...@alvh.no-ip.org]

On 2021-Jul-02, Jacob Champion wrote:

> Only src/common:
> 
> controldata_utils_shlib.o:
>  U close
>  U __errno_location
>  U exit

Actually, I do see these in the .o files as well, but they don't make it
to the .a file.

gcc here is 8.3.0.

-- 
Álvaro Herrera PostgreSQL Developer  —  https://www.EnterpriseDB.com/

Re: Preventing abort() and exit() calls in libpq

[Jacob Champion]

On Fri, 2021-07-02 at 18:45 -0400, Tom Lane wrote:
> Jacob Champion  writes:
> > On Fri, 2021-07-02 at 18:20 -0400, Tom Lane wrote:
> > > What configure options are you using?
> > Just `./configure --enable-coverage`, nothing else. I distclean'd right
> > before for good measure.
> 
> Hmph.  There's *something* different about your setup from what
> either Alvaro or I tried.  What's the compiler (and version)?
> What's the platform exactly?

$ gcc --version
gcc (Ubuntu 9.3.0-17ubuntu1~20.04) 9.3.0
Copyright (C) 2019 Free Software Foundation, Inc.
...

$ cat /etc/os-release
NAME="Ubuntu"
VERSION="20.04.2 LTS (Focal Fossa)"
ID=ubuntu
ID_LIKE=debian
PRETTY_NAME="Ubuntu 20.04.2 LTS"
VERSION_ID="20.04"
...

$ uname -a
Linux HOSTNAME 5.8.0-59-generic #66~20.04.1-Ubuntu SMP Thu Jun 17 11:14:10 
UTC 2021 x86_64 x86_64 x86_64 GNU/Linux

--Jacob

Re: Preventing abort() and exit() calls in libpq

[Tom Lane]

Jacob Champion  writes:
> On Fri, 2021-07-02 at 18:20 -0400, Tom Lane wrote:
>> What configure options are you using?

> Just `./configure --enable-coverage`, nothing else. I distclean'd right
> before for good measure.

Hmph.  There's *something* different about your setup from what
either Alvaro or I tried.  What's the compiler (and version)?
What's the platform exactly?

regards, tom lane

Re: Preventing abort() and exit() calls in libpq

[Jacob Champion]

On Fri, 2021-07-02 at 18:20 -0400, Tom Lane wrote:
> What configure options are you using?

Just `./configure --enable-coverage`, nothing else. I distclean'd right
before for good measure.

> Does "nm -u" report "exit" being referenced from any *.o in libpq,
> or from any *_shlib.o in src/port/ or src/common/ ?

Only src/common:

controldata_utils_shlib.o:
 U close
 U __errno_location
 U exit
...
fe_memutils_shlib.o:
 U exit
...
file_utils_shlib.o:
 U close
 U closedir
 U __errno_location
 U exit
...
hex_shlib.o:
 U exit
...
psprintf_shlib.o:
 U __errno_location
 U exit
...
stringinfo_shlib.o:
 U __errno_location
 U exit
...
username_shlib.o:
 U __errno_location
 U exit
...

--Jacob

Re: Preventing abort() and exit() calls in libpq

[Tom Lane]

Jacob Champion  writes:
> With latest HEAD, building with --enable-coverage still fails on my
> Ubuntu 20.04:

> ! nm -A -u libpq.so.5.15 2>/dev/null | grep -v __cxa_atexit | grep exit
> libpq.so.5.15: U exit@@GLIBC_2.2.5

Hm, weird.  I don't see that here on RHEL8, and 
https://coverage.postgresql.org seems to be working so it doesn't fail on
Alvaro's Debian setup either.  What configure options are you using?
Does "nm -u" report "exit" being referenced from any *.o in libpq,
or from any *_shlib.o in src/port/ or src/common/ ?

regards, tom lane

Re: Preventing abort() and exit() calls in libpq

[Jacob Champion]

On Wed, 2021-06-30 at 10:42 -0400, Tom Lane wrote:
> Alvaro Herrera  writes:
> > On 2021-Jun-30, Tom Lane wrote:
> > > You mentioned __gcov_exit, but I'm not sure if we need an
> > > exception for that.  I see it referenced by the individual .o
> > > files, but the completed .so has no such reference, so at least
> > > on RHEL8 it's apparently satisfied during .so linkage.  Do you
> > > see something different?
> > Well, not really.  I saw it but only after I removed -fprofile-arcs from
> > Makefile.shlib's link line; but per my other email, that doesn't really
> > work.
> > Everything seems to work well for me after removing abort from that grep.
> 
> OK, thanks, will push a fix momentarily.

With latest HEAD, building with --enable-coverage still fails on my
Ubuntu 20.04:

! nm -A -u libpq.so.5.15 2>/dev/null | grep -v __cxa_atexit | grep exit
libpq.so.5.15: U exit@@GLIBC_2.2.5

I don't see any exit references in the libpq objects or in
libpgport_shlib, so it seems like libpgcommon_shlib is the culprit... I
assume turning off optimizations leads to less dead code elimination?

--Jacob

Re: rand48 replacement

[Fabien COELHO]

Hello Dean,

It may be true that the bias is of the same magnitude as FP multiply, 
but it is not of the same nature. With FP multiply, the 
more-likely-to-be-chosen values are more-or-less evenly distributed 
across the range, whereas modulo concentrates them all at one end, 
making it more likely to bias test results.


Yes, that is true.


It's worth paying attention to how other languages/libraries implement
this, and basically no one chooses the modulo method, which ought to
raise alarm bells. Of the biased methods, it has the worst kind of
bias and the worst performance.


Hmmm. That is not exactly how I interpreted the figures in the blog.


If a biased method is OK, then the biased integer multiply method
seems to be the clear winner. This requires the high part of a
64x64-bit product, which is trivial if 128-bit integers are available,
but would need a little more work otherwise. There's some code in
common/d2s that might be suitable.


And yes, modulo is expensive. If we allow 128 bits integers operations, I 
would not choose this RNPG in the first place, I'd take PCG with a 128 bits state.

That does not change the discussion about bias, though.

Most other implementations tend to use an unbiased method though, and I 
think it's worth doing the same. It might be a bit slower, or even 
faster depending on implementation and platform, but in the context of 
the DB as a whole, I don't think a few extra cycles matters either way.


Ok ok ok, I surrender!

The method recommended at the very end of that blog seems to be pretty 
good all round, but any of the other commonly used unbiased methods 
would probably be OK too.


That does not address my other issues with the proposed methods, in 
particular the fact that the generated sequence is less deterministic, but 
I think I have a simple way around that. I'm hesitating to skip to the the 
bitmask method, and give up performance uniformity. I'll try to come up 
with something over the week-end, and also address Tom's comments in 
passing.


--
Fabien.

Re: psql - factor out echo code

[Fabien COELHO]

 "-- #  QUERY\n%s\n\n"


Attached an attempt along those lines. I found another duplicate of the 
ascii-art printing in another function.


Completion queries seems to be out of the echo/echo hidden feature.

Incredible, there is a (small) impact on regression tests for the \gexec 
case. All other changes have no impact, because they are not tested:-(


--
Fabien.diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 543401c6d6..8ec00881db 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -2061,7 +2061,7 @@ exec_command_password(PsqlScanState scan_state, bool active_branch)
 printfPQExpBuffer(&buf, "ALTER USER %s PASSWORD ",
   fmtId(user));
 appendStringLiteralConn(&buf, encrypted_password, pset.db);
-res = PSQLexec(buf.data);
+res = PSQLexec("PASSWORD", buf.data);
 termPQExpBuffer(&buf);
 if (!res)
 	success = false;
@@ -4993,22 +4993,13 @@ do_watch(PQExpBuffer query_buf, double sleep)
  * returns true unless we have ECHO_HIDDEN_NOEXEC.
  */
 static bool
-echo_hidden_command(const char *query)
+echo_hidden_command(const char *origin, const char *query)
 {
 	if (pset.echo_hidden != PSQL_ECHO_HIDDEN_OFF)
 	{
-		printf(_("* QUERY **\n"
- "%s\n"
- "**\n\n"), query);
-		fflush(stdout);
+		echoQuery(stdout, origin, query);
 		if (pset.logfile)
-		{
-			fprintf(pset.logfile,
-	_("* QUERY **\n"
-	  "%s\n"
-	  "**\n\n"), query);
-			fflush(pset.logfile);
-		}
+			echoQuery(pset.logfile, origin, query);
 
 		if (pset.echo_hidden == PSQL_ECHO_HIDDEN_NOEXEC)
 			return false;
@@ -5060,7 +5051,7 @@ lookup_object_oid(EditableObjectType obj_type, const char *desc,
 			break;
 	}
 
-	if (!echo_hidden_command(query->data))
+	if (!echo_hidden_command("INTERNAL", query->data))
 	{
 		destroyPQExpBuffer(query);
 		return false;
@@ -5155,7 +5146,7 @@ get_create_object_cmd(EditableObjectType obj_type, Oid oid,
 			break;
 	}
 
-	if (!echo_hidden_command(query->data))
+	if (!echo_hidden_command("INTERNAL", query->data))
 	{
 		destroyPQExpBuffer(query);
 		return false;
diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
index 9a00499510..69d4e33093 100644
--- a/src/bin/psql/common.c
+++ b/src/bin/psql/common.c
@@ -523,6 +523,17 @@ PrintTiming(double elapsed_msec)
 		   elapsed_msec, days, (int) hours, (int) minutes, seconds);
 }
 
+/*
+ * Echo user query
+ */
+void
+echoQuery(FILE *out, const char *origin, const char *query)
+{
+	fprintf(out,
+			/* FIXME should this really be translatable? */
+			_("-- # %s QUERY\n%s\n\n"), origin, query);
+	fflush(out);
+}
 
 /*
  * PSQLexec
@@ -537,7 +548,7 @@ PrintTiming(double elapsed_msec)
  * caller uses this path to issue "SET CLIENT_ENCODING".
  */
 PGresult *
-PSQLexec(const char *query)
+PSQLexec(const char *origin, const char *query)
 {
 	PGresult   *res;
 
@@ -549,18 +560,9 @@ PSQLexec(const char *query)
 
 	if (pset.echo_hidden != PSQL_ECHO_HIDDEN_OFF)
 	{
-		printf(_("* QUERY **\n"
- "%s\n"
- "**\n\n"), query);
-		fflush(stdout);
+		echoQuery(stdout, origin, query);
 		if (pset.logfile)
-		{
-			fprintf(pset.logfile,
-	_("* QUERY **\n"
-	  "%s\n"
-	  "**\n\n"), query);
-			fflush(pset.logfile);
-		}
+			echoQuery(pset.logfile, origin, query);
 
 		if (pset.echo_hidden == PSQL_ECHO_HIDDEN_NOEXEC)
 			return NULL;
@@ -856,10 +858,7 @@ ExecQueryTuples(const PGresult *result)
  * assumes that MainLoop did that, so we have to do it here.
  */
 if (pset.echo == PSQL_ECHO_ALL && !pset.singlestep)
-{
-	puts(query);
-	fflush(stdout);
-}
+	echoQuery(stdout, "GEXEC", query);
 
 if (!SendQuery(query))
 {
@@ -1226,13 +1225,7 @@ SendQuery(const char *query)
 	}
 
 	if (pset.logfile)
-	{
-		fprintf(pset.logfile,
-_("* QUERY **\n"
-  "%s\n"
-  "**\n\n"), query);
-		fflush(pset.logfile);
-	}
+		echoQuery(pset.logfile, "USER", query);
 
 	SetCancelConn(pset.db);
 
diff --git a/src/bin/psql/common.h b/src/bin/psql/common.h
index 041b2ac068..cdad55414a 100644
--- a/src/bin/psql/common.h
+++ b/src/bin/psql/common.h
@@ -28,9 +28,10 @@ extern sigjmp_buf sigint_interrupt_jmp;
 
 extern void psql_setup_cancel_handler(void);
 
-extern PGresult *PSQLexec(const char *query);
+extern PGresult *PSQLexec(const char *origin, const char *query);
 extern int	PSQLexecWatch(const char *query, const printQueryOpt *opt);
 
+extern void echoQuery(FILE *out, const char *origin, const char *query);
 extern bool SendQuery(const char *query);
 
 extern bool is_superuser(void);
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index 2abf255798..b7f701a68a 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -127,7 +127,7 @@ describeAggregates(const char *pattern, bool verbose, bool showSystem)
 
 	appendPQExpBufferStr(&buf, "ORDER

Re: psql - factor out echo code

[Tom Lane]

Alvaro Herrera  writes:
> I think the most interesting case for decoration is the "step by step"
> mode, where you want the "title" that precedes each query be easily
> visible.

I'm okay with leaving the step-by-step prompt as-is, personally.
It's the inconsistency of the other ones that bugs me.

> Also: one place that prints queries that wasn't mentioned before is
> exec_command_print() in command.c.

Ah, I was wondering if anyplace outside common.c did so.  But that
one seems to me to be a different animal -- it's not logging
queries-about-to-be-executed.

regards, tom lane

Re: Fix uninitialized variable access (src/backend/utils/mmgr/freepage.c)

[Ranier Vilela]

Em qui., 1 de jul. de 2021 às 17:20, Mahendra Singh Thalor <
mahi6...@gmail.com> escreveu:

> On Fri, 2 Jul 2021 at 01:13, Ranier Vilela  wrote:
> >
> > Hi,
> >
> > The function FreePageManagerPutInternal can access an uninitialized
> variable,
> > if the following conditions occur:
>
> Patch looks good to me.
>
> > 1. fpm->btree_depth != 0
> > 2. relptr_off == 0 inside function (FreePageBtreeSearch)
> >
> > Perhaps this is a rare situation, but I think it's worth preventing.
>
> Please can we try to hit this rare condition by any test case. If you have
> any test cases, please share.
>
Added to Commitfest (https://commitfest.postgresql.org/34/3236/), so we
don't forget.

regards,
Ranier Vilela

>

Re: psql - factor out echo code

[Alvaro Herrera]

On 2021-Jul-02, Tom Lane wrote:

> Fabien COELHO  writes:
> > Yes. Maybe decorations should be SQL comments, and the purpose/origin of 
> > the query could be made clear as you suggest, eg something like markdown 
> > in a comment:
> >"-- #  QUERY\n%s\n\n"
> 
> If we keep the decoration, I'd agree with dropping all the asterisks.
> I'd vote for something pretty minimalistic, like
> 
>   -- INTERNAL QUERY:

I think the most interesting case for decoration is the "step by step"
mode, where you want the "title" that precedes each query be easily
visible.  I think two uppercase words are not sufficient for that ...
and Markdown format which would force you to convert to HTML before you
can notice where it is, are not sufficient either.  The line with a few
asterisks seems fine to me for that.  Removing the asterisks in the
other case seems fine.  I admit I don't use the step-by-step mode all
that much, though.

Also: one place that prints queries that wasn't mentioned before is
exec_command_print() in command.c.

-- 
Álvaro Herrera  Valdivia, Chile  —  https://www.EnterpriseDB.com/
"Ed is the standard text editor."
  http://groups.google.com/group/alt.religion.emacs/msg/8d94ddab6a9b0ad3

Re: psql - factor out echo code

[Tom Lane]

Fabien COELHO  writes:
> Yes. Maybe decorations should be SQL comments, and the purpose/origin of 
> the query could be made clear as you suggest, eg something like markdown 
> in a comment:
>"-- #  QUERY\n%s\n\n"

If we keep the decoration, I'd agree with dropping all the asterisks.
I'd vote for something pretty minimalistic, like

-- INTERNAL QUERY:

regards, tom lane

Re: Add proper planner support for ORDER BY / DISTINCT aggregates

[Tom Lane]

Gavin Flower  writes:
> On 2/07/21 8:39 pm, David Rowley wrote:
>> That's a good question. There was an argument in [1] that mentions
>> that there might be a group of people who rely on aggregation being
>> done in a certain order where they're not specifying an ORDER BY
>> clause in the aggregate.  If that group of people exists, then it's
>> possible they might be upset in the scenario that you describe.

> So I think that pg has no obligation to protect the sensibilities of 
> naive users in this case, especially at the expense of users that want 
> queries to complete as quickly as possible.  IMHO

I agree.  We've broken such expectations in the past and I don't
have much hesitation about breaking them again.

regards, tom lane

Re: logical replication worker accesses catalogs in error context callback

[Tom Lane]

I wrote:
> Didn't look at 0002 yet.

... and now that I have, I don't like it much.  It adds a lot of
complexity, plus some lookup cycles that might be wasted.  I experimented
with looking into the range table as I suggested previously, and
that seems to work; see attached.  (This includes a little bit of
code cleanup along with the bug fix proper.)

An interesting point here is that the range table data will represent
table and column aliases, not necessarily their true names.  I don't
find that wrong, it's just different from what the code presently
does.  If we go with this, likely we should change the plain-relation
code path so that it also prints aliases from the RTE instead of
the actual names.

regards, tom lane

diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index fafbab6b02..b40c331f8f 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -7185,6 +7185,10 @@ make_tuple_from_result_row(PGresult *res,
 /*
  * Callback function which is called when error occurs during column value
  * conversion.  Print names of column and relation.
+ *
+ * Note that this function mustn't do any catalog lookups, since we are in
+ * an already-failed transaction.  Fortunately, we can get info from the
+ * relcache entry (for a simple scan) or the query rangetable (for joins).
  */
 static void
 conversion_error_callback(void *arg)
@@ -7198,10 +7202,14 @@ conversion_error_callback(void *arg)
 	{
 		/* error occurred in a scan against a foreign table */
 		TupleDesc	tupdesc = RelationGetDescr(errpos->rel);
-		Form_pg_attribute attr = TupleDescAttr(tupdesc, errpos->cur_attno - 1);
 
 		if (errpos->cur_attno > 0 && errpos->cur_attno <= tupdesc->natts)
+		{
+			Form_pg_attribute attr = TupleDescAttr(tupdesc,
+   errpos->cur_attno - 1);
+
 			attname = NameStr(attr->attname);
+		}
 		else if (errpos->cur_attno == SelfItemPointerAttributeNumber)
 			attname = "ctid";
 
@@ -7220,35 +7228,32 @@ conversion_error_callback(void *arg)
 
 		/*
 		 * Target list can have Vars and expressions.  For Vars, we can get
-		 * its relation, however for expressions we can't.  Thus for
+		 * some information, however for expressions we can't.  Thus for
 		 * expressions, just show generic context message.
 		 */
 		if (IsA(tle->expr, Var))
 		{
-			RangeTblEntry *rte;
 			Var		   *var = (Var *) tle->expr;
+			RangeTblEntry *rte = exec_rt_fetch(var->varno, estate);
 
-			rte = exec_rt_fetch(var->varno, estate);
+			relname = rte->eref->aliasname;
 
 			if (var->varattno == 0)
 is_wholerow = true;
-			else
-attname = get_attname(rte->relid, var->varattno, false);
-
-			relname = get_rel_name(rte->relid);
+			else if (var->varattno > 0 &&
+	 var->varattno <= list_length(rte->eref->colnames))
+attname = strVal(list_nth(rte->eref->colnames,
+		  var->varattno - 1));
 		}
-		else
-			errcontext("processing expression at position %d in select list",
-	   errpos->cur_attno);
 	}
 
-	if (relname)
-	{
-		if (is_wholerow)
-			errcontext("whole-row reference to foreign table \"%s\"", relname);
-		else if (attname)
-			errcontext("column \"%s\" of foreign table \"%s\"", attname, relname);
-	}
+	if (relname && is_wholerow)
+		errcontext("whole-row reference to foreign table \"%s\"", relname);
+	else if (relname && attname)
+		errcontext("column \"%s\" of foreign table \"%s\"", attname, relname);
+	else
+		errcontext("processing expression at position %d in select list",
+   errpos->cur_attno);
 }
 
 /*

Re: Add proper planner support for ORDER BY / DISTINCT aggregates

[Gavin Flower]

On 2/07/21 8:39 pm, David Rowley wrote:

On Fri, 2 Jul 2021 at 19:54, Ronan Dunklau  wrote:

I don't know if it's acceptable, but in the case where you add both an
aggregate with an ORDER BY clause, and another aggregate without the clause,
the output for the unordered one will change and use the same ordering, maybe
suprising the unsuspecting user. Would that be acceptable ?

That's a good question. There was an argument in [1] that mentions
that there might be a group of people who rely on aggregation being
done in a certain order where they're not specifying an ORDER BY
clause in the aggregate.  If that group of people exists, then it's
possible they might be upset in the scenario that you describe.


[...]

I've always worked on the assumption that if I do not specify an ORDER 
BY clause then the rdbms is expected to present rows in the order most 
efficient for it to do so. If pg orders rows when not requested then 
this could add extra elapsed time to the query, which might be 
significant for large queries.


I don't know of any rdbms that guarantees the order of returned rows 
when an ORDER BY clause is not used.


So I think that pg has no obligation to protect the sensibilities of 
naive users in this case, especially at the expense of users that want 
queries to complete as quickly as possible.  IMHO



Cheers,
Gavin

Re: Synchronous commit behavior during network outage

[Jeff Davis]

On Fri, 2021-07-02 at 11:39 +0500, Andrey Borodin wrote:
> If the failover happens due to unresponsive node we cannot just turn
> off sync rep. We need to have some spare connections for that (number
> of stuck backends will skyrocket during network partitioning). We
> need available descriptors and some memory to fork new backend. We
> will need to re-read config. We need time to try after all.
> At some failures we may lack some of these.

I think it's a good point that, when things start to go wrong, they can
go very wrong very quickly.

But until you've disabled sync rep, the primary will essentially be
down for writes whether using this new feature or not. Even if you can
terminate some backends to try to free space, the application will just
make new connections that will get stuck the same way.

You can avoid the "fork backend" problem by keeping a connection always
open from the HA tool, or by editing the conf to disable sync rep and
issuing SIGHUP instead. Granted, that still takes some memory.

> Partial degradation is already hard task. Without ability to easily
> terminate running Postgres HA tool will often resort to SIGKILL.

When the system is really wedged as you describe (waiting on sync rep,
tons of connections, and low memory), what information do you expect
the HA tool to be able to collect, and what actions do you expect it to
take?

Presumably, you'd want it to disable sync rep at some point to get back
online. Where does SIGTERM fit into the picture?

> > If you don't handle the termination case, then there's still a
> > chance
> > for the transaction to become visible to other clients before its
> > replicated.
> 
> Termination is admin command, they know what they are doing.
> Cancelation is part of user protocol.

>From the pg_terminate_backend() docs: "This is also allowed if the
calling role is a member of the role whose backend is being terminated
or the calling role has been granted pg_signal_backend", so it's not
really an admin command. Even for an admin, it might be hard to
understand why terminating a backend could result in losing a visible
transaction.

I'm not really seeing two use cases here for two GUCs. Are you sure you
want to disable only cancels but allow termination to proceed?

Regards,
Jeff Davis

Re: logical replication worker accesses catalogs in error context callback

[Tom Lane]

Bharath Rupireddy  writes:
>> << Attaching v5-0001 here again for completion >>
>> I'm attaching v5-0001 patch that avoids printing the column type names
>> in the context message thus no cache lookups have to be done in the
>> error context callback. I think the column name is enough to know on
>> which column the error occurred and if required it's type can be known
>> by the user. This patch gets rid of printing local and remote type
>> names in slot_store_error_callback and also
>> logicalrep_typmap_gettypname because it's unnecessary. Thoughts?

I've now pushed this patch.  I noted that once we drop
logicalrep_typmap_gettypname, there's really nothing using
the LogicalRepTypMap table at all, so I nuked that too.
(We can always recover the code from the git archives if
some reason to use it emerges.)

Didn't look at 0002 yet.

regards, tom lane

Re: psql - factor out echo code

[Fabien COELHO]

Hello Tom,


I went to commit this, figuring that it was a trivial bit of code
consolidation, but as I looked around in common.c I got rather
unhappy with the inconsistent behavior of things.  Examining
the various places that implement "echo"-related logic, we have
the three places this patch proposes to unify, which log queries
using

   fprintf(out,
   _("* QUERY **\n"
 "%s\n"
 "**\n\n"), query);

and then we have two more that just do

   puts(query);

plus this:

   if (!OK && pset.echo == PSQL_ECHO_ERRORS)
   pg_log_info("STATEMENT:  %s", query);

So it's exactly fifty-fifty as to whether we add all that decoration
or none at all.  I think if we're going to touch this logic, we
ought to try to unify the behavior.


+1.

I did not go this way because I wanted it to be a simple restructuring 
patch so that it could go through without much ado, but I agree with 
improving the current status. I'm not sure we want too much ascii-art.


My vote would be to drop the decoration everywhere, but perhaps there 
are votes not to?


No, I'd be ok with removing the decoration, or at least simplify them, or 
as you suggest below make the have a useful semantics.



A different angle is that the identical decoration is used for both
psql-generated queries that are logged because of ECHO_HIDDEN, and
user-entered queries.  This seems at best rather unhelpful.


Indeed.

If we keep the decoration, should we make it different for those two 
cases?  (Maybe "INTERNAL QUERY" vs "QUERY", for example.)  The cases 
with no decoration likewise fall into multiple categories, both 
user-entered and generated-by-gexec; if we were going with a decorated 
approach I'd think it useful to make a distinction between those, too.


Thoughts?


Yes. Maybe decorations should be SQL comments, and the purpose/origin of 
the query could be made clear as you suggest, eg something like markdown 
in a comment:


  "-- #  QUERY\n%s\n\n"

with  in USER DESCRIPTION COMPLETION GEXEC…

--
Fabien.

Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*)

[Justin Pryzby]

On Tue, Apr 06, 2021 at 11:01:31AM -0500, Justin Pryzby wrote:
> On Wed, Dec 23, 2020 at 01:17:10PM -0600, Justin Pryzby wrote:
> > On Mon, Nov 23, 2020 at 04:14:18PM -0500, Tom Lane wrote:
> > > * I noticed that you did s/stat/lstat/.  That's fine on Unix systems,
> > > but it won't have any effect on Windows systems (cf bed90759f),
> > > which means that we'll have to document a platform-specific behavioral
> > > difference.  Do we want to go there?
> > >
> > > Maybe this patch needs to wait on somebody fixing our lack of real 
> > > lstat() on Windows.
> > 
> > I think only the "top" patches depend on lstat (for the "type" column and
> > recursion, to avoid loops).  The initial patches are independently useful, 
> > and
> > resolve the original issue of hiding tmpdirs.  I've rebased and re-arranged 
> > the
> > patches to reflect this.
> 
> I said that, but then failed to attach the re-arranged patches.
> Now I also renumbered OIDs following best practice.
> 
> The first handful of patches address the original issue, and I think could be
> "ready":
> 
> $ git log --oneline origin..pg-ls-dir-new |tac
> ... Document historic behavior of links to directories..
> ... Add tests on pg_ls_dir before changing it
> ... Add pg_ls_dir_metadata to list a dir with file metadata..
> ... pg_ls_tmpdir to show directories and "isdir" argument..
> ... pg_ls_*dir to show directories and "isdir" column..
> 
> These others are optional:
> ... pg_ls_logdir to ignore error if initial/top dir is missing..
> ... pg_ls_*dir to return all the metadata from pg_stat_file..
> 
> ..and these maybe requires more work for lstat on windows:
> ... pg_stat_file and pg_ls_dir_* to use lstat()..
> ... pg_ls_*/pg_stat_file to show file *type*..
> ... Preserve pg_stat_file() isdir..
> ... Add recursion option in pg_ls_dir_files..

@cfbot: rebased
>From 4b34d394af0253dc3be2e47cb2e8ccd286eea0a3 Mon Sep 17 00:00:00 2001
From: Justin Pryzby 
Date: Mon, 16 Mar 2020 14:12:55 -0500
Subject: [PATCH v30 01/11] Document historic behavior of links to
 directories..

Backpatch to 9.5: pg_stat_file
---
 doc/src/sgml/func.sgml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 6388385edc..7a830f0684 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -27004,7 +27004,7 @@ SELECT convert_from(pg_read_binary_file('file_in_utf8.txt'), 'UTF8');
 Returns a record containing the file's size, last access time stamp,
 last modification time stamp, last file status change time stamp (Unix
 platforms only), file creation time stamp (Windows only), and a flag
-indicating if it is a directory.
+indicating if it is a directory (or a symbolic link to a directory).


 This function is restricted to superusers by default, but other users
-- 
2.17.0

>From effa738721a008a5d0c65defecfb7b36ab8f33b3 Mon Sep 17 00:00:00 2001
From: Justin Pryzby 
Date: Tue, 17 Mar 2020 13:16:24 -0500
Subject: [PATCH v30 02/11] Add tests on pg_ls_dir before changing it

---
 src/test/regress/expected/misc_functions.out | 24 
 src/test/regress/sql/misc_functions.sql  |  8 +++
 2 files changed, 32 insertions(+)

diff --git a/src/test/regress/expected/misc_functions.out b/src/test/regress/expected/misc_functions.out
index e845042d38..ea0fc48dbd 100644
--- a/src/test/regress/expected/misc_functions.out
+++ b/src/test/regress/expected/misc_functions.out
@@ -214,6 +214,30 @@ select count(*) > 0 from
  t
 (1 row)
 
+select * from (select pg_ls_dir('.', false, true) as name) as ls where ls.name='.'; -- include_dot_dirs=true
+ name 
+--
+ .
+(1 row)
+
+select * from (select pg_ls_dir('.', false, false) as name) as ls where ls.name='.'; -- include_dot_dirs=false
+ name 
+--
+(0 rows)
+
+select pg_ls_dir('does not exist', true, false); -- ok with missingok=true
+ pg_ls_dir 
+---
+(0 rows)
+
+select pg_ls_dir('does not exist'); -- fails with missingok=false
+ERROR:  could not open directory "does not exist": No such file or directory
+-- Check that expected columns are present
+select * from pg_stat_file('.') limit 0;
+ size | access | modification | change | creation | isdir 
+--++--++--+---
+(0 rows)
+
 --
 -- Test adding a support function to a subject function
 --
diff --git a/src/test/regress/sql/misc_functions.sql b/src/test/regress/sql/misc_functions.sql
index a398349afc..eb6ac12ab4 100644
--- a/src/test/regress/sql/misc_functions.sql
+++ b/src/test/regress/sql/misc_functions.sql
@@ -69,6 +69,14 @@ select count(*) > 0 from
where spcname = 'pg_default') pts
   join pg_database db on pts.pts = db.oid;
 
+select * from (select pg_ls_dir('.', false, true) as name) as ls where ls.name='.'; -- include_dot_dirs=true
+select * from (select pg_ls_dir('.', false, false) as name) as ls where ls.name='.'; -- include_dot_dirs=false
+select pg_ls_dir('does not exist', true, fa

Re: Numeric multiplication overflow errors

[Ranier Vilela]

On Fri, 2 Jul 2021 at 22:55, Dean Rasheed
 wrote:
> Here's an update with the
> last set of changes discussed.
If you allow me a small suggestion.
Move the initializations of the variable tmp_var to after check if the
function can run.
Saves some cycles, when not running.

  /* Ensure we disallow calling when not in aggregate context */
  if (!AggCheckCallContext(fcinfo, NULL))
  elog(ERROR, "aggregate function called in non-aggregate context");

+ init_var(&tmp_var);
+

regards,
Ranier Vilela

Re: Increase value of OUTER_VAR

[Tom Lane]

Here's a more fleshed-out version of this patch.  I ran around and
fixed all the places where INNER_VAR etc. were being assigned directly to
a variable or parameter of type Index, and also grepped for 'Index.*varno'
to find suspicious declarations.  (I didn't change every last instance
of the latter though; just places that could possibly be looking at
post-setrefs.c Vars.)

I concluded that we don't really need to change the type of
CurrentOfExpr.cvarno, because that's never set to a special value.

The main thing I remain concerned about is whether there are more
places like set_pathtarget_cost_width(), where we could be making
an inequality comparison on "varno" that would now be wrong.
I tried to catch this by enabling -Wsign-compare and -Wsign-conversion,
but that produced so many thousands of uninteresting warnings that
I soon gave up.  I'm not sure there's any good way to catch remaining
places like that except to commit the patch and wait for trouble
reports.

So I'm inclined to propose pushing this and seeing what happens.

regards, tom lane

diff --git a/src/backend/executor/execScan.c b/src/backend/executor/execScan.c
index 69ab34573e..9f1d8b6d1e 100644
--- a/src/backend/executor/execScan.c
+++ b/src/backend/executor/execScan.c
@@ -282,7 +282,7 @@ ExecAssignScanProjectionInfo(ScanState *node)
  *		As above, but caller can specify varno expected in Vars in the tlist.
  */
 void
-ExecAssignScanProjectionInfoWithVarno(ScanState *node, Index varno)
+ExecAssignScanProjectionInfoWithVarno(ScanState *node, int varno)
 {
 	TupleDesc	tupdesc = node->ss_ScanTupleSlot->tts_tupleDescriptor;
 
diff --git a/src/backend/executor/execUtils.c b/src/backend/executor/execUtils.c
index ad11392b99..4ab1302313 100644
--- a/src/backend/executor/execUtils.c
+++ b/src/backend/executor/execUtils.c
@@ -65,7 +65,7 @@
 #include "utils/typcache.h"
 
 
-static bool tlist_matches_tupdesc(PlanState *ps, List *tlist, Index varno, TupleDesc tupdesc);
+static bool tlist_matches_tupdesc(PlanState *ps, List *tlist, int varno, TupleDesc tupdesc);
 static void ShutdownExprContext(ExprContext *econtext, bool isCommit);
 
 
@@ -553,7 +553,7 @@ ExecAssignProjectionInfo(PlanState *planstate,
  */
 void
 ExecConditionalAssignProjectionInfo(PlanState *planstate, TupleDesc inputDesc,
-	Index varno)
+	int varno)
 {
 	if (tlist_matches_tupdesc(planstate,
 			  planstate->plan->targetlist,
@@ -579,7 +579,7 @@ ExecConditionalAssignProjectionInfo(PlanState *planstate, TupleDesc inputDesc,
 }
 
 static bool
-tlist_matches_tupdesc(PlanState *ps, List *tlist, Index varno, TupleDesc tupdesc)
+tlist_matches_tupdesc(PlanState *ps, List *tlist, int varno, TupleDesc tupdesc)
 {
 	int			numattrs = tupdesc->natts;
 	int			attrno;
diff --git a/src/backend/executor/nodeCustom.c b/src/backend/executor/nodeCustom.c
index c82060e6d1..1dfa53c381 100644
--- a/src/backend/executor/nodeCustom.c
+++ b/src/backend/executor/nodeCustom.c
@@ -31,7 +31,7 @@ ExecInitCustomScan(CustomScan *cscan, EState *estate, int eflags)
 	CustomScanState *css;
 	Relation	scan_rel = NULL;
 	Index		scanrelid = cscan->scan.scanrelid;
-	Index		tlistvarno;
+	int			tlistvarno;
 
 	/*
 	 * Allocate the CustomScanState object.  We let the custom scan provider
diff --git a/src/backend/executor/nodeForeignscan.c b/src/backend/executor/nodeForeignscan.c
index 9dc38d47ea..cd93d1d768 100644
--- a/src/backend/executor/nodeForeignscan.c
+++ b/src/backend/executor/nodeForeignscan.c
@@ -128,7 +128,7 @@ ExecInitForeignScan(ForeignScan *node, EState *estate, int eflags)
 	ForeignScanState *scanstate;
 	Relation	currentRelation = NULL;
 	Index		scanrelid = node->scan.scanrelid;
-	Index		tlistvarno;
+	int			tlistvarno;
 	FdwRoutine *fdwroutine;
 
 	/* check for unsupported flags */
diff --git a/src/backend/nodes/makefuncs.c b/src/backend/nodes/makefuncs.c
index 01c110cd2f..7d1a01d1ed 100644
--- a/src/backend/nodes/makefuncs.c
+++ b/src/backend/nodes/makefuncs.c
@@ -63,7 +63,7 @@ makeSimpleA_Expr(A_Expr_Kind kind, char *name,
  *	  creates a Var node
  */
 Var *
-makeVar(Index varno,
+makeVar(int varno,
 		AttrNumber varattno,
 		Oid vartype,
 		int32 vartypmod,
@@ -85,7 +85,7 @@ makeVar(Index varno,
 	 * them, but just initialize them to the given varno/varattno.  This
 	 * reduces code clutter and chance of error for most callers.
 	 */
-	var->varnosyn = varno;
+	var->varnosyn = (Index) varno;
 	var->varattnosyn = varattno;
 
 	/* Likewise, we just set location to "unknown" here */
@@ -100,7 +100,7 @@ makeVar(Index varno,
  *		TargetEntry
  */
 Var *
-makeVarFromTargetEntry(Index varno,
+makeVarFromTargetEntry(int varno,
 	   TargetEntry *tle)
 {
 	return makeVar(varno,
@@ -131,7 +131,7 @@ makeVarFromTargetEntry(Index varno,
  */
 Var *
 makeWholeRowVar(RangeTblEntry *rte,
-Index varno,
+int varno,
 Index varlevelsup,
 bool allowScalar)
 {
diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index e32b92e299..bcb116ae8d 10

Re: Signed vs. Unsigned (some)

[Ranier Vilela]

Em sex., 2 de jul. de 2021 às 13:29, Alvaro Herrera 
escreveu:

> On 2021-Jul-02, Justin Pryzby wrote:
>
> > On Fri, Jul 02, 2021 at 12:09:23PM +0200, Peter Eisentraut wrote:
>
> > > The use of InvalidBlockNumber with vac_update_relstats() looks a bit
> fishy
> > > to me.  We are using in the same call 0 as the default for
> > > num_all_visible_pages, and we generally elsewhere also use 0 as the
> starting
> > > value for relpages, so it's not clear to me why it should be -1 or
> > > InvalidBlockNumber here.  I'd rather leave it "slightly wrong" for now
> so it
> > > can be checked again.
>
> > |commit 0e69f705cc1a3df273b38c9883fb5765991e04fe
> > |Author: Alvaro Herrera 
> > |Date:   Fri Apr 9 11:29:08 2021 -0400
> > |
> > |Set pg_class.reltuples for partitioned tables
> >
> > 3d35 also affects partitioned tables, and 0e69 appears to do the right
> thing by
> > preserving relpages=-1 during auto-analyze.
>
> I suppose the question is what is the value used for.  BlockNumber is
> typedef'd uint32, an unsigned variable, so using -1 for it is quite
> fishy.  The weird thing is that in vac_update_relstats we cast it to
> (int32) when storing it in the pg_class tuple, so that's quite fishy
> too.
>

> What we really want is for table_block_relation_estimate_size to work
> properly.  What that does is get the signed-int32 value from pg_class
> and cast it back to BlockNumber.  If that assignment gets -1 again, then
> it's all fine.  I didn't test it.
>
It seems to me that it is happening, but it is risky to make comparisons
between different types.

1)
#define InvalidBlockNumber 0x

int main()
{
unsigned int num_pages;
int rel_pages;

num_pages = -1;
rel_pages = (int) num_pages;
printf("num_pages = %u\n", num_pages);
printf("rel_pages = %d\n", rel_pages);
printf("(num_pages == InvalidBlockNumber) => %u\n", (num_pages ==
InvalidBlockNumber));
printf("(rel_pages == InvalidBlockNumber) => %u\n", (rel_pages ==
InvalidBlockNumber));
}

num_pages = 4294967295
rel_pages = -1
(num_pages == InvalidBlockNumber) => 1
(rel_pages == InvalidBlockNumber) => 1 /* 17:68: warning: comparison
between signed and unsigned integer expressions [-Wsign-compare] */

If num_pages is promoted to uint64 and rel_pages to int64:
2)
#define InvalidBlockNumber 0x

int main()
{
unsigned long int num_pages;
long int rel_pages;

num_pages = -1;
rel_pages = (int) num_pages;
printf("num_pages = %lu\n", num_pages);
printf("rel_pages = %ld\n", rel_pages);
printf("(num_pages == InvalidBlockNumber) => %u\n", (num_pages ==
InvalidBlockNumber));
printf("(rel_pages == InvalidBlockNumber) => %u\n", (rel_pages ==
InvalidBlockNumber));
}

num_pages = 18446744073709551615
rel_pages = -1
(num_pages == InvalidBlockNumber) => 0
(rel_pages == InvalidBlockNumber) => 0 /* 17:68: warning: comparison
between signed and unsigned integer expressions [-Wsign-compare] */

As Kyotaro said:
"they might be different if we forget to widen the constant
when widening the types"

regards,
Ranier Vilela

Back-branch commit complexity

[Bruce Momjian]

In my git workflow, I normally use scripts to simplify and check things.
Previously, most of my workfload was on master, with patches migrated to
appropriate back branches.

FYI, now that we have the release notes only in the major version
branches, I have had to adjust my scripts to allow for more
per-major-version branches and automated doc builds of back branches.  I
thought people might like to know if they come upon the same issue.

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

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

Re: Signed vs. Unsigned (some)

[Alvaro Herrera]

On 2021-Jul-02, Justin Pryzby wrote:

> On Fri, Jul 02, 2021 at 12:09:23PM +0200, Peter Eisentraut wrote:

> > The use of InvalidBlockNumber with vac_update_relstats() looks a bit fishy
> > to me.  We are using in the same call 0 as the default for
> > num_all_visible_pages, and we generally elsewhere also use 0 as the starting
> > value for relpages, so it's not clear to me why it should be -1 or
> > InvalidBlockNumber here.  I'd rather leave it "slightly wrong" for now so it
> > can be checked again.

> |commit 0e69f705cc1a3df273b38c9883fb5765991e04fe
> |Author: Alvaro Herrera 
> |Date:   Fri Apr 9 11:29:08 2021 -0400
> |
> |Set pg_class.reltuples for partitioned tables
> 
> 3d35 also affects partitioned tables, and 0e69 appears to do the right thing 
> by
> preserving relpages=-1 during auto-analyze.

I suppose the question is what is the value used for.  BlockNumber is
typedef'd uint32, an unsigned variable, so using -1 for it is quite
fishy.  The weird thing is that in vac_update_relstats we cast it to
(int32) when storing it in the pg_class tuple, so that's quite fishy
too.

What we really want is for table_block_relation_estimate_size to work
properly.  What that does is get the signed-int32 value from pg_class
and cast it back to BlockNumber.  If that assignment gets -1 again, then
it's all fine.  I didn't test it.

I think changing the vac_update_relstats call I added in 0e69f705cc1a to
InvalidBlockNumber is fine.  I didn't verify any other places.

I think storing BlockNumber values >= 2^31 in an int32 catalog column is
asking for trouble.  We'll have to fix that at some point.

-- 
Álvaro Herrera  Valdivia, Chile  —  https://www.EnterpriseDB.com/

Re: wrong relkind error messages

[Alvaro Herrera]

On 2021-Jun-24, Peter Eisentraut wrote:

> There might be room for some wordsmithing in a few places, but generally I
> think this is complete.

This looks good to me.  I am +0.1 on your proposal of "cannot have
triggers" vs Michael's "cannot create triggers", but really I could go
with either.  Michael's idea has the disadvantage that if the user fails
to see the trailing "s" in "triggers" they could get the idea that it's
possible to create some other trigger; that seems impossible to miss
with your wording.  But it's not that bad either.

It seemed odd to me at first that errdetail_relkind_not_supported()
returns int, but I realized that it's a trick to let you write "return
errdetail()" so you don't have to have "break" which would require one
extra line.  Looks fine.

-- 
Álvaro Herrera PostgreSQL Developer  —  https://www.EnterpriseDB.com/

Re: Preventing abort() and exit() calls in libpq

[Tom Lane]

Alvaro Herrera  writes:
> Now it's hoverfly:
> ! nm -A -u libpq.so.5 2>/dev/null | grep -v __cxa_atexit | grep exit
> libpq.so.5: atexit   U   -
> libpq.so.5: pthread_exit U   -

Ugh.  What in the world is producing those references?

(As I mentioned upthread, I'm quite suspicious of libpq trying to
perform any actions in an atexit callback, because of the uncertainty
about whether some later atexit callback could try to use libpq
functions.  So this seems like it might be an actual bug.)

regards, tom lane

Re: psql - factor out echo code

[Tom Lane]

Fabien COELHO  writes:
> [ psql-echo-2.patch ]

I went to commit this, figuring that it was a trivial bit of code
consolidation, but as I looked around in common.c I got rather
unhappy with the inconsistent behavior of things.  Examining
the various places that implement "echo"-related logic, we have
the three places this patch proposes to unify, which log queries
using

fprintf(out,
_("* QUERY **\n"
  "%s\n"
  "**\n\n"), query);

and then we have two more that just do

puts(query);

plus this:

if (!OK && pset.echo == PSQL_ECHO_ERRORS)
pg_log_info("STATEMENT:  %s", query);

So it's exactly fifty-fifty as to whether we add all that decoration
or none at all.  I think if we're going to touch this logic, we
ought to try to unify the behavior.  My vote would be to drop the
decoration everywhere, but perhaps there are votes not to?

A different angle is that the identical decoration is used for both
psql-generated queries that are logged because of ECHO_HIDDEN, and
user-entered queries.  This seems at best rather unhelpful.  If
we keep the decoration, should we make it different for those two
cases?  (Maybe "INTERNAL QUERY" vs "QUERY", for example.)  The
cases with no decoration likewise fall into multiple categories,
both user-entered and generated-by-gexec; if we were going with
a decorated approach I'd think it useful to make a distinction
between those, too.

Thoughts?

regards, tom lane

Re: Preventing abort() and exit() calls in libpq

[Alvaro Herrera]

Now it's hoverfly:

! nm -A -u libpq.so.5 2>/dev/null | grep -v __cxa_atexit | grep exit
libpq.so.5: atexit   U   -
libpq.so.5: pthread_exit U   -

https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=hoverfly&dt=2021-07-02%2010%3A10%3A29

-- 
Álvaro Herrera PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"Use it up, wear it out, make it do, or do without"

Re: Detecting File Damage & Inconsistencies

[Simon Riggs]

On Fri, Jul 2, 2021 at 5:34 AM Craig Ringer
 wrote:
>
> On Fri, 2 Jul 2021 at 00:19, Simon Riggs  wrote:
>
>>
>> > So yeah. I think it'd be better to log the info you want at start-of-txn 
>> > unless there's a compelling reason not so, and I don't see one yet.
>>
>> AFAIK, XLOG_XACT_ASSIGNMENT does not occur for normal top-level
>> transactions, only for subxids.
>>
>> I don't really want to add an extra record just for this because it
>> will slow down applications and it won't get turned on as often.
>
>
> OK, that makes sense - I was indeed operating on an incorrect assumption.
>
> I wouldn't want to add a new record either. I thought we could piggyback on 
> XLOG_XACT_ASSIGNMENT with a new chunk that's only added when the feature is 
> required, much like we do for replication origin info on commit records.
>
> Is it worth considering forcing XLOG_XACT_ASSIGNMENT to be logged if this 
> feature is enabled?

My feeling is that the drop in performance would lead to it being
turned off most of the time, reducing the value of the feature.

Does anyone else disagree?

> If you don't think the sorts of use cases I presented are worth the trouble 
> that's fair enough. I'm not against adding it on the commit record. It's just 
> that with logical decoding moving toward a streaming model I suspect only 
> having it at commit time may cause us some pain later.

I think you have some good ideas about how to handle larger
transactions with streaming. As a separate patch it might be worth
keeping track of transaction size and logging something when a
transaction gets too large.

-- 
Simon Riggshttp://www.EnterpriseDB.com/

Re: Signed vs. Unsigned (some)

[Justin Pryzby]

On Fri, Jul 02, 2021 at 12:09:23PM +0200, Peter Eisentraut wrote:
> On 16.06.21 10:48, Peter Eisentraut wrote:
> > On 15.06.21 10:17, Kyotaro Horiguchi wrote:
> > > The definitions are not ((type) -1) but ((type) 0x) so
> > > actually they might be different if we forget to widen the constant
> > > when widening the types.  Regarding to the compiler behavior, I think
> > > we are assuming C99[1] and C99 defines that -1 is converted to
> > > Uxxx_MAX. (6.3.1.3 Singed and unsigned integers)
> > > 
> > > I'm +0.2 on it.  It might be worthwhile as a matter of style.
> > 
> > I think since we have the constants we should use them.
> 
> I have pushed the InvalidBucket changes.
> 
> The use of InvalidBlockNumber with vac_update_relstats() looks a bit fishy
> to me.  We are using in the same call 0 as the default for
> num_all_visible_pages, and we generally elsewhere also use 0 as the starting
> value for relpages, so it's not clear to me why it should be -1 or
> InvalidBlockNumber here.  I'd rather leave it "slightly wrong" for now so it
> can be checked again.

There's two relevant changes:

|commit 3d351d916b20534f973eda760cde17d96545d4c4
|Author: Tom Lane 
|Date:   Sun Aug 30 12:21:51 2020 -0400
|
|Redefine pg_class.reltuples to be -1 before the first VACUUM or ANALYZE.

|commit 0e69f705cc1a3df273b38c9883fb5765991e04fe
|Author: Alvaro Herrera 
|Date:   Fri Apr 9 11:29:08 2021 -0400
|
|Set pg_class.reltuples for partitioned tables

3d35 also affects partitioned tables, and 0e69 appears to do the right thing by
preserving relpages=-1 during auto-analyze.

Note that Alvaro's commit message and comment refer to relpages, but should
have said reltuples - comment fixed at 7ef8b52cf.

-- 
Justin

Re: [bug?] Missed parallel safety checks, and wrong parallel safety

[Robert Haas]

On Wed, Jun 30, 2021 at 11:46 PM Greg Nancarrow  wrote:
> I personally think "(b) provide an option to the user to specify
> whether inserts can be parallelized on a relation" is the preferable
> option.
> There seems to be too many issues with the alternative of trying to
> determine the parallel-safety of a partitioned table automatically.
> I think (b) is the simplest and most consistent approach, working the
> same way for all table types, and without the overhead of (a).
> Also, I don't think (b) is difficult for the user. At worst, the user
> can use the provided utility-functions at development-time to verify
> the intended declared table parallel-safety.
> I can't really see some mixture of (a) and (b) being acceptable.

Yeah, I'd like to have it be automatic, but I don't have a clear idea
how to make that work nicely. It's possible somebody (Tom?) can
suggest something that I'm overlooking, though.

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

Re: RFC: Logging plan of the running query

[Bharath Rupireddy]

On Tue, Jun 22, 2021 at 8:00 AM torikoshia  wrote:
> Updated the patch.

Thanks for the patch. Here are some comments on the v4 patch:

1) Can we do + ExplainState *es = NewExplainState(); and es
assignments after if (!ActivePortal || !ActivePortal->queryDesc), just
to avoid unnecessary call in case of error hit? Also note that, we can
easily hit the error case.

2) It looks like there's an improper indentation. MyProcPid and
es->str->data, should start from the ".
+ ereport(LOG_SERVER_ONLY,
+ (errmsg("backend with PID %d is not running a query",
+ MyProcPid)));

+ ereport(LOG_SERVER_ONLY,
+ (errmsg("plan of the query running on backend with PID %d is:\n%s",
+ MyProcPid,
+ es->str->data),
For reference see errmsg("unrecognized value for EXPLAIN option \"%s\": \"%s\"",

3)I prefer to do this so that any new piece of code can be introduced
in between easily and it will be more readable as well.
+Datum
+pg_log_current_query_plan(PG_FUNCTION_ARGS)
+{
+ pid_t pid;
+ bool result;
+
+ pid = PG_GETARG_INT32(0);
+ result = SendProcSignalForLogInfo(pid, PROCSIG_LOG_CURRENT_PLAN);
+
+ PG_RETURN_BOOL(result);
+}
If okay, please also change for the pg_log_backend_memory_contexts.

4) Extra whitespace before the second line i.e. 2nd line reason should
be aligned with the 1st line reason.
+ Assert(reason == PROCSIG_LOG_MEMORY_CONTEXT ||
+ reason == PROCSIG_LOG_CURRENT_PLAN);

5) How about "Requests to log the plan of the query currently running
on the backend with specified process ID along with the untruncated
query string"?
+Requests to log the untruncated query string and its plan for
+the query currently running on the backend with the specified
+process ID.

6) A typo: it is "nested statements (..) are not"
+Note that nested statements (statements executed inside a function) is not

7) I'm not sure what you mean by "Some functions output what you want
to the log."
--- Memory contexts are logged and they are not returned to the function.
+-- Some functions output what you want to the log.
Instead, can we say "These functions return true if the specified
backend is successfully signaled, otherwise false. Upon receiving the
signal, the backend will log the information to the server log."

Regards,
Bharath Rupireddy.

Re: Commit fest manager

[Ibrar Ahmed]

On Fri, 2 Jul 2021 at 7:06 PM, vignesh C  wrote:

> On Fri, Jul 2, 2021 at 6:05 PM Ibrar Ahmed  wrote:
> >
> >
> >
> > On Fri, 2 Jul 2021 at 1:47 PM, David Rowley 
> wrote:
> >>
> >> On Fri, 2 Jul 2021 at 15:04, vignesh C  wrote:
> >> > I'm interested in assisting Ibrar Ahmed.
> >>
> >> It might be worth talking to Ibrar to see where you can lend a hand. I
> >> think in terms of the number of patches, this might be our biggest
> >> commitfest yet.
> >>
> >> 2020-07 246
> >> 2020-09 235
> >> 2020-11 244
> >> 2021-01 260
> >> 2020-03 295
> >> 2020-07 342
> >>
> >> It's possible Ibrar would welcome you helping to take care of some of
> >> the duties.  I've never been brave enough to take on the CF manager
> >> role yet, but from what I can see, to do a good job takes a huge
> >> amount of effort.
> >>
> >> David
> >
> >
> > I am willing to take the responsibility, help from vegnsh is welcome
>
> Thanks, Can someone provide me permissions as this will be my first
> time. My username is vignesh.postgres.
>
> Regards,
> Vignesh

i need permission my id is ibrar.ah...@gmail.com

>
> --
Ibrar Ahmed

Re: Commit fest manager

[vignesh C]

On Fri, Jul 2, 2021 at 6:05 PM Ibrar Ahmed  wrote:
>
>
>
> On Fri, 2 Jul 2021 at 1:47 PM, David Rowley  wrote:
>>
>> On Fri, 2 Jul 2021 at 15:04, vignesh C  wrote:
>> > I'm interested in assisting Ibrar Ahmed.
>>
>> It might be worth talking to Ibrar to see where you can lend a hand. I
>> think in terms of the number of patches, this might be our biggest
>> commitfest yet.
>>
>> 2020-07 246
>> 2020-09 235
>> 2020-11 244
>> 2021-01 260
>> 2020-03 295
>> 2020-07 342
>>
>> It's possible Ibrar would welcome you helping to take care of some of
>> the duties.  I've never been brave enough to take on the CF manager
>> role yet, but from what I can see, to do a good job takes a huge
>> amount of effort.
>>
>> David
>
>
> I am willing to take the responsibility, help from vegnsh is welcome

Thanks, Can someone provide me permissions as this will be my first
time. My username is vignesh.postgres.

Regards,
Vignesh

Re: Mention --enable-tap-tests in the TAP section page

[Andrew Dunstan]

On 7/1/21 9:53 PM, Michael Paquier wrote:
> On Thu, Jul 01, 2021 at 10:03:10AM -0400, Greg Sabino Mullane wrote:
>> Searching about the TAP tests often leads to this page, but there is no
>> easy link or mention of the fact that the sample invocations will not work
>> without the special config flag.
> This is mentioned on a different page, "Running the Tests", but for
> the set of extra tests.  Adding an extra reference on this page is a
> good idea.




Agreed.


cheers


andrew

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

RE: ECPG bug fix: DECALRE STATEMENT and DEALLOCATE, DESCRIBE

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

Dear Hackers,

I revised my patch.

> Please also ensure that you're generating the patch against git HEAD.
> The cfbot shows it as failing to apply, likely because you're looking
> at something predating ad8305a43d1890768a613d3fb586b44f17360f29.

Maybe there was something wrong with my local environment. Sorry.

> However, I perfectly agree that it's very difficult for users to find a 
> problem from the message.
> I will try to add information to it in the next patch.

I added such a message and some tests, but I began to think this is strange.
Now I'm wondering why the connection is checked in some DESCRIPTOR-related
statements? In my understanding connection name is not used in 
ECPGallocate_desc(),
ECPGdeallocate_desc(), ECPGget_desc() and so on.
Hence I think lookup_descriptor() and drop_descriptor() can be removed.
This idea can solve your first argument.

> You're right. This is very stupid program. I'll combine them into one.

Check_declared_list() was moved to stmt:ECPGDescribe rule.
Some similar rules still remain in ecpg.trailer, but INPUT/OUTPUT statements 
have
different rules and actions and I cannot combine well.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED



v02-0001-fix-deallocate-describe.patch
Description: v02-0001-fix-deallocate-describe.patch


v02-0002-add-test.patch
Description: v02-0002-add-test.patch

Re: Replication protocol doc fix

[Robert Haas]

On Fri, Jul 2, 2021 at 1:55 AM Jeff Davis  wrote:
> On Wed, 2021-06-30 at 12:25 -0400, Robert Haas wrote:
> > I am not sure whether this works or not. Holding off cancel
> > interrupts
> > across possible network I/O seems like a non-starter. We have to be
> > able to kill off connections that have wedged.
>
> I was following a pattern that I saw in CopyGetData() and
> SocketBackend(). If I understand correctly, the idea is to avoid a
> cancel leaving part of a message unread, which would desync the
> protocol.

Right, that seems like a good goal. Thinking about this a little more,
it's only holding off *cancel* interrupts, not *all* interrupts, so
presumably you can still terminate the backend in this state. That's
not so bad, and it's not clear how we could do any better. So I
withdraw my previous complaint about this point.

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

Re: Update maintenance_work_mem/autovacuum_work_mem to reflect the 1GB limitation with VACUUM

[Laurenz Albe]

On Fri, 2021-07-02 at 23:31 +1200, David Rowley wrote:
> I had a look at the patch in [1] and I find it a bit weird that we'd
> write the following about autovacuum_work_mem in our docs:
> 
> +   
> +Note that VACUUM has a hard-coded limit of 1GB
> +for the amount of memory used, so setting
> +autovacuum_work_mem higher than that has no 
> effect.
> +   
> 
> Since that setting is *only* used for auto vacuum, why don't we just
> limit the range of the GUC to 1GB?
> 
> Of course, it wouldn't be wise to backpatch the reduced limit of
> autovacuum_work_mem as it might upset people who have higher values in
> their postgresql.conf when their database fails to restart after an
> upgrade. I think what might be best is just to reduce the limit in
> master and apply the doc patch for just maintenance_work_mem in all
> supported versions. We could just ignore doing anything with
> autovacuum_work_mem in the back branches and put it down to a
> historical mistake that can't easily be fixed now.
> 
> I've attached what I came up with.
> 
> What do you think?
> 
> [1] 
> https://www.postgresql.org/message-id/514fe5ce4714b7b33cb0a611f0c7b72df413bef5.camel%40cybertec.at

I think that is much better.
I am fine with that patch.

Yours,
Laurenz Albe

Re: Commit fest manager

[Ibrar Ahmed]

On Fri, 2 Jul 2021 at 1:47 PM, David Rowley  wrote:

> On Fri, 2 Jul 2021 at 15:04, vignesh C  wrote:
> > I'm interested in assisting Ibrar Ahmed.
>
> It might be worth talking to Ibrar to see where you can lend a hand. I
> think in terms of the number of patches, this might be our biggest
> commitfest yet.
>
> 2020-07 246
> 2020-09 235
> 2020-11 244
> 2021-01 260
> 2020-03 295
> 2020-07 342
>
> It's possible Ibrar would welcome you helping to take care of some of
> the duties.  I've never been brave enough to take on the CF manager
> role yet, but from what I can see, to do a good job takes a huge
> amount of effort.
>
> David


I am willing to take the responsibility, help from vegnsh is welcome

>
> --
Ibrar Ahmed

Re: Numeric multiplication overflow errors

[David Rowley]

On Fri, 2 Jul 2021 at 22:55, Dean Rasheed  wrote:
> Here's an update with the
> last set of changes discussed.

Looks good to me.

Just the question of if we have any problems changing the serialized
format in the back branches.  I'm not sure if that's something we've
done before. I only had a quick look of git blame in the
serial/deserial functions and the only changes I really see apart from
a few cosmetic ones were a57d312a7 and 9cca11c91. Both of which just
went into master.

David

Re: Update maintenance_work_mem/autovacuum_work_mem to reflect the 1GB limitation with VACUUM

[David Rowley]

On Fri, 21 May 2021 at 03:52, Laurenz Albe  wrote:
> Just sending a reply to -hackers so I can add it to the commitfest.

I had a look at the patch in [1] and I find it a bit weird that we'd
write the following about autovacuum_work_mem in our docs:

+   
+Note that VACUUM has a hard-coded limit of 1GB
+for the amount of memory used, so setting
+autovacuum_work_mem higher than that has no effect.
+   

Since that setting is *only* used for auto vacuum, why don't we just
limit the range of the GUC to 1GB?

Of course, it wouldn't be wise to backpatch the reduced limit of
autovacuum_work_mem as it might upset people who have higher values in
their postgresql.conf when their database fails to restart after an
upgrade. I think what might be best is just to reduce the limit in
master and apply the doc patch for just maintenance_work_mem in all
supported versions. We could just ignore doing anything with
autovacuum_work_mem in the back branches and put it down to a
historical mistake that can't easily be fixed now.

I've attached what I came up with.

What do you think?

[1] 
https://www.postgresql.org/message-id/514fe5ce4714b7b33cb0a611f0c7b72df413bef5.camel%40cybertec.at


limit_vacuum_memory_to_1gb.patch
Description: Binary data

Re: Schema variables - new implementation for Postgres 15

[Pavel Stehule]

so 12. 6. 2021 v 8:00 odesílatel Pavel Stehule 
napsal:

> Hi
>
> rebase
>
>
rebase only

Regards

Pavel


Regards
>
> Pavel
>


schema-variables-20210702.patch.gz
Description: application/gzip

Re: Signed vs. Unsigned (some)

[Ranier Vilela]

Em sex., 2 de jul. de 2021 às 07:09, Peter Eisentraut <
peter.eisentr...@enterprisedb.com> escreveu:

> On 16.06.21 10:48, Peter Eisentraut wrote:
> > On 15.06.21 10:17, Kyotaro Horiguchi wrote:
> >> The definitions are not ((type) -1) but ((type) 0x) so
> >> actually they might be different if we forget to widen the constant
> >> when widening the types.  Regarding to the compiler behavior, I think
> >> we are assuming C99[1] and C99 defines that -1 is converted to
> >> Uxxx_MAX. (6.3.1.3 Singed and unsigned integers)
> >>
> >> I'm +0.2 on it.  It might be worthwhile as a matter of style.
> >
> > I think since we have the constants we should use them.
>
> I have pushed the InvalidBucket changes.
>
Nice. Thanks.


> The use of InvalidBlockNumber with vac_update_relstats() looks a bit
> fishy to me.  We are using in the same call 0 as the default for
> num_all_visible_pages, and we generally elsewhere also use 0 as the
> starting value for relpages, so it's not clear to me why it should be -1
> or InvalidBlockNumber here.

It seems to me that the only use in vac_update_relstats is to mark relpages
as invalid (dirty = true).


>   I'd rather leave it "slightly wrong" for
> now so it can be checked again.
>
Ideally InvalidBlockNumber should be 0.
Maybe in the long run this will be fixed.

regards,
Ranier Vilela

Re: Numeric multiplication overflow errors

[Dean Rasheed]

On Fri, 2 Jul 2021 at 10:24, David Rowley  wrote:
>
> I ran this again with a few different worker counts after tuning a few
> memory settings so there was no spilling to disk and so everything was
> in RAM. Mostly so I could get consistent results.
>
> Here's the results. Average over 3 runs on each:
>
> Workers Master Patched Percent
> 811094.1 11084.9 100.08%
> 16   8711.4  8562.6   101.74%
> 32   6961.4  6726.3  103.50%
> 64   6137.4  5854.8  104.83%
> 128 6090.3  5747.4  105.96%
>

Thanks for testing again. Those are nice looking results, and are much
more in line with what I was seeing.

> So the gains are much less at lower worker counts.  I think this is
> because most of the gains are in the serial part of the plan and with
> higher worker counts that part of the plan is relatively much bigger.
>
> So likely performance isn't too critical here, but it is something to
> keep in mind.
>

Yes, agreed. I suspect there's not much more that can be shaved off
this particular piece of code now though. Here's an update with the
last set of changes discussed.

Regards,
Dean
diff --git a/src/backend/utils/adt/numeric.c b/src/backend/utils/adt/numeric.c
new file mode 100644
index eb78f0b..a8c6bbf
--- a/src/backend/utils/adt/numeric.c
+++ b/src/backend/utils/adt/numeric.c
@@ -515,6 +515,9 @@ static void set_var_from_var(const Numer
 static char *get_str_from_var(const NumericVar *var);
 static char *get_str_from_var_sci(const NumericVar *var, int rscale);
 
+static void numericvar_serialize(StringInfo buf, const NumericVar *var);
+static void numericvar_deserialize(StringInfo buf, NumericVar *var);
+
 static Numeric duplicate_numeric(Numeric num);
 static Numeric make_result(const NumericVar *var);
 static Numeric make_result_opt_error(const NumericVar *var, bool *error);
@@ -4943,38 +4946,25 @@ numeric_avg_serialize(PG_FUNCTION_ARGS)
 {
 	NumericAggState *state;
 	StringInfoData buf;
-	Datum		temp;
-	bytea	   *sumX;
 	bytea	   *result;
 	NumericVar	tmp_var;
 
+	init_var(&tmp_var);
+
 	/* Ensure we disallow calling when not in aggregate context */
 	if (!AggCheckCallContext(fcinfo, NULL))
 		elog(ERROR, "aggregate function called in non-aggregate context");
 
 	state = (NumericAggState *) PG_GETARG_POINTER(0);
 
-	/*
-	 * This is a little wasteful since make_result converts the NumericVar
-	 * into a Numeric and numeric_send converts it back again. Is it worth
-	 * splitting the tasks in numeric_send into separate functions to stop
-	 * this? Doing so would also remove the fmgr call overhead.
-	 */
-	init_var(&tmp_var);
-	accum_sum_final(&state->sumX, &tmp_var);
-
-	temp = DirectFunctionCall1(numeric_send,
-			   NumericGetDatum(make_result(&tmp_var)));
-	sumX = DatumGetByteaPP(temp);
-	free_var(&tmp_var);
-
 	pq_begintypsend(&buf);
 
 	/* N */
 	pq_sendint64(&buf, state->N);
 
 	/* sumX */
-	pq_sendbytes(&buf, VARDATA_ANY(sumX), VARSIZE_ANY_EXHDR(sumX));
+	accum_sum_final(&state->sumX, &tmp_var);
+	numericvar_serialize(&buf, &tmp_var);
 
 	/* maxScale */
 	pq_sendint32(&buf, state->maxScale);
@@ -4993,6 +4983,8 @@ numeric_avg_serialize(PG_FUNCTION_ARGS)
 
 	result = pq_endtypsend(&buf);
 
+	free_var(&tmp_var);
+
 	PG_RETURN_BYTEA_P(result);
 }
 
@@ -5006,9 +4998,10 @@ numeric_avg_deserialize(PG_FUNCTION_ARGS
 {
 	bytea	   *sstate;
 	NumericAggState *result;
-	Datum		temp;
-	NumericVar	tmp_var;
 	StringInfoData buf;
+	NumericVar	tmp_var;
+
+	init_var(&tmp_var);
 
 	if (!AggCheckCallContext(fcinfo, NULL))
 		elog(ERROR, "aggregate function called in non-aggregate context");
@@ -5029,11 +5022,7 @@ numeric_avg_deserialize(PG_FUNCTION_ARGS
 	result->N = pq_getmsgint64(&buf);
 
 	/* sumX */
-	temp = DirectFunctionCall3(numeric_recv,
-			   PointerGetDatum(&buf),
-			   ObjectIdGetDatum(InvalidOid),
-			   Int32GetDatum(-1));
-	init_var_from_num(DatumGetNumeric(temp), &tmp_var);
+	numericvar_deserialize(&buf, &tmp_var);
 	accum_sum_add(&(result->sumX), &tmp_var);
 
 	/* maxScale */
@@ -5054,6 +5043,8 @@ numeric_avg_deserialize(PG_FUNCTION_ARGS
 	pq_getmsgend(&buf);
 	pfree(buf.data);
 
+	free_var(&tmp_var);
+
 	PG_RETURN_POINTER(result);
 }
 
@@ -5067,11 +5058,10 @@ numeric_serialize(PG_FUNCTION_ARGS)
 {
 	NumericAggState *state;
 	StringInfoData buf;
-	Datum		temp;
-	bytea	   *sumX;
-	NumericVar	tmp_var;
-	bytea	   *sumX2;
 	bytea	   *result;
+	NumericVar	tmp_var;
+
+	init_var(&tmp_var);
 
 	/* Ensure we disallow calling when not in aggregate context */
 	if (!AggCheckCallContext(fcinfo, NULL))
@@ -5079,36 +5069,18 @@ numeric_serialize(PG_FUNCTION_ARGS)
 
 	state = (NumericAggState *) PG_GETARG_POINTER(0);
 
-	/*
-	 * This is a little wasteful since make_result converts the NumericVar
-	 * into a Numeric and numeric_send converts it back again. Is it worth
-	 * splitting the tasks in numeric_send into separate functions to stop
-	 * this? Doing so would also remove the fmgr call overhead.
-	 */
-	init_var(&tmp_var);
-
-	accum_sum_final(&state->sumX, &tmp_v

Re: wrong relkind error messages

[Peter Eisentraut]

On 02.07.21 08:25, Michael Paquier wrote:

+   ereport(ERROR,
+   (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+errmsg("ALTER action %s cannot be performed on relation 
\"%s\"",
+   action_str, RelationGetRelationName(rel)),
+errdetail_relkind_not_supported(rel->rd_rel->relkind)));
Perhaps the result of alter_table_type_to_string() is worth a note for
translators?


ok


+   case AT_DetachPartitionFinalize:
+   return "DETACH PARTITION FINALIZE";
To be exact, I think that this one should be "DETACH PARTITION
... FINALIZE".


ok


+   if (relkind == RELKIND_INDEX || relkind == RELKIND_PARTITIONED_INDEX)
+   ereport(ERROR,
+   (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+errmsg("cannot change schema of index \"%s\"",
+   rv->relname),
+errhint("Change the schema of the table instead.")));
+   else if (relkind == RELKIND_COMPOSITE_TYPE)
+   ereport(ERROR,
+   (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+errmsg("cannot change schema of composite type
\"%s\"",
+   rv->relname),
+errhint("Use ALTER TYPE instead.")));
I would simplify this part by removing the errhint(), and use "cannot
change schema of relation .." as error string, with a dose of
errdetail_relkind_not_supported().


I aimed for parity with the error reporting in ATExecChangeOwner() here.


+errmsg("relation \"%s\" cannot have triggers",
+   RelationGetRelationName(rel)),
Better as "cannot create/rename/remove triggers on relation \"%s\""
for the three code paths of trigger.c?

+errmsg("relation \"%s\" cannot have rules",
[...]
+errmsg("relation \"%s\" cannot have rules",
For rewriteDefine.c, this could be "cannot create/rename rules on
relation".


I had it like that, but in previous reviews some people liked it better 
this way. ;-)  I tend to agree with that, since the error condition 
isn't that you can't create a rule/etc. (like, due to incorrect 
prerequisite state) but that there cannot be one ever.

Re: Small clean up in nodeAgg.c

[David Rowley]

On Thu, 1 Jul 2021 at 11:09, Tom Lane  wrote:
> Just reading it over quickly, I noticed a comment mentioning
> "aggcombinedfn" which I suppose should be "aggcombinefn".

Thanks. I've fixed that locally.

> No particular opinion on whether this is a net reduction
> of logical complexity.

I had another look over it and I think we do need to be more clear
about when we're talking about aggtransfn and aggcombinefn.  The
existing code uses variables name aggtransfn when the value stored
could be the value for the aggcombinefn.  Additionally, the other
change to remove the special case build_aggregate_combinefn_expr()
function seems good in a sense of reusing more code and reducing the
amount of code in that file.

Unless anyone thinks differently about this, I plan on pushing the
patch in the next day or so.

David

RE: [HACKERS] logical decoding of two-phase transactions

[tanghy.f...@fujitsu.com]

On Thursday, July 1, 2021 11:48 AM Ajin Cherian 
> 
> Adding a new patch (0004) to this patch-set that handles skipping of
> empty streamed transactions. patch-0003 did not
> handle empty streamed transactions. To support this, added a new flag
> "sent_stream_start" to PGOutputTxnData.
> Also transactions which do not have any data will not be stream
> committed or stream prepared or stream aborted.
> Do review and let me know if you have any comments.
> 

Thanks for your patch. I met an issue while using it. When a transaction 
contains TRUNCATE, the subscriber reported an error: " ERROR:  no data left in 
message" and the data couldn't be replicated. 

Steps to reproduce the issue:

(set logical_decoding_work_mem to 64kB at publisher so that streaming could 
work. )

--publisher--
create table test (a int primary key, b varchar);
create publication pub for table test;

--subscriber--
create table test (a int primary key, b varchar);
create subscription sub connection 'dbname=postgres' publication pub 
with(two_phase=on, streaming=on);

--publisher--
BEGIN;
TRUNCATE test;
INSERT INTO test SELECT i, md5(i::text) FROM generate_series(1001, 6000) s(i);
UPDATE test SET b = md5(b) WHERE mod(a,2) = 0;
DELETE FROM test WHERE mod(a,3) = 0;
COMMIT;

The above case worked ok when remove 0004 patch, so I think it’s a problem of 
0004 patch. Please have a look.

Regards
Tang

Re: Signed vs. Unsigned (some)

[Peter Eisentraut]

On 16.06.21 10:48, Peter Eisentraut wrote:

On 15.06.21 10:17, Kyotaro Horiguchi wrote:

The definitions are not ((type) -1) but ((type) 0x) so
actually they might be different if we forget to widen the constant
when widening the types.  Regarding to the compiler behavior, I think
we are assuming C99[1] and C99 defines that -1 is converted to
Uxxx_MAX. (6.3.1.3 Singed and unsigned integers)

I'm +0.2 on it.  It might be worthwhile as a matter of style.


I think since we have the constants we should use them.


I have pushed the InvalidBucket changes.

The use of InvalidBlockNumber with vac_update_relstats() looks a bit 
fishy to me.  We are using in the same call 0 as the default for 
num_all_visible_pages, and we generally elsewhere also use 0 as the 
starting value for relpages, so it's not clear to me why it should be -1 
or InvalidBlockNumber here.  I'd rather leave it "slightly wrong" for 
now so it can be checked again.

Re: Added schema level support for publication.

[vignesh C]

On Fri, Jul 2, 2021 at 10:18 AM tanghy.f...@fujitsu.com
 wrote:
>
> On Wednesday, June 30, 2021 7:43 PM vignesh C  wrote:
> >
> > Thanks for reporting this issue, the attached v9 patch fixes this issue. 
> > This also fixes the other issue you reported at [1].
>
> A comment on v9:
>
> src/bin/psql/describe.c
>
> +  if (pset.sversion >= 15000)
>
> I think it should be 15.

Thanks for reporting this, I will fix this in the next version of the patch.

Regards,
Vignesh

Re: Numeric multiplication overflow errors

[David Rowley]

On Fri, 2 Jul 2021 at 00:28, Dean Rasheed  wrote:
>
> On Thu, 1 Jul 2021 at 06:43, David Rowley  wrote:
> >
> > Master @ 3788c6678
> >
> > Execution Time: 8306.319 ms
> > Execution Time: 8407.785 ms
> > Execution Time: 8491.056 ms
> >
> > Master + numeric-agg-sumX2-overflow-v2.patch
> > Execution Time: 6633.278 ms
> > Execution Time: 6657.350 ms
> > Execution Time: 6568.184 ms
> >
>
> Hmm, I'm a bit surprised by those numbers. I wouldn't have expected it
> to be spending enough time in the serialization/deserialization code
> for it to make such a difference. I was only able to measure a 2-3%
> performance improvement with the same test, and that was barely above
> the noise.

I ran this again with a few different worker counts after tuning a few
memory settings so there was no spilling to disk and so everything was
in RAM. Mostly so I could get consistent results.

Here's the results. Average over 3 runs on each:

Workers Master Patched Percent
811094.1 11084.9 100.08%
16   8711.4  8562.6   101.74%
32   6961.4  6726.3  103.50%
64   6137.4  5854.8  104.83%
128 6090.3  5747.4  105.96%

So the gains are much less at lower worker counts.  I think this is
because most of the gains are in the serial part of the plan and with
higher worker counts that part of the plan is relatively much bigger.

So likely performance isn't too critical here, but it is something to
keep in mind.

David

Re: Commit fest manager

[David Rowley]

On Fri, 2 Jul 2021 at 15:04, vignesh C  wrote:
> I'm interested in assisting Ibrar Ahmed.

It might be worth talking to Ibrar to see where you can lend a hand. I
think in terms of the number of patches, this might be our biggest
commitfest yet.

2020-07 246
2020-09 235
2020-11 244
2021-01 260
2020-03 295
2020-07 342

It's possible Ibrar would welcome you helping to take care of some of
the duties.  I've never been brave enough to take on the CF manager
role yet, but from what I can see, to do a good job takes a huge
amount of effort.

David

Re: Add proper planner support for ORDER BY / DISTINCT aggregates

[David Rowley]

On Fri, 2 Jul 2021 at 19:54, Ronan Dunklau  wrote:
> I don't know if it's acceptable, but in the case where you add both an
> aggregate with an ORDER BY clause, and another aggregate without the clause,
> the output for the unordered one will change and use the same ordering, maybe
> suprising the unsuspecting user. Would that be acceptable ?

That's a good question. There was an argument in [1] that mentions
that there might be a group of people who rely on aggregation being
done in a certain order where they're not specifying an ORDER BY
clause in the aggregate.  If that group of people exists, then it's
possible they might be upset in the scenario that you describe.

I also think that it's going to be pretty hard to make significant
gains in this area if we are too scared to make changes to undefined
behaviour.  You wouldn't have to look too hard in the pgsql-general
mailing list to find someone complaining that their query output is in
the wrong order on some query that does not have an ORDER BY. We
pretty much always tell those people that the order is undefined
without an ORDER BY. I'm not too sure why Tom in [1] classes the ORDER
BY aggregate people any differently. We'll be stuck forever here and
in many other areas if we're too scared to change the order of
aggregation. You could argue that something like parallelism has
changed that for people already. I think the multi-batch Hash
Aggregate code could also change this.

> I was curious about the performance implication of that additional transition,
> and could not reproduce a signifcant difference. I may be doing something
> wrong: how did you highlight it ?

It was pretty basic. I just created a table with two columns and no
index and did something like SELECT a,SUM(b ORDER BY b) from ab GROUP
BY 1;  the new code will include a Sort due to lack of any index and
the old code would have done a sort inside nodeAgg.c. I imagine that
the overhead comes from the fact that in the patched version nodeAgg.c
must ask its subnode (nodeSort.c) for the next tuple each time,
whereas unpatched nodeAgg.c already has all the tuples in a tuplestore
and can fetch them very quickly in a tight loop.

David

[1] https://www.postgresql.org/message-id/6538.1522096067%40sss.pgh.pa.us

Re: rand48 replacement

[Dean Rasheed]

On Thu, 1 Jul 2021 at 22:18, Fabien COELHO  wrote:
>
> > However, this patch seems to be replacing that with a simple
> > modulo operation, which is perhaps the worst possible way to do it.
>
> The modulo operation is biased for large ranges close to the limit, sure.
> Also, the bias is somehow of the same magnitude as the FP multiplication
> approach used previously, so the "worst" has not changed much, it is
> really the same as before.
>

It may be true that the bias is of the same magnitude as FP multiply,
but it is not of the same nature. With FP multiply, the
more-likely-to-be-chosen values are more-or-less evenly distributed
across the range, whereas modulo concentrates them all at one end,
making it more likely to bias test results.

It's worth paying attention to how other languages/libraries implement
this, and basically no one chooses the modulo method, which ought to
raise alarm bells. Of the biased methods, it has the worst kind of
bias and the worst performance.

If a biased method is OK, then the biased integer multiply method
seems to be the clear winner. This requires the high part of a
64x64-bit product, which is trivial if 128-bit integers are available,
but would need a little more work otherwise. There's some code in
common/d2s that might be suitable.

Most other implementations tend to use an unbiased method though, and
I think it's worth doing the same. It might be a bit slower, or even
faster depending on implementation and platform, but in the context of
the DB as a whole, I don't think a few extra cycles matters either
way. The method recommended at the very end of that blog seems to be
pretty good all round, but any of the other commonly used unbiased
methods would probably be OK too.

Regards,
Dean

Re: Add proper planner support for ORDER BY / DISTINCT aggregates

[Ronan Dunklau]

> 
> This allows us to give presorted input to both aggregates in the following
> case:
> 
> SELECT agg(a ORDER BY a),agg2(a ORDER BY a,b) ...
> 
> but just the first agg in this one:
> 
> SELECT agg(a ORDER BY a),agg2(a ORDER BY c) ...

I don't know if it's acceptable, but in the case where you add both an 
aggregate with an ORDER BY clause, and another aggregate without the clause, 
the output for the unordered one will change and use the same ordering, maybe 
suprising the unsuspecting user. Would that be acceptable ?

> When testing the performance of all this I found that when a suitable
> index exists to provide pre-sorted input for the aggregation that the
> performance does improve. Unfortunately, it looks like things get more
> complex when no index exists.  In this case, since we're setting
> pathkeys to tell the planner we need a plan that provides pre-sorted
> input to the aggregates, the planner will add a sort below the
> aggregate node.  I initially didn't see any problem with that as it
> just moves the sort to a Sort node rather than having it done
> implicitly inside nodeAgg.c.  The problem is, it just does not perform
> as well.  I guess this is because when the sort is done inside
> nodeAgg.c that the transition function is called in a tight loop while
> reading records back from the tuplestore.  In the patched version,
> there's an additional node transition in between nodeAgg and nodeSort
> and that causes slower performance.  For now, I'm not quite sure what
> to do about that.  We set the plan pathkeys well before we could
> possibly decide if asking for pre-sorted input for the aggregates
> would be a good idea or not.

I was curious about the performance implication of that additional transition, 
and could not reproduce a signifcant difference. I may be doing something 
wrong: how did you highlight it ?

Regards,

--
Ronan Dunklau

Re: Teach pg_receivewal to use lz4 compression

[gkokolatos]

‐‐‐ Original Message ‐‐‐

On Friday, July 2nd, 2021 at 03:10, Michael Paquier  wrote:

> On Thu, Jul 01, 2021 at 02:10:17PM +, gkokola...@pm.me wrote:
>
> > Micheal suggested on the same thread to move my entry in the help output so 
> > that
> >
> > the output remains ordered. I would like the options for the compression 
> > method and
> >
> > the already existing compression level to next to each other if possible. 
> > Then it
> >
> > should be either 'X' or 'Y'.
>
> Hmm. Grouping these together makes sense for the user. One choice
>
> that we have here is to drop the short option, and just use a long
>
> one. What I think is important for the user when it comes to this
>
> option is the consistency of its naming across all the tools
>
> supporting it. pg_dump and pg_basebackup, where we could plug LZ4,
>
> already use most of the short options you could use for pg_receivewal,
>
> having only a long one gives a bit more flexibility.


Good point. I am going with that one.


> --
>
> Michael

Re: row filtering for logical replication

[Peter Smith]

Hi.

I have been looking at the latest patch set (v16). Below are my review
comments and some patches.

The patches are:
v16-0001. This is identical to your previously posted 0001 patch. (I
only attached it again hoping it can allow the cfbot to keep working
OK).
v16-0002,0003. These are for demonstrating some of the review comments
v16-0004. This is a POC plan cache for your consideration.

//

REVIEW COMMENTS
===

1. Patch 0001 comment - typo

you can optionally filter rows that does not satisfy a WHERE condition

typo: does/does

~~

2. Patch 0001 comment - typo

The WHERE clause should probably contain only columns that are part of
the primary key or that are covered by REPLICA IDENTITY. Otherwise,
and DELETEs won't be replicated.

typo: "Otherwise, and DELETEs" ??

~~

3. Patch 0001 comment - typo and clarification

If your publication contains partitioned table, the parameter
publish_via_partition_root determines if it uses the partition row filter (if
the parameter is false -- default) or the partitioned table row filter.

Typo: "contains partitioned table" -> "contains a partitioned table"

Also, perhaps the text "or the partitioned table row filter." should
say "or the root partitioned table row filter." to disambiguate the
case where there are more levels of partitions like A->B->C. e.g. What
filter does C use?

~~

4. src/backend/catalog/pg_publication.c - misleading names

-publication_add_relation(Oid pubid, Relation targetrel,
+publication_add_relation(Oid pubid, PublicationRelationInfo *targetrel,
  bool if_not_exists)

Leaving this parameter name as "targetrel" seems a bit misleading now
in the function code. Maybe this should be called something like "pri"
which is consistent with other places where you have declared
PublicationRelationInfo.

Also, consider declaring some local variables so that the patch may
have less impact on existing code. e.g.
Oid relid = pri->relid
Relation *targetrel = relationinfo->relation

~~

5. src/backend/commands/publicationcmds.c - simplify code

- rels = OpenTableList(stmt->tables);
+ if (stmt->tableAction == DEFELEM_DROP)
+ rels = OpenTableList(stmt->tables, true);
+ else
+ rels = OpenTableList(stmt->tables, false);

Consider writing that code more simply as just:

rels = OpenTableList(stmt->tables, stmt->tableAction == DEFELEM_DROP);

~~

6. src/backend/commands/publicationcmds.c - bug?

- CloseTableList(rels);
+ CloseTableList(rels, false);
 }

Is this a potential bug? When you called OpenTableList the 2nd param
was maybe true/false, so is it correct to be unconditionally false
here? I am not sure.

~~

7. src/backend/commands/publicationcmds.c - OpenTableList function comment.

  * Open relations specified by a RangeVar list.
+ * AlterPublicationStmt->tables has a different list element, hence, is_drop
+ * indicates if it has a RangeVar (true) or PublicationTable (false).
  * The returned tables are locked in ShareUpdateExclusiveLock mode in order to
  * add them to a publication.

I am not sure about this. Should that comment instead say "indicates
if it has a Relation (true) or PublicationTable (false)"?

~~

8. src/backend/commands/publicationcmds.c - OpenTableList

- RangeVar   *rv = castNode(RangeVar, lfirst(lc));
- bool recurse = rv->inh;
+ PublicationTable *t = NULL;
+ RangeVar   *rv;
+ bool recurse;
  Relation rel;
  Oid myrelid;

+ if (is_drop)
+ {
+ rv = castNode(RangeVar, lfirst(lc));
+ }
+ else
+ {
+ t = lfirst(lc);
+ rv = castNode(RangeVar, t->relation);
+ }
+
+ recurse = rv->inh;
+

For some reason it feels kind of clunky to me for this function to be
processing the list differently according to the 2nd param. e.g. the
name "is_drop" seems quite unrelated to the function code, and more to
do with where it was called from. Sorry, I don't have any better ideas
for improvement atm.

~~

9. src/backend/commands/publicationcmds.c - OpenTableList bug?

- rels = lappend(rels, rel);
+ pri = palloc(sizeof(PublicationRelationInfo));
+ pri->relid = myrelid;
+ pri->relation = rel;
+ if (!is_drop)
+ pri->whereClause = t->whereClause;
+ rels = lappend(rels, pri);

I felt maybe this is a possible bug here because there seems no code
explicitly assigning the whereClause = NULL  if "is_drop" is true so
maybe it can have a garbage value which could cause problems later.
Maybe this is fixed by using palloc0.

Same thing is 2x in this function.

~~

10. src/backend/commands/publicationcmds.c - CloseTableList function comment

@@ -587,16 +609,28 @@ OpenTableList(List *tables)
  * Close all relations in the list.
  */
 static void
-CloseTableList(List *rels)
+CloseTableList(List *rels, bool is_drop)
 {

Probably the meaning of "is_drop" should be described in this function comment.

~~

11. src/backend/replication/pgoutput/pgoutput.c - get_rel_sync_entry signature.

-static RelationSyncEntry *get_rel_sync_entry(PGOutputData *data, Oid relid);
+static RelationSyncEntry *get_rel_sync_entry(PGOutputData *data, Relation rel);

I see that this fu

Re: Refactor "mutually exclusive options" error reporting code in parse_subscription_options

[Amit Kapila]

On Fri, Jul 2, 2021 at 8:35 AM Alvaro Herrera  wrote:
>
> > The latest patch sent by Bharath looks good to me. Would you like to
> > commit it or shall I take care of it?
>
> Please, go ahead.
>

Okay, I'll push it early next week (by Tuesday) unless there are more
comments or suggestions. Thanks!


-- 
With Regards,
Amit Kapila.