Re: Should smgrdounlink() be removed?
On Fri, May 08, 2020 at 09:21:25PM -0700, Peter Geoghegan wrote: > Fine with me. Thanks, Peter. Done. -- Michael signature.asc Description: PGP signature
Re: [PATCH] Incremental sort (was: PoC: Partial sort)
On Sat, May 9, 2020 at 3:19 PM Tomas Vondra wrote: > I'm generally OK with most of this - I'd probably keep the single-line > format, but I don't feel very strongly about that and if others think > using two lines is better ... > > Barring objections I'll get this polished and pushed soon-ish (say, > early next week). I see something about starting a new thread on the Open Items page. Please CC me on this. Speaking in my capacity as an RMT member: Glad to see that you plan to close this item out next week. (I had planned on giving you a nudge about this, but it looks like I don't really have to now.) Thanks -- Peter Geoghegan
Re: Add -Wold-style-definition to CFLAGS?
On Sat, May 09, 2020 at 07:11:56PM -0400, Tom Lane wrote: > I'd be OK with pushing it now, but I dunno about other people. Sounds like a good idea to me to apply this part now. > If we do want to push this sort of thing now, the nearby changes > to enable fallthrough warnings should go in too. If we do that, merging this second part before beta1 is out looks like a good compromise to me. -- Michael signature.asc Description: PGP signature
Re: Add -Wold-style-definition to CFLAGS?
Andres Freund writes: > On 2020-05-09 14:15:01 -0400, Tom Lane wrote: >> Andres Freund writes: >>> Since gcc has a warning detecting such definition, I think we ought to >>> automatically add it when available? >> +1 > Any opinion about waiting for branching or not? I'd be OK with pushing it now, but I dunno about other people. If we do want to push this sort of thing now, the nearby changes to enable fallthrough warnings should go in too. regards, tom lane
Re: [PATCH] Fix division by zero (explain.c)
Em sáb., 9 de mai. de 2020 às 17:48, Tomas Vondra <
tomas.von...@2ndquadrant.com> escreveu:
> On Sat, May 09, 2020 at 02:51:50PM -0300, Ranier Vilela wrote:
> >Em sáb., 9 de mai. de 2020 às 14:44, Tomas Vondra <
> >tomas.von...@2ndquadrant.com> escreveu:
> >
> >> On Sat, May 09, 2020 at 06:48:59AM -0300, Ranier Vilela wrote:
> >> >Em sáb., 9 de mai. de 2020 às 01:45, Tom Lane
> >> escreveu:
> >> >
> >> >> James Coleman writes:
> >> >> > There are always full sort groups before any prefix groups can
> happen,
> >> >> > so we know (even though the tooling doesn't) that the 2nd test can
> >> >> > never contradict the first.
> >> >>
> >> >> So maybe an assertion enforcing that would be appropriate?
> >> >> Untested, but:
> >> >>
> >> >> - if (fullsortGroupInfo->groupCount == 0 &&
> >> >> - prefixsortGroupInfo->groupCount == 0)
> >> >> + if (fullsortGroupInfo->groupCount == 0)
> >> >> + {
> >> >> +
> Assert(prefixsortGroupInfo->groupCount
> >> ==
> >> >> 0);
> >> >> continue;
> >> >> + }
> >> >>
> >> >I agree, asserts always help.
> >> >
> >>
> >> That doesn't work, because the prefixSortGroupInfo is used before
> >> assignment, producing compile-time warnings.
> >>
> >> I've pushed a simpler fix without the assert. If we want to make this
> >> check, perhaps doing it in incremental sort itself would be better than
> >> doing it in explain.
> >>
> >Thanks anyway for the commit.
> >But if you used the first version of my patch, would the author be me and
> >am I as reported?
> >What does it take to be considered the author?
> >
>
> Apologies. I should have listed you as an author, not just in the
> reported-by field.
>
Apologies accepted.
Thank you.
Ranier VIilela
Re: [PATCH] Incremental sort (was: PoC: Partial sort)
On Sat, May 09, 2020 at 03:18:36PM -0500, Justin Pryzby wrote: Checking if it's possible to address this Opened Item before 13b1. https://wiki.postgresql.org/wiki/PostgreSQL_13_Open_Items consistency of explain output: two spaces, equals vs colons, semicolons (incremental sort) Yes. Now that the other items related to incremental sort are fixed, this is next on my TODO. regards -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: [PATCH] Fix division by zero (explain.c)
On Sat, May 09, 2020 at 02:51:50PM -0300, Ranier Vilela wrote:
Em sáb., 9 de mai. de 2020 às 14:44, Tomas Vondra <
tomas.von...@2ndquadrant.com> escreveu:
On Sat, May 09, 2020 at 06:48:59AM -0300, Ranier Vilela wrote:
>Em sáb., 9 de mai. de 2020 às 01:45, Tom Lane
escreveu:
>
>> James Coleman writes:
>> > There are always full sort groups before any prefix groups can happen,
>> > so we know (even though the tooling doesn't) that the 2nd test can
>> > never contradict the first.
>>
>> So maybe an assertion enforcing that would be appropriate?
>> Untested, but:
>>
>> - if (fullsortGroupInfo->groupCount == 0 &&
>> - prefixsortGroupInfo->groupCount == 0)
>> + if (fullsortGroupInfo->groupCount == 0)
>> + {
>> + Assert(prefixsortGroupInfo->groupCount
==
>> 0);
>> continue;
>> + }
>>
>I agree, asserts always help.
>
That doesn't work, because the prefixSortGroupInfo is used before
assignment, producing compile-time warnings.
I've pushed a simpler fix without the assert. If we want to make this
check, perhaps doing it in incremental sort itself would be better than
doing it in explain.
Thanks anyway for the commit.
But if you used the first version of my patch, would the author be me and
am I as reported?
What does it take to be considered the author?
Apologies. I should have listed you as an author, not just in the
reported-by field.
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: [PATCH] Incremental sort (was: PoC: Partial sort)
Checking if it's possible to address this Opened Item before 13b1. https://wiki.postgresql.org/wiki/PostgreSQL_13_Open_Items consistency of explain output: two spaces, equals vs colons, semicolons (incremental sort) On Sun, Apr 19, 2020 at 09:46:55AM -0400, James Coleman wrote: > On Sat, Apr 18, 2020 at 10:36 PM Justin Pryzby wrote: > > > > On Tue, Apr 07, 2020 at 10:53:05AM -0500, Justin Pryzby wrote: > > > On Tue, Apr 07, 2020 at 08:40:30AM -0400, James Coleman wrote: > > > > > And, should it use two spaces before "Sort Method", "Memory" and > > > > > "Pre-sorted > > > ... > > > > I read through that subthread, and the ending seemed to be Peter > > > > wanting things to be unified. Was there a conclusion beyond that? > > > > > > This discussion is ongoing. I think let's wait until that's settled > > > before > > > addressing this more complex and even newer case. We can add "explain, > > > two > > > spaces and equals vs colon" to the "Open items" list if need be - I hope > > > the > > > discussion will not delay the release. > > > > The change proposed on the WAL thread is minimal, and makes new explain(WAL) > > output consistent with the that of explain(BUFFERS). > > > > That uses a different format from "Sort", which is what incremental sort > > should > > follow. (Hashjoin also uses the Sort's format of two-spaces and colons > > rather > > than equals). > > I think it's not great that buffers/sort are different, but I agree > that we should follow sort. > > > So the attached 0001 makes explain output for incremental sort more > > consistent > > with sort: > > > > - Two spaces; > > - colons rather than equals; > > - Don't use semicolon, which isn't in use anywhere else; > > > > I tested with this: > > template1=# DROP TABLE t; CREATE TABLE t(i int, j int); INSERT INTO t > > SELECT a-(a%100), a%1000 FROM generate_series(1,9)a; CREATE INDEX ON > > t(i); VACUUM VERBOSE ANALYZE t; > > template1=# explain analyze SELECT * FROM t a ORDER BY i,j; > > ... > >Full-sort Groups: 1000 Sort Method: quicksort Average Memory: 28kB > > Peak Memory: 28kB Pre-sorted Groups: 1000 Sort Method: quicksort Average > > Memory: 30kB Peak Memory: 30kB > > > > On Tue, Apr 07, 2020 at 05:34:15PM +0200, Tomas Vondra wrote: > > > On Tue, Apr 07, 2020 at 08:40:30AM -0400, James Coleman wrote: > > > > On Tue, Apr 7, 2020 at 12:25 AM Justin Pryzby > > > > wrote: > > > > > Should "Pre-sorted Groups:" be on a separate line ? > > > > > | Full-sort Groups: 1 Sort Method: quicksort Memory: avg=28kB > > > > > peak=28kB Pre-sorted Groups: 1 Sort Method: quicksort Memory: > > > > > avg=30kB peak=30kB > > > > > > > > I'd originally had that, but Tomas wanted it to be more compact. It's > > > > easy to adjust though if the consensus changes on that. > > > > > > I'm OK with changing the format if there's a consensus. The current > > > format seemed better to me, but I'm not particularly attached to it. > > > > I still think Pre-sorted groups should be on a separate line, as in 0002. > > In addition to looking better (to me), and being easier to read, another > > reason > > is that there are essentially key=>values here, but the keys are repeated > > (Sort > > Method, etc). > > I collapsed this into 0001 because I think that if we're going to do > away with the key=value style then we effectively to have to do this > to avoid the repeated values being confusing (with key=value it kinda > worked, because that made it seem like the avg/peak were clearly a > subset of the Sort Groups info). > > I also cleaned up the newline patch a bit in the process (we already > have a way to indent with a flag so don't need to do it directly). > > > I also suggested to rename: s/Presorted/Pre-sorted/, but I didn't do that > > here. > > I went ahead and did that too; we already use "Full-sort", so the > proposed change makes both parallel. > > > Michael already patched most of the comment typos, the remainder I'm > > including > > here as a "nearby patch".. > > Modified slightly. > > James > From becd60ba348558fa241db5cc2100a84b757cbdc5 Mon Sep 17 00:00:00 2001 > From: Justin Pryzby > Date: Mon, 6 Apr 2020 17:37:31 -0500 > Subject: [PATCH v2 2/2] comment typos: Incremental Sort > > commit d2d8a229bc58a2014dce1c7a4fcdb6c5ab9fb8da > Author: Tomas Vondra > > Previously reported here: > https://www.postgresql.org/message-id/20200407042521.GH2228%40telsasoft.com > --- > src/backend/commands/explain.c | 4 ++-- > src/backend/executor/nodeIncrementalSort.c | 10 ++ > src/backend/utils/sort/tuplesort.c | 8 > 3 files changed, 12 insertions(+), 10 deletions(-) > > diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c > index 5f91c569a0..86c10458f4 100644 > --- a/src/backend/commands/explain.c > +++ b/src/backend/commands/explain.c > @@ -2865,7 +2865,7 @@ > show_incremental_sort_group_info(IncrementalSortGroupInfo *groupInfo, > } > >
Re: Add -Wold-style-definition to CFLAGS?
Hi, On 2020-05-09 14:15:01 -0400, Tom Lane wrote: > Andres Freund writes: > > Since gcc has a warning detecting such definition, I think we ought to > > automatically add it when available? > > +1 Any opinion about waiting for branching or not? - Andres
Re: Logical replication subscription owner
On Fri, 8 May 2020 at 03:03, Kyotaro Horiguchi wrote: > > A user can start physical replication without needing CONNECT on any > database if it has REPLICATION attribute. That means any user that > is allowed logical replication on a specific database (or even no > databases) can replicate the whole cluster using physical replication. > I don't think it is a proper behavior from the security perspective. > > Physical replication has a special entry in pg_hba.conf, hence, I don't think you need CONNECT on all databases. However, logical replication uses the same entry from a regular connection and I concur with Michael and Stephen that we should have LOGIN and REPLICATION privileges in those cases. If we drop the LOGIN requirement for logical replication, it means that a simple NOLOGIN won't be sufficient to block a certain role to execute queries because "replication=database" could be used to bypass it. Physical replication can't execute queries but logical replication can. IMO REPLICATION is an additional capability and it is not a superset that contains LOGIN. I prefer a fine-grained control. In sections 26.2.5.1 and 30.7, LOGIN are documented accordingly. I'm +0.5 to the idea of adding a WARNING when you create/alter a role that has REPLICATION but not LOGIN. -- Euler Taveira http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Logical replication subscription owner
Stephen Frost writes: > * Tom Lane (t...@sss.pgh.pa.us) wrote: >> ISTM those statements are contradictory. The two privileges could >> only be called orthogonal if it's possible to make use of one without >> having the other. As things stand, REPLICATION without LOGIN is an >> entirely useless setting. > Allowing a login to the system by a role that doesn't have the LOGIN > privilege isn't sensible though. The fundamental issue here is whether a replication connection is a "login". I'd argue that it is not; "login" ought to mean a normal SQL connection. I realize that a replication connection can issue SQL commands (which, as I recall, Robert has blasted as a crappy design --- and I agree). But it's already the case that a replication connection has much greater privileges than plain SQL, so I don't think that that aspect ought to compel us to design the privilege bits as they are set up now. If you think that LOGIN should be required to issue SQL commands, then shouldn't doing SET ROLE to a non-LOGIN role disable your ability to issue SQL? > Perhaps a middle ground would be to set LOGIN on a role when REPLICATION > is set on it, if it's not already set (maybe with a NOTICE or WARNING or > such saying "also enabling LOGIN for role X", or maybe not if people > really think it should be obvious). It seems to me that there's value in having a role that can only connect for replication purposes and not as a regular SQL user. The existing definition doesn't support that, and the rather silly kluge you're proposing doesn't fix it. regards, tom lane
Re: Add -Wold-style-definition to CFLAGS?
Andres Freund writes: > Since gcc has a warning detecting such definition, I think we ought to > automatically add it when available? +1 regards, tom lane
Re: Another modest proposal for docs formatting: catalog descriptions
Fabien COELHO writes: > Possibly. I'm a little at odds with Type not being above types, but far on > the left, so that you cannot really "see" that it is about the format, Yeah, agreed. We can adjust the space in the header independently of what's in the table entries, so it'd be possible to put more space between "Column" and "Type" ... but I'm not sure if that would fix it. > If I can suggest more adjustements, maybe the description margin is a too > much, I'd propose reduce it to about 3 chars wide. Obviously any aesthetic > opinion is by definition subjective and prone to differ from one person to > the next… This is more Jonathan's department than mine, but I personally prefer more indentation to less --- you want the column names to stick out so you can find them. Anyway, the present indentation is (it looks like) the same as we are using in s, which this layout is based on. regards, tom lane
Re: Logical replication subscription owner
Greetings, * Tom Lane (t...@sss.pgh.pa.us) wrote: > Michael Paquier writes: > > Not to make the life of everybody more complicated here, but I don't > > agree. LOGIN and REPLICATION are in my opinion completely orthogonal > > and it sounds more natural IMO that a REPLICATION user should be able > > to log into the server only if it has LOGIN defined. > > ISTM those statements are contradictory. The two privileges could > only be called orthogonal if it's possible to make use of one without > having the other. As things stand, REPLICATION without LOGIN is an > entirely useless setting. Allowing a login to the system by a role that doesn't have the LOGIN privilege isn't sensible though. Perhaps a middle ground would be to set LOGIN on a role when REPLICATION is set on it, if it's not already set (maybe with a NOTICE or WARNING or such saying "also enabling LOGIN for role X", or maybe not if people really think it should be obvious). I don't think taking away login should take away replication though as maybe there's some reason why someone would want that, nor should we take away login if replication is taken away, this would strictly just be a change for when REPLICATION is added to a role that doesn't have LOGIN already. Thanks, Stephen signature.asc Description: PGP signature
Re: [PATCH] Fix division by zero (explain.c)
Em sáb., 9 de mai. de 2020 às 14:44, Tomas Vondra <
tomas.von...@2ndquadrant.com> escreveu:
> On Sat, May 09, 2020 at 06:48:59AM -0300, Ranier Vilela wrote:
> >Em sáb., 9 de mai. de 2020 às 01:45, Tom Lane
> escreveu:
> >
> >> James Coleman writes:
> >> > There are always full sort groups before any prefix groups can happen,
> >> > so we know (even though the tooling doesn't) that the 2nd test can
> >> > never contradict the first.
> >>
> >> So maybe an assertion enforcing that would be appropriate?
> >> Untested, but:
> >>
> >> - if (fullsortGroupInfo->groupCount == 0 &&
> >> - prefixsortGroupInfo->groupCount == 0)
> >> + if (fullsortGroupInfo->groupCount == 0)
> >> + {
> >> + Assert(prefixsortGroupInfo->groupCount
> ==
> >> 0);
> >> continue;
> >> + }
> >>
> >I agree, asserts always help.
> >
>
> That doesn't work, because the prefixSortGroupInfo is used before
> assignment, producing compile-time warnings.
>
> I've pushed a simpler fix without the assert. If we want to make this
> check, perhaps doing it in incremental sort itself would be better than
> doing it in explain.
>
Thanks anyway for the commit.
But if you used the first version of my patch, would the author be me and
am I as reported?
What does it take to be considered the author?
regards,
Ranier Vilela
Add -Wold-style-definition to CFLAGS?
Hi,
ISTM that it's our coding style that we use
something
my_paramless_func(void)
{
...
}
definitions rather than omitting the (void), which makes the function
look like an old-style function declaration. I somewhat regularly notice
such omissions during review, and fix them.
Since gcc has a warning detecting such definition, I think we ought to
automatically add it when available?
The attached patch makes configure do so, and also fixes a handful of
uses that crept in.
Greetings,
Andres Freund
>From 21ed43b0b0b57c29e31973d1fea0e3ed2f93a4d4 Mon Sep 17 00:00:00 2001
From: Andres Freund
Date: Sat, 9 May 2020 10:38:02 -0700
Subject: [PATCH] Add -Wold-style-definition to CFLAGS.
Author:
Reviewed-By:
Discussion: https://postgr.es/m/
Backpatch:
---
src/backend/postmaster/autovacuum.c | 2 +-
src/backend/storage/buffer/freelist.c | 2 +-
src/backend/utils/adt/jsonpath_scan.l | 2 +-
src/backend/utils/misc/queryenvironment.c | 2 +-
src/interfaces/libpq/fe-misc.c| 2 +-
configure | 41 +++
configure.in | 2 ++
7 files changed, 48 insertions(+), 5 deletions(-)
diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index 7e97ffab27d..5641f59016d 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -834,7 +834,7 @@ HandleAutoVacLauncherInterrupts(void)
* Perform a normal exit from the autovac launcher.
*/
static void
-AutoVacLauncherShutdown()
+AutoVacLauncherShutdown(void)
{
ereport(DEBUG1,
(errmsg("autovacuum launcher shutting down")));
diff --git a/src/backend/storage/buffer/freelist.c b/src/backend/storage/buffer/freelist.c
index aa5539a2e45..942f8d4edd2 100644
--- a/src/backend/storage/buffer/freelist.c
+++ b/src/backend/storage/buffer/freelist.c
@@ -177,7 +177,7 @@ ClockSweepTick(void)
* should not call this.
*/
bool
-have_free_buffer()
+have_free_buffer(void)
{
if (StrategyControl->firstFreeBuffer >= 0)
return true;
diff --git a/src/backend/utils/adt/jsonpath_scan.l b/src/backend/utils/adt/jsonpath_scan.l
index be0a2cfa2f7..df56a268d62 100644
--- a/src/backend/utils/adt/jsonpath_scan.l
+++ b/src/backend/utils/adt/jsonpath_scan.l
@@ -330,7 +330,7 @@ static const JsonPathKeyword keywords[] = {
/* Check if current scanstring value is a keyword */
static enum yytokentype
-checkKeyword()
+checkKeyword(void)
{
int res = IDENT_P;
int diff;
diff --git a/src/backend/utils/misc/queryenvironment.c b/src/backend/utils/misc/queryenvironment.c
index c0b85ed0b36..31de81f353e 100644
--- a/src/backend/utils/misc/queryenvironment.c
+++ b/src/backend/utils/misc/queryenvironment.c
@@ -36,7 +36,7 @@ struct QueryEnvironment
QueryEnvironment *
-create_queryEnv()
+create_queryEnv(void)
{
return (QueryEnvironment *) palloc0(sizeof(QueryEnvironment));
}
diff --git a/src/interfaces/libpq/fe-misc.c b/src/interfaces/libpq/fe-misc.c
index 19729f96311..9afa0533a62 100644
--- a/src/interfaces/libpq/fe-misc.c
+++ b/src/interfaces/libpq/fe-misc.c
@@ -1250,7 +1250,7 @@ PQenv2encoding(void)
#ifdef ENABLE_NLS
static void
-libpq_binddomain()
+libpq_binddomain(void)
{
static bool already_bound = false;
diff --git a/configure b/configure
index 83abe872aa6..b5bd3b2e9c0 100755
--- a/configure
+++ b/configure
@@ -5321,6 +5321,47 @@ if test x"$pgac_cv_prog_CC_cflags__Wdeclaration_after_statement" = x"yes"; then
fi
+ # Not applicable for C++
+
+{ $as_echo "$as_me:${as_lineno-$LINENO}: checking whether ${CC} supports -Wold-style-definition, for CFLAGS" >&5
+$as_echo_n "checking whether ${CC} supports -Wold-style-definition, for CFLAGS... " >&6; }
+if ${pgac_cv_prog_CC_cflags__Wold_style_definition+:} false; then :
+ $as_echo_n "(cached) " >&6
+else
+ pgac_save_CFLAGS=$CFLAGS
+pgac_save_CC=$CC
+CC=${CC}
+CFLAGS="${CFLAGS} -Wold-style-definition"
+ac_save_c_werror_flag=$ac_c_werror_flag
+ac_c_werror_flag=yes
+cat confdefs.h - <<_ACEOF >conftest.$ac_ext
+/* end confdefs.h. */
+
+int
+main ()
+{
+
+ ;
+ return 0;
+}
+_ACEOF
+if ac_fn_c_try_compile "$LINENO"; then :
+ pgac_cv_prog_CC_cflags__Wold_style_definition=yes
+else
+ pgac_cv_prog_CC_cflags__Wold_style_definition=no
+fi
+rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext
+ac_c_werror_flag=$ac_save_c_werror_flag
+CFLAGS="$pgac_save_CFLAGS"
+CC="$pgac_save_CC"
+fi
+{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $pgac_cv_prog_CC_cflags__Wold_style_definition" >&5
+$as_echo "$pgac_cv_prog_CC_cflags__Wold_style_definition" >&6; }
+if test x"$pgac_cv_prog_CC_cflags__Wold_style_definition" = x"yes"; then
+ CFLAGS="${CFLAGS} -Wold-style-definition"
+fi
+
+
# -Wdeclaration-after-statement isn't applicable for C++. Specific C files
# disable it, so AC_SUBST the negative form.
PERMIT_DECLARATION_AFTER_STATEMENT=
diff --git a/configure.in b/configure.in
index ecdf1723967..2b0a88ab911 100644
--- a/configure.in
+++
Re: Incremental sorts and EXEC_FLAG_REWIND
On Fri, May 08, 2020 at 07:36:38PM -0400, James Coleman wrote: On Fri, May 8, 2020 at 7:14 PM Tomas Vondra wrote: On Fri, Apr 24, 2020 at 04:35:02PM -0400, James Coleman wrote: >On Sun, Apr 19, 2020 at 12:14 PM James Coleman wrote: >> >> On Wed, Apr 15, 2020 at 2:04 PM James Coleman wrote: >> > >> > On Wed, Apr 15, 2020 at 11:02 AM James Coleman wrote: >> > > >> > > On Tue, Apr 14, 2020 at 2:53 AM Michael Paquier wrote: >> > > > >> > > > Hi, >> > > > >> > > > When initializing an incremental sort node, we have the following as >> > > > of ExecInitIncrementalSort(): >> > > > /* >> > > > * Incremental sort can't be used with either EXEC_FLAG_REWIND, >> > > > * EXEC_FLAG_BACKWARD or EXEC_FLAG_MARK, because we only one of many sort >> > > > * batches in the current sort state. >> > > > */ >> > > > Assert((eflags & (EXEC_FLAG_BACKWARD | >> > > >EXEC_FLAG_MARK)) == 0); >> > > > While I don't quite follow why EXEC_FLAG_REWIND should be allowed here >> > > > to begin with (because incremental sorts don't support rescans without >> > > > parameter changes, right?), the comment and the assertion are telling >> > > > a different story. >> > > >> > > I remember changing this assertion in response to an issue I'd found >> > > which led to rewriting the rescan implementation, but I must have >> > > missed updating the comment. >> > >> > All right, here are the most relevant messages: >> > >> > [1]: Here I'd said: >> > -- >> > While working on finding a test case to show rescan isn't implemented >> > properly yet, I came across a bug. At the top of >> > ExecInitIncrementalSort, we assert that eflags does not contain >> > EXEC_FLAG_REWIND. But the following query (with merge and hash joins >> > disabled) breaks that assertion: >> > >> > select * from t join (select * from t order by a, b) s on s.a = t.a >> > where t.a in (1,2); >> > >> > The comments about this flag in src/include/executor/executor.h say: >> > >> > * REWIND indicates that the plan node should try to efficiently support >> > * rescans without parameter changes. (Nodes must support ExecReScan calls >> > * in any case, but if this flag was not given, they are at liberty to do it >> > * through complete recalculation. Note that a parameter change forces a >> > * full recalculation in any case.) >> > >> > Now we know that except in rare cases (as just discussed recently up >> > thread) we can't implement rescan efficiently. >> > >> > So is this a planner bug (i.e., should we try not to generate >> > incremental sort plans that require efficient rewind)? Or can we just >> > remove that part of the assertion and know that we'll implement the >> > rescan, albeit inefficiently? We already explicitly declare that we >> > don't support backwards scanning, but I don't see a way to declare the >> > same for rewind. >> > -- >> > >> > So it seems to me that we can't disallow REWIND, and we have to >> > support rescan, but, we could try to mitigate the effects (without a >> > param change) with a materialize node, as noted below. >> > >> > [2]: Here, in response to my questioning above if this was a planner >> > bug, I'd said: >> > -- >> > Other nodes seem to get a materialization node placed above them to >> > support this case "better". Is that something we should be doing? >> > -- >> > >> > I never got any reply on this point; if we _did_ introduce a >> > materialize node here, then it would mean we could start disallowing >> > REWIND again. See the email for full details of a specific plan that I >> > encountered that reproduced this. >> > >> > Thoughts? >> > >> > > In the meantime, your question is primarily about making sure the >> > > code/comments/etc. are consistent and not a behavioral problem or >> > > failure you've seen in testing? >> > >> > Still want to confirm this is the case. >> > >> > James >> > >> > [1]: https://www.postgresql.org/message-id/CAAaqYe9%2Bap2SbU_E2WaC4F9ZMF4oa%3DpJZ1NBwaKDMP6GFUA77g%40mail.gmail.com >> > [2]: https://www.postgresql.org/message-id/CAAaqYe-sOp2o%3DL7nvGZDJ6GsL9%3Db_ztrGE1rhyi%2BF82p3my2bQ%40mail.gmail.com >> >> Looking at this more, I think this is definitely suspect. The current >> code shields lower nodes from EXEC_FLAG_BACKWARD and EXEC_FLAG_MARK -- >> the former is definitely fine because we declare that we don't support >> backwards scans. The latter seems like the same reasoning would apply, >> but unfortunately we didn't add it to ExecSupportsMarkRestore, so I've >> attached a patch to do that. >> >> The EXEC_FLAG_REWIND situation though I'm still not clear on -- given >> the comments/docs seem to suggest it's a hint for efficiency rather >> than something that has to work or be declared as not implemented, so >> it seems like one of the following should be the outcome: >> >> 1. "Disallow" it by only generating materialize nodes above the >> incremental sort node if REWIND will be required. I'm not sure if this >>
Re: [PATCH] Fix division by zero (explain.c)
On Sat, May 09, 2020 at 06:48:59AM -0300, Ranier Vilela wrote:
Em sáb., 9 de mai. de 2020 às 01:45, Tom Lane escreveu:
James Coleman writes:
> There are always full sort groups before any prefix groups can happen,
> so we know (even though the tooling doesn't) that the 2nd test can
> never contradict the first.
So maybe an assertion enforcing that would be appropriate?
Untested, but:
- if (fullsortGroupInfo->groupCount == 0 &&
- prefixsortGroupInfo->groupCount == 0)
+ if (fullsortGroupInfo->groupCount == 0)
+ {
+ Assert(prefixsortGroupInfo->groupCount ==
0);
continue;
+ }
I agree, asserts always help.
That doesn't work, because the prefixSortGroupInfo is used before
assignment, producing compile-time warnings.
I've pushed a simpler fix without the assert. If we want to make this
check, perhaps doing it in incremental sort itself would be better than
doing it in explain.
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Another modest proposal for docs formatting: catalog descriptions
Hello Tom, Here's a more fully fleshed out draft for this, with stylesheet markup to get extra space around the column type names. I find this added spacing awkward, espacially as attribute names are always one word anyway. I prefer the non spaced approach. It's certainly arguable that that look is too heavy-handed. In the attached, I knocked down the extra space from 1em to 0.25em, which makes it quite a bit subtler --- are you any happier with this? Yes, definitely. BTW, I don't think it's very accurate that "attribute names are always one word" --- see the second attachment. Indeed. Here if anything I'm wanting a little more space. I'm fine with 0.25em which allows some breathing without looking awkward. Maybe a little more would still be okay, but not much more. If spacing is discussed, should the layout rather try to align type information, eg: I thought about that, but it seems extremely close to some of the earlier function-table layouts that were so widely panned. The SGML source would have to be a lot uglier too, probably with explicit use of spanspec's on every row. Hmmm, that's the kind of things I was afraid of. It could be done no doubt, but I think people would not see it as an improvement. Possibly. I'm a little at odds with Type not being above types, but far on the left, so that you cannot really "see" that it is about the format, especially with long attribute names: Column Type Description quite_a_long_attribute and_its_type ... The horizontal distance between "Type" and "and_its_type" is so wide as to hide the clue that the former is describing the later. But maybe aligning would be too ugly. If I can suggest more adjustements, maybe the description margin is a too much, I'd propose reduce it to about 3 chars wide. Obviously any aesthetic opinion is by definition subjective and prone to differ from one person to the next… -- Fabien.
Re: Logical replication subscription owner
Michael Paquier writes: > Not to make the life of everybody more complicated here, but I don't > agree. LOGIN and REPLICATION are in my opinion completely orthogonal > and it sounds more natural IMO that a REPLICATION user should be able > to log into the server only if it has LOGIN defined. ISTM those statements are contradictory. The two privileges could only be called orthogonal if it's possible to make use of one without having the other. As things stand, REPLICATION without LOGIN is an entirely useless setting. regards, tom lane
Re: +(pg_lsn, int8) and -(pg_lsn, int8) operators
On 2020/05/08 12:10, Kyotaro Horiguchi wrote:
At Fri, 8 May 2020 11:31:42 +0900, Fujii Masao
wrote in
You mean that pg_lsn_pli() and pg_lsn_mii() should emit an error like
"the number of bytes to add/subtract cannnot be NaN" when NaN is
specified?
The function is called while executing an expression, so "NaN cannot
be used in this expression" or something like that would work.
This sounds ambiguous. I like to use clearer messages like
cannot add NaN to pg_lsn
cannot subtract NaN from pg_lsn
They works fine to me.
Ok, I updated pg_lsn_pli() and pg_lsn_mii() so that they emit an error
when NaN is specified as the number of bytes.
I'm not sure if int128 is available in every environments.
In second thought, I found that we don't have enough substitute
functions for the platforms without a native implement. Instead,
there are some overflow-safe uint64 math functions, that is,
pg_add/sub_u64_overflow. This patch defines numeric_pg_lsn which is
substantially numeric_uint64. By using them, for example, we can make
pg_lsn_pli mainly with integer arithmetic as follows.
Sorry, I'm not sure what the benefit of this approach...
(If we don't allow negative nbytes,)
We accept numeric so that the operators can accept values out of range
of int64, but we don't need to perform all arithmetic in numeric. That
approach does less numeric arithmetic, that is, faster and simpler.
We don't need to string'ify LSN with it. That avoid stack consumption.
If invalid values are given as the addend, the following message would
make sense.
=# select '1/1::pg_lsn + 'NaN'::numeric;
ERROR: cannot use NaN in this expression
=# select '1/1::pg_lsn + '-1'::numeric;
ERROR: numeric value out of range for this expression
Could you tell me why we should reject this calculation?
IMO it's ok to add the negative number, and which is possible
with the latest patch.
Sorry, I misread the patch as it rejected -1 for *nbytes*, by seeing
numeric_pg_lsn.
Finally, I'm convinced that we lack required integer arithmetic
infrastructure to perform the objective.
The patch looks good to me except the size of buf[], but I don't
strongly object to that.
Ok, I changed the size of buf[] to 32.
Attached is the updated version of the patch.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
diff --git a/doc/src/sgml/datatype.sgml b/doc/src/sgml/datatype.sgml
index a8d0780387..a86b794ce0 100644
--- a/doc/src/sgml/datatype.sgml
+++ b/doc/src/sgml/datatype.sgml
@@ -4823,7 +4823,13 @@ SELECT * FROM pg_attribute
standard comparison operators, like = and
. Two LSNs can be subtracted using the
- operator; the result is the number of bytes separating
-those write-ahead log locations.
+those write-ahead log locations. Also the number of bytes can be
+added into and subtracted from LSN using the
++(pg_lsn,numeric) and
+-(pg_lsn,numeric) operators, respectively. Note that
+the calculated LSN should be in the range of pg_lsn type,
+i.e., between 0/0 and
+/.
diff --git a/src/backend/utils/adt/numeric.c b/src/backend/utils/adt/numeric.c
index 9986132b45..94593c7f63 100644
--- a/src/backend/utils/adt/numeric.c
+++ b/src/backend/utils/adt/numeric.c
@@ -41,6 +41,7 @@
#include "utils/guc.h"
#include "utils/int8.h"
#include "utils/numeric.h"
+#include "utils/pg_lsn.h"
#include "utils/sortsupport.h"
/* --
@@ -472,6 +473,7 @@ static void apply_typmod(NumericVar *var, int32 typmod);
static bool numericvar_to_int32(const NumericVar *var, int32 *result);
static bool numericvar_to_int64(const NumericVar *var, int64 *result);
static void int64_to_numericvar(int64 val, NumericVar *var);
+static bool numericvar_to_uint64(const NumericVar *var, uint64 *result);
#ifdef HAVE_INT128
static bool numericvar_to_int128(const NumericVar *var, int128 *result);
static void int128_to_numericvar(int128 val, NumericVar *var);
@@ -3688,6 +3690,30 @@ numeric_float4(PG_FUNCTION_ARGS)
}
+Datum
+numeric_pg_lsn(PG_FUNCTION_ARGS)
+{
+ Numeric num = PG_GETARG_NUMERIC(0);
+ NumericVar x;
+ XLogRecPtr result;
+
+ if (NUMERIC_IS_NAN(num))
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+errmsg("cannot convert NaN to pg_lsn")));
+
+ /* Convert to variable format and thence to pg_lsn */
+ init_var_from_num(num, );
+
+ if (!numericvar_to_uint64(, (uint64 *) ))
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+errmsg("pg_lsn out of range")));
+
+ PG_RETURN_LSN(result);
+}
+
+
/* --
*
* Aggregate functions
@@ -6739,6 +6765,78 @@ int64_to_numericvar(int64 val, NumericVar *var)
var->weight = ndigits -
Re: PG 13 release notes, first draft
On Fri, May 8, 2020 at 12:07 PM Amit Langote wrote: > On Fri, May 8, 2020 at 2:06 AM Bruce Momjian wrote: > > On Fri, May 8, 2020 at 12:32:16AM +0900, Amit Langote wrote: > > > c8434d64c implements a new feature whereby, to use partitionwise join, > > > partition bounds of the tables being joined no longer have to match > > > exactly. I think it might be better to mention this explicitly > > > because it enables partitionwise joins to be used in more partitioning > > > setups. > > > > Well, the text says: > > > > Allow partitionwise joins to happen in more cases (Ashutosh Bapat, > > Etsuro Fujita, Amit Langote, Tom Lane) > > > > Isn't that what you just said? I just added this paragraph: > > > > For example, partitionwise joins can now happen between partitioned > > tables where the ancestors do not exactly match. > > > > Does that help? > > Yes, although "ancestors do not exactly match" doesn't make clear what > about partitioned tables doesn't match. "partition bounds do not > exactly match" would. +1 for that change. Thank you for taking the time to this! Best regards, Etsuro Fujita
Re: [PATCH] Fix division by zero (explain.c)
Em sáb., 9 de mai. de 2020 às 01:45, Tom Lane escreveu:
> James Coleman writes:
> > There are always full sort groups before any prefix groups can happen,
> > so we know (even though the tooling doesn't) that the 2nd test can
> > never contradict the first.
>
> So maybe an assertion enforcing that would be appropriate?
> Untested, but:
>
> - if (fullsortGroupInfo->groupCount == 0 &&
> - prefixsortGroupInfo->groupCount == 0)
> + if (fullsortGroupInfo->groupCount == 0)
> + {
> + Assert(prefixsortGroupInfo->groupCount ==
> 0);
> continue;
> + }
>
I agree, asserts always help.
regards,
Ranier Vilela
fix_division_by_zero_explain_v2.patch
Description: Binary data
Re: Logical replication subscription owner
On Fri, May 08, 2020 at 03:03:26PM +0900, Kyotaro Horiguchi wrote: > At Fri, 8 May 2020 01:02:11 -0400, Alvaro Herrera > wrote in >> On 2020-May-07, Tom Lane wrote: >>> FWIW, I would argue that LOGIN permits logging in on a regular SQL >>> connection, while REPLICATION should permit logging in on a >>> replication connection, and there's no reason for either to depend on >>> or require the other. >> >> I agree with this. > > I agree, too. Anyway, it is unreasonable that a user is banned for > the lack of replication-attribute after a successful *replication* > login. Not to make the life of everybody more complicated here, but I don't agree. LOGIN and REPLICATION are in my opinion completely orthogonal and it sounds more natural IMO that a REPLICATION user should be able to log into the server only if it has LOGIN defined. -- Michael signature.asc Description: PGP signature
Re: stat() on Windows might cause error if target file is larger than 4GB
On Sat, May 9, 2020 at 2:49 AM Alvaro Herrera
wrote:
> On 2018-Sep-13, Tom Lane wrote:
>
> > What I was vaguely imagining is that win32_port.h could #include
> > whichever Windows header defines these functions and structs, and
> > then do
> >
> > #define stat __stat64
> >
> > static inline ... __stat64(...) { return _stat64(...); }
> >
> > What would need testing is whether the #define has nasty side-effects
> > even if we've already included the system header. I don't think it'd
> > hurt, eg, local variables named "stat"; though people might be surprised
> > when examining things in a debugger.
>
> Did anybody test this idea? It seems we let this problem slip unfixed,
> which means Windows users cannot use pg_dump -Fd (incl. parallel dump)
> when output files are large.
>
This issue gets reported from time to time as bug, it also affects COPY.
There is an open item for so:
https://commitfest.postgresql.org/28/2189/
Regards,
Juan José Santamaría Flecha
Re: Strange decreasing value of pg_last_wal_receive_lsn()
On Fri, May 08, 2020 at 03:02:26PM +0500, godjan • wrote: > Can you recommend what to use to determine which quorum standby > should be promoted in such case? > We planned to use pg_last_wal_receive_lsn() to determine which has > fresh data but if it returns the beginning of the segment on both > replicas we can’t determine which standby confirmed that write > transaction to disk. If you want to preserve transaction-level consistency across those notes, what is your configuration for synchronous_standby_names and synchronous_commit on the primary? Cannot you rely on that? -- Michael signature.asc Description: PGP signature
Re: pg_restore error message
On Fri, May 08, 2020 at 07:45:16PM -0400, Alvaro Herrera wrote: > Yeah, there's a lot of frontend code that uses free() instead of > pg_free(). There are too many of these that worrying about a single one > would not improve things much. I guess we could convert them all, but I > don't see much point. Doing a hard switch would have the disadvantage to create more problems when back-patching. Even if such conflicts would be I guess simple enough to address, that's less to worry about. I think however that there is a point in switching to a more PG-like API if reworking an area of the code for a new feature or a refactoring, but this is a case-by-case judgement usually. >> 2. %m, is a format to parameter, right? >> But what parameter? Both fatal call, do not pass this parameter, or is >> it implied? > > %m is an implied "strerror(errno)", implemented by our snprintf > replacement. Originally, %m is a glibc extension, which has been added recently in our port in src/port/snprintf.c as of d6c55de. -- Michael signature.asc Description: PGP signature
Re: Another modest proposal for docs formatting: catalog descriptions
Hello Tom, Here's a more fully fleshed out draft for this, with stylesheet markup to get extra space around the column type names. I find this added spacing awkward, espacially as attribute names are always one word anyway. I prefer the non spaced approach. If spacing is discussed, should the layout rather try to align type information, eg: attributetype description --- foo bla this does this and that ... and here is an example about it --- foo-foo-foo bla-bla whatever bla bla blah bla foo foo foo ... and stuff I'm not sure how achievable it is from a xml & processing point of view, and how desirable it is, I'm just throwing it for consideration. -- Fabien.
