Re: Remove useless associativity/precedence from parsers

[Akim Demaille]

Hi Tom,

> Le 21 mai 2019 à 21:06, Tom Lane  a écrit :
> 
> Akim Demaille  writes:
>>> Le 20 mai 2019 à 15:54, Tom Lane  a écrit :
>>> 2013?  Certainly not.  We have a lot of buildfarm critters running
>>> older platforms than that.
> 
>> This I fully understand.  However, Bison is a source generator,
>> and it's quite customary to use modern tools on the maintainer
>> side, and then deploy the result them on possibly much older
>> architectures.
>> Usually users of Bison build tarballs with the generated parsers
>> in them, and ship/test from that.
> 
> As do we, but at the same time we don't want to make our tool
> requirements too onerous.  I think that really the practical limit
> right now is Bison 2.3 --- Apple is still shipping that as their system
> version,

And do not expect Apple to update it at all.  Apple refuses the
GPLv3, and stopped updating Bison to the last GPLv2 release,
as it did for every GPL'd program.

> so requiring something newer than 2.3 would put extra burden
> on people doing PG development on Macs (of which there are a lot).

Honestly, I seriously doubt that you have contributors that don't
have MacPorts or Brew installed, and both are pretty up to date on
Bison.

>> So Bison, and your use of it today, is exactly what you need?
>> There's no limitation of that tool that you'd like to see
>> address that would make it a better tool for PostgreSQL?
> 
> Well, there are a couple of pain points, but they're not going to be
> addressed by marginal hacking on declarations ;-).  The things that
> we find really painful, IMV, are:
> 
> * Speed of the generated parser could be better.

Expect news this year about that.  I precisely came to look at
PostgreSQL for this.  Is there an easy way to bench pg and the various
costs?  To be explicit: is there a way to see how long the parsing
phase takes?  And some mighty inputs to bench against?

> I suspect this has
> a lot to do with the fact that our grammar is huge, and so are the
> tables, and that causes lots of cache misses.  Maybe this could be
> addressed by trying to make the tables smaller and/or laid out in
> a way with better cache locality?

The improvement I have in mind is about LR techniques, not about
this.  But you are right that it might be interesting.

It's unlikely that the table can be made smaller though.  Bison
was designed when space was really scarce, and a lot of efforts
were invested to make the tables as small as possible.  The
current trend is actually to optionally consume more space in
exchange for better services (such as more accurate error messages).


> * Lack of run-time extensibility of the parser.  There are many PG
> extensions that wish they could add things into the grammar, and can't.

Making the grammars extensible definitely makes sense, and it's
in the wishlist.  But making this doable at runtime is a much bigger
problem...

However, maybe this can be achieved by calling the plugin parser
from the outer parser.  Provided, of course, that the grammar of
the plugin is really in a "separate world"; if it also wants to
get bits of the host grammar, it's certainly not so easy.

Are there documented examples of this?  What would that look like?


> * LALR(1) parsing can only barely cope with SQL, and the standards
> committee keeps making it harder.

But Bison does more: it also provides support for LR(1) and IELR(1),
which accept more (deterministic) grammars, and are not subject
to "mysterious s/r conflicts" as in LALR.  But maybe you refer to
even beyond LR(1):

> We've got some hacks that fake
> an additional token of lookahead in some places, but that's just an
> ugly (and performance-eating) hack.

More than k=1 is unlikely to happen.  Given that we have GLR, which
provides us with k=∞ :)

> Maybe Bison's GLR mode would already solve that,

No doubt about that.

Bison's grammar is not LR(1) either, because the rules are not mandated
to end with ';', so when reading a grammar, in a sequence such as
" <:> " the parser cannot know whether the second  is the
RHS of the rule introduced by the first , or the beginning of another
rule if the sequence is actually " <:>  <:>".  Because of that,
Bison is also playing dirty tricks to turn this LR(2) into LR(1).

But as a consequence the grammar is much harder to evolve, the locations
are less accurate (because two tokens are merged together into a mega
token), etc.

So it is considered to turn Bison's own parser to GLR.

> but no one here has really looked into whether
> it could improve matters or whether it'd come at a performance cost.

That should be very easy to check: just adding %glr to the grammar
should not change the API, should not change the visible behavior,
but will give you a hint of the intrinsic cost of the GLR backend.

> The Bison manual's description of GLR doesn't give me a warm feeling
> either about the performance impact

The GLR backend is efficient... for a GLR backend.  I have not
benched it against the deterministic backend, but 

Excessive memory usage in multi-statement queries w/ partitioning

[Andreas Seltenreich]

Hi,

a customer reported excessive memory usage and out-of-memory ERRORs
after introducing native partitioning in one of their databases.  We
could narrow it down to the overhead introduced by the partitioning when
issuing multiple statements in a single query.  I could reduce the
problem to the following recipe:

--8<---cut here---start->8---
#!/bin/bash

# create 100 partitions
psql -c 'create table t(c int primary key) partition by range(c)'
for i in {1..100}; do
psql -e -c "create table t$i partition of t for values
 from ($(((i-1)*100))) to ($((i*100-1))) "
done

# artificially limit per-process memory by setting a resource limit for
# the postmaster to 256MB

prlimit -d$((256*1024*1024)) -p $POSTMASTER_PID
--8<---cut here---end--->8---

Now, updates to a partition are fine with 4000 update statements:

,
| $ psql -c "$(yes update t2 set c=c where c=6 \; | head -n 4000)"
| UPDATE 0
`

…but when doing it on the parent relation, even 100 statements are
enough to exceed the limit:

,
| $ psql -c "$(yes update t set c=c where c=6 \; | head -n 100)"
| FEHLER:  Speicher aufgebraucht
| DETAIL:  Failed on request of size 200 in memory context "MessageContext".
`

The memory context dump shows plausible values except for the MessageContext:

TopMemoryContext: 124336 total in 8 blocks; 18456 free (11 chunks); 105880 used
  [...]
  MessageContext: 264241152 total in 42 blocks; 264 free (0 chunks); 264240888 
used
  [...]

Maybe some tactically placed pfrees or avoiding putting redundant stuff
into MessageContext can relax the situation?

regards,
Andreas

Re: PostgreSQL 12 Beta 1 press release draft

[David Rowley]

On Thu, 23 May 2019 at 15:31, Jonathan S. Katz  wrote:
> Attached is
> v3 of the patch, along with a diff.

Minor details, but this query is not valid:

> WITH c AS MATERIALIZED (
>   SELECT * FROM a WHERE a.x % 4
> )
> SELECT * FROM c JOIN d ON d.y = a.x;

a.x % 4 is not a boolean clause, and "a" is not in the main query, so
a.x can't be referenced there.

How about:

WITH c AS MATERIALIZED (
SELECT * FROM a WHERE a.x % 4 = 0
)
SELECT * FROM c JOIN d ON d.y = c.x;

-- 
 David Rowley   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Re: nitpick about poor style in MergeAttributes

[Michael Paquier]

On Wed, May 22, 2019 at 06:20:01PM -0700, Mark Dilger wrote:
> What to do about this is harder to say.  In the following
> patch, I'm just doing what I think is standard for callers
> of list_delete_cell, and assigning the return value back
> to the list (similar to how a call to repalloc should do).
> But since there is an implicit assumption that the list
> is never emptied by this operation, perhaps checking
> against NIL and elog'ing makes more sense?

Yes, I agree that this is a bit fuzzy, and this code is new as of
705d433.  As you say, I agree that making sure that the return value
of list_delete_cell is not NIL is a sensible choice.

I don't think that an elog() is in place here though as this does not
rely directly on catalog contents, what about just an assertion?

Here is an idea of message for the elog(ERROR) if we go that way:
"no remaining columns after merging column \"%s\"".
--
Michael


signature.asc
Description: PGP signature

Re: Patch to fix write after end of array in hashed agg initialization

[Andrew Gierth]

> "Tom" == Tom Lane  writes:

 > Andrew Gierth  writes:
 >> My inclination is to fix this in the planner rather than the
 >> executor; there seems no good reason to actually hash a duplicate
 >> column more than once.

 Tom> Sounds reasonable --- but would it make sense to introduce some
 Tom> assertions, or other cheap tests, into the executor to check that
 Tom> it's not being given a case it can't handle?

Oh definitely, I was planning on it.

-- 
Andrew (irc:RhodiumToad)

Re: Patch to fix write after end of array in hashed agg initialization

[Tom Lane]

Andrew Gierth  writes:
> My inclination is to fix this in the planner rather than the executor;
> there seems no good reason to actually hash a duplicate column more than
> once.

Sounds reasonable --- but would it make sense to introduce some
assertions, or other cheap tests, into the executor to check that
it's not being given a case it can't handle?

regards, tom lane

Re: Remove useless associativity/precedence from parsers

[Tom Lane]

Robert Haas  writes:
> Another thing is that it would be nice to have a better way of
> resolving conflicts than attaching precedence declarations.  Some
> problems can't be solved that way at all, and others can only be
> solved that way at the risk of unforeseen side effects.

Yeah, we've definitely found that resolving shift/reduce conflicts via
precedence declarations has more potential for surprising side-effects
than one would think.  It feels to me that there's something basically
wrong with that concept, or at least wrong with the way we've used it.
Some relevant commits: 670a6c7a2, 12b716457, 6fe27ca2f, and the
"x NOT-something y" hacks in commit c6b3c939b (that one has a whole bunch
of other cruft in it, so it might be hard to spot what I'm talking about).

> One possible
> idea is a way to mark a rule %weak, meaning that it should only be
> used if no non-%weak rule could apply.  I'm not sure if that would
> really be the best way, but it's one idea.  A more general version
> would be some kind of ability to give rules different strengths; in
> the case of a grammar conflict, the "stronger" rule would win.

Hmmm ... not apparent to me that that's really going to help.
Maybe it will, but it sounds like more likely it's just another
mechanism with not-as-obvious-as-you-thought side effects.

regards, tom lane

nitpick about useless floating point division in gimme_edge_table

[Mark Dilger]

Hackers,

The return value of gimme_edge_table is not used anywhere in the
core code, so far as I can see.  But the value is computed as

  /* return average number of edges per index */
  return ((float) (edge_total * 2) / (float) num_gene);

which involves some floating point math.  I'm not sure that this matters
much, but (1) it deceives a reader of this code into thinking that this
calculation is meaningful, which it is not, and (2) gimme_edge_table is
called inside a loop, so this is happening repeatedly, though admittedly
that loop is perhaps not terribly large.

mark

Re: Patch to fix write after end of array in hashed agg initialization

[Andrew Gierth]

> "Andrew" == Andrew Gierth  writes:

 >>> Attached is a patch for a write after allocated memory which we
 >>> found in testing. Its an obscure case but can happen if the same
 >>> column is used in different grouping keys, as in the example below,
 >>> which uses tables from the regress test suite (build with
 >>> --enable-cassert in order to turn on memory warnings). Patch is
 >>> against master.

 Andrew> I'll look into it.

OK, so my first impression is that this is down to (a) the fact that
when planning a GROUP BY, we eliminate duplicate grouping keys; (b) due
to (a), the executor code isn't expecting to have to deal with
duplicates, but (c) when using a HashAgg to implement a Unique path, the
planner code isn't making any attempt to eliminate duplicates so they
get through.

It was wrong before commit b5635948, looks like Andres' fc4b3dea2 which
introduced the arrays and the concept of narrowing the stored tuples is
the actual culprit. But I'll deal with fixing it anyway unless Andres
has a burning desire to step in.

My inclination is to fix this in the planner rather than the executor;
there seems no good reason to actually hash a duplicate column more than
once.

-- 
Andrew (irc:RhodiumToad)

Re: Remove useless associativity/precedence from parsers

[Robert Haas]

On Tue, May 21, 2019 at 3:07 PM Tom Lane  wrote:
> Other PG hackers might have a different laundry list, but that's mine.

Good list.

Another thing is that it would be nice to have a better way of
resolving conflicts than attaching precedence declarations.  Some
problems can't be solved that way at all, and others can only be
solved that way at the risk of unforeseen side effects.  One possible
idea is a way to mark a rule %weak, meaning that it should only be
used if no non-%weak rule could apply.  I'm not sure if that would
really be the best way, but it's one idea.  A more general version
would be some kind of ability to give rules different strengths; in
the case of a grammar conflict, the "stronger" rule would win.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: PostgreSQL 12 Beta 1 press release draft

[Jonathan S. Katz]

Hi Ilmari,

On 5/22/19 6:52 PM, Dagfinn Ilmari Mannsåker wrote:
> Hi Jonathan,
> 
> "Jonathan S. Katz"  writes:
> 
>> If you have additional feedback please provide it before 7am EDT
>> tomorrow.
> 
> Thanks for writing this up.  Below are some things I noticed when
> reading through (disclaimer: I'm not a native speaker).

Thanks for the fixes + suggestions. I accepted most of them. Attached is
v3 of the patch, along with a diff.

Best,

Jonathan
PostgreSQL 12 Beta 1 Released
=

The PostgreSQL Global Development Group announces that the first beta release of
PostgreSQL 12 is now available for download. This release contains previews of
all features that will be available in the final release of PostgreSQL 12,
though some details of the release could change before then.

In the spirit of the open source PostgreSQL community, we strongly encourage you
to test the new features of PostgreSQL 12 in your database systems to help us
eliminate any bugs or other issues that may exist. While we do not advise for
you to run PostgreSQL 12 Beta 1 in your production environments, we encourage
you to find ways to run your typical application workloads against this beta
release.

Your testing and feedback will help the community ensure that the PostgreSQL 12
release upholds our standards of providing a stable, reliable release of the
world's most advanced open source relational database.

PostgreSQL 12 Features Highlights
-

### Indexing Performance, Functionality, and Management

PostgreSQL 12 improves the overall performance of the standard B-tree indexes
with improvements to the overall space management of these indexes as well.
These improvements also provide an overall reduction of index size for B-tree
indexes that are frequently modified, in addition to a performance gain.

Additionally, PostgreSQL 12 adds the ability to rebuild indexes concurrently,
which lets you perform a [`REINDEX`](https://www.postgresql.org/docs/devel/sql-reindex.html) operation
without blocking any writes to the index. The inclusion of this feature should
help with lengthy index rebuilds that could cause potential downtime when
managing a PostgreSQL database in a production environment.

PostgreSQL 12 extends the abilities of several of the specialized indexing
mechanisms. The ability to create covering indexes, i.e. the `INCLUDE` clause
that was introduced in PostgreSQL 11, has now been added to GiST indexes.
SP-GiST indexes now support the ability to perform K-nearest neighbor (K-NN)
queries for data types that support the distance (`<->`) operation.

The amount of write-ahead log (WAL) overhead generated when creating a GiST,
GIN, or SP-GiST index is also significantly reduced in PostgreSQL 12, which
provides several benefits to the overall disk utilization of a PostgreSQL
cluster and features such as continuous archiving and streaming replication.

### Inlined WITH queries (Common table expressions)

Common table expressions (aka `WITH` queries) can now be automatically inlined
in a query if they a) are not recursive, b) do not have any side-effects and
c) are only referenced once in a later part of a query. This removes an
"optimization fence" that has existed since the introduction of the `WITH`
clause in PostgreSQL 8.4

If need be, you can force a `WITH` query to materialize using the `MATERIALIZED`
clause, e.g.

```
WITH c AS MATERIALIZED (
SELECT * FROM a WHERE a.x % 4
)
SELECT * FROM c JOIN d ON d.y = a.x;
```

### Partitioning

PostgreSQL 12 improves on the performance when processing tables with thousands
of partitions for operations that only need to use a small number of partitions.

PostgreSQL 12 also provides improvements to the performance of both `INSERT` and
`COPY` into a partitioned table.  `ATTACH PARTITION` can now also be performed
without blocking concurrent queries on the partitioned table.  Additionally, the
ability to use foreign keys to reference partitioned tables is now permitted in
PostgreSQL 12.

### JSON path queries per SQL/JSON specification

PostgreSQL 12 now allows execution of [JSON path queries](https://www.postgresql.org/docs/devel/functions-json.html#FUNCTIONS-SQLJSON-PATH)
per the SQL/JSON specification in the SQL:2016 standard. Similar to XPath
expressions for XML, JSON path expressions let you evaluate a variety of
arithmetic expressions and functions in addition to comparing values within JSON
documents.

A subset of these expressions can be accelerated with GIN indexes, allowing the
execution of highly performant lookups across sets of JSON data.

### Collations

PostgreSQL 12 now supports case-insensitive and accent-insensitive comparisons
for ICU provided collations, also known as "[nondeterministic collations](https://www.postgresql.org/docs/devel/collation.html#COLLATION-NONDETERMINISTIC)".
When used, these collations can provide convenience for comparisons and sorts,
but can also lead to a performance penalty as a collation 

Re: docs about FKs referencing partitioned tables

[Michael Paquier]

On Mon, May 20, 2019 at 10:35:31PM -0700, Paul A Jungwirth wrote:
> I agree that sounds better. To avoid repeating it I changed the second
> instance to just "inherited tables". New patch attached.

Looking closer, you are adding that:
+ 
+  
+   While primary keys are supported on tables using inheritance
+   for partitioning, foreign keys referencing these tables are not
+   supported.  (Foreign key references from an inherited table to
+   some other table are supported.)
+  
+ 

However that's just fine:
=# create table aa (a int primary key);
CREATE TABLE
=# create table aa_child (a int primary key, inherits aa, foreign key
(a) references aa);
CREATE TABLE
=# create table aa_grandchild (a int primary key, inherits aa_child,
foreign key (a) references aa_child);
CREATE TABLE

The paragraph you are removing from 5.11.2.3 (limitations of
declarative partitioning) only applies to partitioned tables, not to
plain tables.  And there is no such thing for paritioning based on
inheritance, so we should just remove one paragraph, and not add the
extra one, no?
--
Michael


signature.asc
Description: PGP signature

Re: Minor typos and copyright year slippage

[Michael Paquier]

On Thu, May 23, 2019 at 01:28:45PM +1200, Thomas Munro wrote:
> Here are some tiny things I noticed in passing.

Good catches.  And you have spotted all the blank spots for the
copyright notices.
--
Michael


signature.asc
Description: PGP signature

Re: ACL dump ordering broken as well for tablespaces

[Michael Paquier]

On Wed, May 22, 2019 at 06:35:31PM +, Bossart, Nathan wrote:
> The patch looks good to me.

Thanks for double-checking.  I have applied and back-patched.  The good
thing here is that there were zero conflicts.

> A bit of digging led me to the commit that removed databases and
> tablespaces from pg_init_privs [0] and to a related thread [1].  IIUC
> the problem is that using pg_init_privs for databases is complicated
> by the ability to drop and recreate the template databases.

I don't quite get this argument.  If a user is willing to drop
template1, then it is logic to also drop its initial privilege entries
and recreate new ones from scratch.  I think that this deserves a
closer lookup.  For tablespaces, we are limited by the ability of not
sharing pg_init_privs?
--
Michael


signature.asc
Description: PGP signature

Re: MSVC Build support with visual studio 2019

[Haribabu Kommi]

On Wed, May 22, 2019 at 7:36 AM Juanjo Santamaria Flecha <
juanjo.santama...@gmail.com> wrote:

> I have gone through path
> '0001-Support-building-with-visual-studio-2019.patch' only, but I am sure
> some comments will also apply to back branches.
>

Thanks for the review.



> 1. The VisualStudioVersion value looks odd:
>
> +   $self->{VisualStudioVersion}= '16.0.32.32432';
>
> Are you using a pre-release version [1]?
>

I first developed this patch on the preview version.
I updated it to version 16.0.28729.10.


> 2. There is a typo: s/stuido/studio/:
>
> +   # The major visual stuido that is suppored has nmake version >=
> 14.20 and < 15.
>
> There is something in the current code that I think should be also
> updated. The code for _GetVisualStudioVersion contains:
>
>   if ($major > 14)
> {
> carp
>  "The determined version of Visual Studio is newer than the latest
> supported version. Returning the latest supported version instead.";
> return '14.00';
> }
>
> Shouldn't the returned value be '14.20' for Visual Studio 2019?
>

Yes, that will be good to return Visual Studio 2019, updated.

Updated patches are attached for all branches.

Regards,
Haribabu Kommi
Fujitsu Australia


0001-Support-building-with-visual-studio-2019_HEAD.patch
Description: Binary data


0001-support-building-with-visual-studio-2019_v11.patch
Description: Binary data


0001-support-building-with-visual-studio-2019_v9.4.patch
Description: Binary data


0001-support-building-with-visual-studio-2019_v10_to_v9.5.patch
Description: Binary data

Minor typos and copyright year slippage

[Thomas Munro]

Hi,

Here are some tiny things I noticed in passing.

-- 
Thomas Munro
https://enterprisedb.com


0001-Fix-typos.patch
Description: Binary data


0002-Update-copyright-year.patch
Description: Binary data

nitpick about poor style in MergeAttributes

[Mark Dilger]

Hackers,

I have been auditing the v12 source code for places
which inappropriately ignore the return value of a function
and have found another example which seems to me
a fertile source of future bugs.

In src/backend/nodes/list.c, list_delete_cell frees the list
and returns NIL when you delete the last element of a
list, placing a responsibility on any caller to check the
return value.

In tablecmds.c, MergeAttributes fails to do this.  My
inspection of the surrounding code leads me to suspect
that logically the cell being deleted can never be the
last cell, and hence the failure to check the return value
does not manifest as a bug.  But the surrounding
code is rather large and convoluted, and I have
little confidence that the code couldn't be changed such
that the return value would be NIL, possibly leading
to memory bugs.

What to do about this is harder to say.  In the following
patch, I'm just doing what I think is standard for callers
of list_delete_cell, and assigning the return value back
to the list (similar to how a call to repalloc should do).
But since there is an implicit assumption that the list
is never emptied by this operation, perhaps checking
against NIL and elog'ing makes more sense?

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 602a8dbd1c..96d6833274 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -2088,7 +2088,7 @@ MergeAttributes(List *schema, List *supers, char
relpersistence,
coldef->cooked_default =
restdef->cooked_default;
coldef->constraints =
restdef->constraints;
coldef->is_from_type = false;
-   list_delete_cell(schema, rest, prev);
+   schema =
list_delete_cell(schema, rest, prev);
}
else
ereport(ERROR,

Re: Typo: llvm*.cpp files identified as llvm*.c

[Thomas Munro]

On Wed, Jan 23, 2019 at 7:24 PM Andrew Gierth
 wrote:
> > "Andres" == Andres Freund  writes:
>
>  [IDENTIFICATION]
>
>  Andres> I think we should just rip them out. It's useless noise.
>
> +1

Here's a patch like that.

 948 files changed, 3215 deletions(-)

-- 
Thomas Munro
https://enterprisedb.com


0001-Drop-IDENTIFICATION-comment-blocks.patch.gz
Description: GNU Zip compressed data

Re: with oids option not removed in pg_dumpall

[Michael Paquier]

On Tue, May 21, 2019 at 05:24:57PM +0900, Michael Paquier wrote:
> Good catch.  Your cleanup looks correct to me.  Andres, perhaps you
> would prefer doing the cleanup yourself?

As I am cleaning up the area for another issue, applied.
--
Michael


signature.asc
Description: PGP signature

Re: Patch to fix write after end of array in hashed agg initialization

[Andrew Gierth]

> "Tom" == Tom Lane  writes:

 >> Attached is a patch for a write after allocated memory which we
 >> found in testing. Its an obscure case but can happen if the same
 >> column is used in different grouping keys, as in the example below,
 >> which uses tables from the regress test suite (build with
 >> --enable-cassert in order to turn on memory warnings). Patch is
 >> against master.

 Tom> I confirm the appearance of the memory-overwrite warnings in HEAD.

 Tom> It looks like the bad code is (mostly) the fault of commit
 Tom> b5635948. Andrew, can you take a look at this fix?

I'll look into it.

-- 
Andrew (irc:RhodiumToad)

Re: pg_dump throwing "column number -1 is out of range 0..36" on HEAD

[Michael Paquier]

On Wed, May 22, 2019 at 02:39:54PM -0400, Tom Lane wrote:
> Andres Freund  writes:
>> Well, if we explicitly have to check for -1, it's not really an error of
>> omission for everything. Yes, we could forget returning the amname in a
>> newer version of the query, but given that we usually just forward copy
>> the query that's not that likely.
> 
> No, the concerns I have are about (1) failure to insert the extra return
> column into all branches where it's needed; (2) some unexpected run-time
> problem causing the PGresult to not have the expected column.

Using a -1 check is not something I find much helpful, because this
masks the real problem that some queries may not have the output they
expect.

> I'm not sure if it'd work quite that cleanly when we need changes in the
> FROM part, but certainly for newly-added result fields this would be
> hugely better than repeating the whole query.  And yes, I'd still insist
> that explicitly providing the alternative NULL value is not optional.

This makes the addition of JOIN clauses and WHERE quals harder to
follow and read, and it makes back-patching harder (with testing to
older versions it is already complicated enough) so I don't think that
this is a good idea.  One extra idea I have would be to add a
compile-time flag which we could use to enforce a hard failure with an
assertion or such in those code paths, because we never expect it in
the in-core clients.  And that would cause any failure to be
immediately visible, at the condition of using the flag of course.

> int
> PQgetisnull(const PGresult *res, int tup_num, int field_num)
> {
> if (!check_tuple_field_number(res, tup_num, field_num))
> return 1;/* pretend it is null */
> 
> which just happens to be the right thing --- in this case --- for
> the back branches.

Yes.  I don't think that this is completely wrong.  So, are there any
objections if I just apply the patch at the top of this thread and fix
the issue?
--
Michael


signature.asc
Description: PGP signature

Suppressing noise in successful check-world runs

[Tom Lane]

[ redirected from a thread in pgsql-committers[1] ]

As of commit eb9812f27 you can run a manual check-world with
stdout dumped to /dev/null, and get fairly clean results:

$ time make check-world -j10 >/dev/null
NOTICE:  database "regression" does not exist, skipping

real1m43.875s
user2m50.659s
sys 1m22.518s
$

This is a productive way to work because if you do get a failure,
make's bleating gives you enough context to see which subdirectory
to check the log files in; so you don't really need to see all the
noise that goes to stdout.  (OTOH, if you don't discard stdout,
it's a mess; if you get a failure it could easily scroll off the
screen before you ever see it, leaving you with a false impression
that the test succeeded.)

However ... there is that one NOTICE, which is annoying just because
it's the only one left.  That's coming from the pg_upgrade test's
invocation of "make installcheck" in the instance it's just built.
(Every other test lets pg_regress build its own temp instance,
and then pg_regress knows it needn't bother with "DROP DATABASE
regression".)

I experimented with the attached quick-hack patch to make pg_regress
suppress notices from its various initial DROP/CREATE IF [NOT] EXISTS
commands.  I'm not entirely convinced whether suppressing them is
a good idea though.  Perhaps some hack with effects confined to
pg_upgrade's test would be better.  I don't have a good idea what
that would look like, however.

Or we could just say this isn't annoying enough to fix.

Thoughts?

regards, tom lane

[1] https://postgr.es/m/e1hsk9c-0002hh...@gemulon.postgresql.org
diff --git a/src/test/regress/pg_regress.c b/src/test/regress/pg_regress.c
index a1a3d48..57a154c 100644
--- a/src/test/regress/pg_regress.c
+++ b/src/test/regress/pg_regress.c
@@ -1087,6 +1087,7 @@ psql_command(const char *database, const char *query,...)
 	char		query_formatted[1024];
 	char		query_escaped[2048];
 	char		psql_cmd[MAXPGPATH + 2048];
+	const char *quiet;
 	va_list		args;
 	char	   *s;
 	char	   *d;
@@ -1106,11 +1107,23 @@ psql_command(const char *database, const char *query,...)
 	}
 	*d = '\0';
 
+	/*
+	 * If the query uses IF EXISTS or IF NOT EXISTS, suppress useless
+	 * "skipping" notices.  We intentionally consider only the constant
+	 * "query" string, not what was interpolated into it.
+	 */
+	if (strstr(query, "IF EXISTS") != NULL ||
+		strstr(query, "IF NOT EXISTS") != NULL)
+		quiet = " -c \"SET client_min_messages = warning\"";
+	else
+		quiet = "";
+
 	/* And now we can build and execute the shell command */
 	snprintf(psql_cmd, sizeof(psql_cmd),
-			 "\"%s%spsql\" -X -c \"%s\" \"%s\"",
+			 "\"%s%spsql\" -X%s -c \"%s\" \"%s\"",
 			 bindir ? bindir : "",
 			 bindir ? "/" : "",
+			 quiet,
 			 query_escaped,
 			 database);
 

Re: PostgreSQL 12 Beta 1 press release draft

[Dagfinn Ilmari Mannsåker]

Hi Jonathan,

"Jonathan S. Katz"  writes:

> If you have additional feedback please provide it before 7am EDT
> tomorrow.

Thanks for writing this up.  Below are some things I noticed when
reading through (disclaimer: I'm not a native speaker).

> PostgreSQL 12 extends the abilities of several of the specialized indexing
> mechanisms. The ability to create covering indexes, i.e. the `INCLUDE` clause
> that was introduced in PostgreSQL 11, havs now been added to GiST indexes.

"havs" should be "has"

> The amount of write-ahead log (WAL) overhead generated when creating a GiST,
> GIN, or SP-GiST index is also significantly reduced in PostgreSQL 12, which
> provides several benefits to the overall disk utilization of a PostgreSQL
> cluster as well as using features such as continuous archiving and streaming
> replication.

The "using" reads odd to me.  I think it would be better either omitted,
or expanded to "when using".

> ### Partitioning
>
> PostgreSQL 12 improves on the performance when processing tables with 
> thousands
> of partitions for operations that only need to use a small number of 
> partitions.
>
> PostgreSQL 12 also provides improvements to the performance of both `INSERT` 
> and
> `COPY` into a partitioned table.  `ATTACH PARTITION` can now also be performed
> without blocking concurrent queries on the partitioned table.  Additionally, 
> the
> ability to use foreign keys to reference partitioned tables is now allowed in
> PostgreSQL 12.

"the ability to use ... is now allowed" doesn't look right.  How about
"the ability to use ... is now provided" or "using ... is now allowed"?

> ### Collations
>
> PostgreSQL 12 now supports case-insensitive and accent-insensitive collations
> for ICU provided collations,

"collations for ... collations" doesn't look right.  I think either
"comparison for ... collations" or "collation ... for collations" would
be better, but I'm not sure which.

> ### Generated Columns
>
> PostgreSQL 12 lets you create [generated 
> columns](https://www.postgresql.org/docs/devel/ddl-generated-columns.html)
> that compute their values based on the contents of other columns. This feature
> provides stored generated columns, which are computed on inserts and updated 
> and
> are saved on disk.

Should be "on inserts and updates".

> ### Pluggable Table Storage Interface
>
> PostgreSQL 12 introduces the pluggable table storage interface that allows for
> the creation and use of different storage mechanisms for table storage. New
> access methods can be added to a PostgreSQL cluster using the [`CREATE ACCESS 
> METHOD`](https://www.postgresql.org/docs/devel/sql-create-access-method.html)
> and subsequently added to tables with the new `USING` clause on `CREATE 
> TABLE`.

Should be either "the CREATE ACCESS METHOD command" or just "CREATE
ACCESS METHOD".

> ### Page Checksums
>
> The `pg_verify_checkums` command has been renamed to 
> [`pg_checksums`](https://www.postgresql.org/docs/devel/app-pgchecksums.html)
> and now supports the ability to enable and disable page checksums across an
> PostgreSQL cluster that is offline.

Should be "a PostgreSQL cluster", not "an".

- ilmari
-- 
"A disappointingly low fraction of the human race is,
 at any given time, on fire." - Stig Sandbeck Mathisen

Re: Remove useless associativity/precedence from parsers

[Tom Lane]

Andrew Dunstan  writes:
> On 5/21/19 11:49 AM, Akim Demaille wrote:
>> Usually users of Bison build tarballs with the generated parsers
>> in them, and ship/test from that.

> The buildfarm client does not build from tarballs, it builds from git,
> meaning it has to run bison. Thus Tom's objection is quite valid, and
> your dismissal of it is not.

Right, but that's a much narrower set of people who need to update
than "all PG users" or even "all PG developers".

I checked the buildfarm's configure results not too long ago, and noted
that the oldest bison versions are

 gaur  | configure: using bison (GNU Bison) 1.875
 prairiedog| configure: using bison (GNU Bison) 1.875
 dromedary | configure: using bison (GNU Bison) 2.3
 locust| configure: using bison (GNU Bison) 2.3
 longfin   | configure: using bison (GNU Bison) 2.3
 nudibranch| configure: using bison (GNU Bison) 2.3
 anole | configure: using bison (GNU Bison) 2.4.1
 fulmar| configure: using bison (GNU Bison) 2.4.1
 gharial   | configure: using bison (GNU Bison) 2.4.1
 grouse| configure: using bison (GNU Bison) 2.4.1
 koreaceratops | configure: using bison (GNU Bison) 2.4.1
 leech | configure: using bison (GNU Bison) 2.4.1
 magpie| configure: using bison (GNU Bison) 2.4.1
 treepie   | configure: using bison (GNU Bison) 2.4.1
 coypu | configure: using bison (GNU Bison) 2.4.3
 friarbird | configure: using bison (GNU Bison) 2.4.3
 nightjar  | configure: using bison (GNU Bison) 2.4.3
 (then 2.5 and later)

(This doesn't cover the Windows members, unfortunately.)

gaur and prairiedog are my own pet dinosaurs, and updating them would
not be very painful.  (Neither of them are using the original vendor
Bison to begin with ... as I said, they're dinosaurs.)  Meanwhile,
three of the 2.3 members are Mac systems; nudibranch is SUSE 11.
Requiring anything newer than 2.4.1 would start to cause problems
for a fair number of people, I think.

Still, the bottom line here is that we could require a new(ish) Bison
if we could point to clear benefits that outweigh the pain.  Right
now there's not much argument for it.

regards, tom lane

Re: Remove useless associativity/precedence from parsers

[Andrew Dunstan]

On 5/21/19 11:49 AM, Akim Demaille wrote:
> Hi Tom!
>
>> Le 20 mai 2019 à 15:54, Tom Lane  a écrit :
>>
>> Akim Demaille  writes:
>>> It is for the same reasons that I would recommend not using associativity 
>>> directives (%left, %right, %nonassoc) where associativity plays no role: 
>>> %precedence is made for this.  But it was introduced in Bison 2.7.1 
>>> (2013-04-15), and I don't know if requiring it is acceptable to PostgreSQL.
>> 2013?  Certainly not.  We have a lot of buildfarm critters running
>> older platforms than that.
> This I fully understand.  However, Bison is a source generator,
> and it's quite customary to use modern tools on the maintainer
> side, and then deploy the result them on possibly much older
> architectures.
>
> Usually users of Bison build tarballs with the generated parsers
> in them, and ship/test from that.
>


The buildfarm client does not build from tarballs, it builds from git,
meaning it has to run bison. Thus Tom's objection is quite valid, and
your dismissal of it is not.


cheers


andrew


-- 

Andrew Dunstanhttps://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: PostgreSQL 12 Beta 1 press release draft

[Jonathan S. Katz]

On 5/21/19 11:39 PM, Jonathan S. Katz wrote:
> Hi,
> 
> Attached is a draft of the PG12 Beta 1 press release that is going out
> this Thursday. The primary goals of this release announcement are to
> introduce new features, enhancements, and changes that are available in
> PG12, as well as encourage our users to test and provide feedback to
> help ensure the stability of the release.
> 
> Speaking of feedback, please provide me with your feedback on the
> technical correctness of this announcement so I can incorporate changes
> prior to the release.

Thank you everyone for your feedback. I have incorporated most of it
into this latest revision. For your convenience I have also attached a diff.

Please let me know if you have any questions. If you have additional
feedback please provide it before 7am EDT tomorrow.

Thanks!

Jonathan
PostgreSQL 12 Beta 1 Released
=

The PostgreSQL Global Development Group announces that the first beta release of
PostgreSQL 12 is now available for download. This release contains previews of
all features that will be available in the final release of PostgreSQL 12,
though some details of the release could change before then.

In the spirit of the open source PostgreSQL community, we strongly encourage you
to test the new features of PostgreSQL 12 in your database systems to help us
eliminate any bugs or other issues that may exist. While we do not advise for
you to run PostgreSQL 12 Beta 1 in your production environments, we encourage
you to find ways to run your typical application workloads against this beta
release.

Your testing and feedback will help the community ensure that the PostgreSQL 12
release upholds our standards of providing a stable, reliable release of the
world's most advanced open source relational database.

PostgreSQL 12 Features Highlights
-

### Indexing Performance, Functionality, and Management

PostgreSQL 12 improves the overall performance of the standard B-tree indexes
with improvements to the overall space management of these indexes as well.
These improvements also provide an overall reduction of index size for B-tree
indexes that are frequently modified, in addition to a performance gain.

Additionally, PostgreSQL 12 adds the ability to rebuild indexes concurrently,
which lets you perform a [`REINDEX`](https://www.postgresql.org/docs/devel/sql-reindex.html) operation
without blocking any writes to the index. The inclusion of this feature should
help with lengthy index rebuilds that could cause potential downtime when
managing a PostgreSQL database in a production environment.

PostgreSQL 12 extends the abilities of several of the specialized indexing
mechanisms. The ability to create covering indexes, i.e. the `INCLUDE` clause
that was introduced in PostgreSQL 11, havs now been added to GiST indexes.
SP-GiST indexes now support the ability to perform K-nearest neighbor (K-NN)
queries for data types that support the distance (`<->`) operation.

The amount of write-ahead log (WAL) overhead generated when creating a GiST,
GIN, or SP-GiST index is also significantly reduced in PostgreSQL 12, which
provides several benefits to the overall disk utilization of a PostgreSQL
cluster as well as using features such as continuous archiving and streaming
replication.

### Inlined WITH queries (Common table expressions)

Common table expressions (aka `WITH` queries) can now be automatically inlined
in a query if they a) are not recursive, b) do not have any side-effects and
c) are only referenced once in a later part of a query. This removes an
"optimization fence" that has existed since the introduction of the `WITH`
clause in PostgreSQL 8.4

If need be, you can force a `WITH` query to materialize using the `MATERIALIZED`
clause, e.g.

```
WITH c AS MATERIALIZED (
SELECT * FROM a WHERE a.x % 4
)
SELECT * FROM c JOIN d ON d.y = a.x;
```

### Partitioning

PostgreSQL 12 improves on the performance when processing tables with thousands
of partitions for operations that only need to use a small number of partitions.

PostgreSQL 12 also provides improvements to the performance of both `INSERT` and
`COPY` into a partitioned table.  `ATTACH PARTITION` can now also be performed
without blocking concurrent queries on the partitioned table.  Additionally, the
ability to use foreign keys to reference partitioned tables is now allowed in
PostgreSQL 12.

### JSON path queries per SQL/JSON specification

PostgreSQL 12 now allows execution of [JSON path queries](https://www.postgresql.org/docs/devel/functions-json.html#FUNCTIONS-SQLJSON-PATH)
per the SQL/JSON specification in the SQL:2016 standard. Similar to XPath
expressions for XML, JSON path expressions let you evaluate a variety of
arithmetic expressions and functions in addition to comparing values within JSON
documents.

A subset of these expressions can be accelerated with GIN indexes, allowing the
execution of highly performant lookups across sets of 

Re: Remove useless associativity/precedence from parsers

[Daniel Gustafsson]

> On 22 May 2019, at 23:25, Tom Lane  wrote:
> 
> Akim Demaille  writes:
>> Honestly, I seriously doubt that you have contributors that don't
>> have MacPorts or Brew installed, and both are pretty up to date on
>> Bison.
> 
> Hm, well, I'm a counterexample ;-)

And one more.  While I do have brew installed, I prefer to use it as little as
possible.

cheers ./daniel

Re: Remove useless associativity/precedence from parsers

[Tom Lane]

Akim Demaille  writes:
> Honestly, I seriously doubt that you have contributors that don't
> have MacPorts or Brew installed, and both are pretty up to date on
> Bison.

Hm, well, I'm a counterexample ;-).  Right now you can develop PG
on a Mac just fine without any additional stuff, excepting maybe
OpenSSL if you want that.  If we have a strong reason to require
a newer Bison, I'd be willing to do so, but it needs to be a
strong reason.

>> * Speed of the generated parser could be better.

> Expect news this year about that.  I precisely came to look at
> PostgreSQL for this.

That's very cool news.

> Is there an easy way to bench pg and the various
> costs?  To be explicit: is there a way to see how long the parsing
> phase takes?  And some mighty inputs to bench against?

The easiest method is to fire up some client code that repeatedly
does whatever you want to test, and then look at perf or oprofile
or local equivalent to see where the time is going in the backend
process.

For the particular case of stressing the parser, probably the
best thing to look at is test cases that do a lot of low-overhead
DDL, such as creating views.  You could do worse than just repeatedly
sourcing our standard view files, like
src/backend/catalog/system_views.sql
src/backend/catalog/information_schema.sql
(In either case, I'd suggest adapting the file to create all
its objects in some transient schema that you can just drop.
Repointing information_schema.sql to some other schema is
trivial, just change a couple of commands at the top; and
you could tweak system_views.sql similarly.  Also consider
wrapping the whole thing in BEGIN; ... ROLLBACK; instead of
spending time on an explicit DROP.)

Somebody else might know of a better test case but I'd try
that first.

There would still be a fair amount of I/O and catalog lookup
overhead in a test run that way, but it would be an honest
approximation of useful real-world cases.  If you're willing to
put some blinders on and just micro-optimize the flex/bison
code, you could set up a custom function that just calls that
stuff.  I actually did that not too long ago; C code attached
for amusement's sake.

>> * Lack of run-time extensibility of the parser.  There are many PG
>> extensions that wish they could add things into the grammar, and can't.

> Are there documented examples of this?  What would that look like?

I'm just vaguely recalling occasional how-could-I-do-this complaints
on the pgsql-hackers mailing list.  Perhaps somebody else could say
something more concrete.

regards, tom lane

/*

build this into a Postgres shared library, then

create function drive_parser(query text, count int) returns void
strict volatile language c as '.../drive_parser.so';

\timing

select drive_parser('some-query', 1000);

 */

#include "postgres.h"

#include "fmgr.h"
#include "miscadmin.h"
#include "tcop/tcopprot.h"
#include "utils/builtins.h"
#include "utils/memutils.h"

PG_MODULE_MAGIC;

/*
 * drive_parser(query text, count int) returns void
 */
PG_FUNCTION_INFO_V1(drive_parser);
Datum
drive_parser(PG_FUNCTION_ARGS)
{
	text	   *txt = PG_GETARG_TEXT_PP(0);
	int32		count = PG_GETARG_INT32(1);
	char	   *query_string = text_to_cstring(txt);
	MemoryContext mycontext;

	mycontext = AllocSetContextCreate(CurrentMemoryContext,
	  "drive_parser work cxt",
	  ALLOCSET_DEFAULT_SIZES);

	while (count-- > 0)
	{
		List	   *parsetree_list;
		MemoryContext oldcontext;

		oldcontext = MemoryContextSwitchTo(mycontext);

		/* This times raw parsing only */
		parsetree_list = pg_parse_query(query_string);

		MemoryContextSwitchTo(oldcontext);

		MemoryContextReset(mycontext);

		CHECK_FOR_INTERRUPTS();
	}

	PG_RETURN_VOID();
}

Re: pgindent run next week?

[Euler Taveira]

Em qua, 22 de mai de 2019 às 14:08, Tom Lane  escreveu:
>
> I wrote:
> > Hearing no objections, I'll plan on running pgindent tomorrow sometime.
>
> And done.
>
> > The new underlying pg_bsd_indent (2.1) is available now from
> > https://git.postgresql.org/git/pg_bsd_indent.git
>
> Please update your local copy if you have one.
>
I give it a try in a fork of PostgreSQL 10. The difference between v10
and my fork is not huge. The stats are 56 files changed, 2240
insertions(+), 203 deletions(-) and patch size is 139 Kb. I have
conflicts in 3 of 19 .h files and 1 of 25 .c files. Like Mark, I don't
have a strong preference, however, re-indent files would reduce
developer time while preparing patches to back branches.


-- 
   Euler Taveira   Timbira -
http://www.timbira.com.br/
   PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento

Re: Is it safe to ignore the return value of SPI_finish and SPI_execute?

[Mark Dilger]

On Wed, May 22, 2019 at 1:52 PM Tom Lane  wrote:
>
> Mark Dilger  writes:
> > On Fri, May 17, 2019 at 6:12 PM Tom Lane  wrote:
> >> One reasonable solution would be to change the callers that got this
> >> wrong.  Another one would be to reconsider whether the error-return-code
> >> convention makes any sense at all here.  If we changed the above-quoted
> >> bit to be an ereport(ERROR), then we could say that SPI_finish either
> >> returns 0 or throws error, making it moot whether callers check, and
> >> allowing removal of now-useless checks from all the in-core callers.
>
> > Does this proposal of yours seem good enough for me to make a patch
> > based on this design?
>
> Just to clarify --- I think what's being discussed here is "change some
> large fraction of the SPI functions that can return SPI_ERROR_xxx error
> codes to throw elog/ereport(ERROR) instead".

Yes, I was talking about that, but was ambiguous in how I phrased my
question.

> Figuring out what fraction
> that should be is part of the work --- but just in a quick scan through
> spi.c, it seems like there might be a case for deprecating practically
> all the SPI_ERROR_xxx codes except for SPI_ERROR_NOATTRIBUTE.
> I'd definitely argue that SPI_ERROR_UNCONNECTED and SPI_ERROR_ARGUMENT
> deserve that treatment.
>
> I'm for it, if you want to do the work, but I don't speak for everybody.

I do want to write the patch, but I'll wait for other opinions.

mark

Re: Why could GEQO produce plans with lower costs than the standard_join_search?

[Finnerty, Jim]

Fwiw, I had an intern do some testing on the JOB last year, and he reported 
that geqo sometimes produced plans of lower cost than the standard planner (we 
were on PG10 at the time).  I filed it under "unexplained things that we need 
to investigate when we have time", but alas...

In any case, Donald isn't the only one who has noticed this behavior. 

On 5/22/19, 3:54 PM, "Donald Dong"  wrote:

On May 22, 2019, at 11:42 AM, Tom Lane  wrote:
> 
> Donald Dong  writes:
>> I find the cost from cheapest_total_path->total_cost is different
>> from the cost from  queryDesc->planstate->total_cost. What I saw was
>> that GEQO tends to form paths with lower
>> cheapest_total_path->total_cost (aka the fitness of the children).
>> However, standard_join_search is more likely to produce a lower
>> queryDesc->planstate->total_cost, which is the cost we get using
>> explain.
> 
>> I wonder why those two total costs are different? If the total_cost
>> from the planstate is more accurate, could we use that instead as the
>> fitness in geqo_eval?
> 
> You're still asking us to answer hypothetical questions unsupported
> by evidence.  In what case does that really happen?

Hi,

My apologies if this is not the minimal necessary set up. But here's
more information about what I saw using the following query
(JOB/1a.sql):

SELECT MIN(mc.note) AS production_note,
   MIN(t.title) AS movie_title,
   MIN(t.production_year) AS movie_year
FROM company_type AS ct,
 info_type AS it,
 movie_companies AS mc,
 movie_info_idx AS mi_idx,
 title AS t
WHERE ct.kind = 'production companies'
  AND it.info = 'top 250 rank'
  AND mc.note NOT LIKE '%(as Metro-Goldwyn-Mayer Pictures)%'
  AND (mc.note LIKE '%(co-production)%'
   OR mc.note LIKE '%(presents)%')
  AND ct.id = mc.company_type_id
  AND t.id = mc.movie_id
  AND t.id = mi_idx.movie_id
  AND mc.movie_id = mi_idx.movie_id
  AND it.id = mi_idx.info_type_id;

I attached the query plan and debug_print_rel output for GEQO and
standard_join_search.

planstate->total_cost   cheapest_total_path
GEQO54190.1354239.03
STD 54179.0254273.73

Here I observe GEQO  produces a lower
cheapest_total_path->total_cost, but its planstate->total_cost is higher
than what standard_join_search produces.

Regards,
Donald Dong

Re: Is it safe to ignore the return value of SPI_finish and SPI_execute?

[Tom Lane]

Mark Dilger  writes:
> On Fri, May 17, 2019 at 6:12 PM Tom Lane  wrote:
>> One reasonable solution would be to change the callers that got this
>> wrong.  Another one would be to reconsider whether the error-return-code
>> convention makes any sense at all here.  If we changed the above-quoted
>> bit to be an ereport(ERROR), then we could say that SPI_finish either
>> returns 0 or throws error, making it moot whether callers check, and
>> allowing removal of now-useless checks from all the in-core callers.

> Does this proposal of yours seem good enough for me to make a patch
> based on this design?

Just to clarify --- I think what's being discussed here is "change some
large fraction of the SPI functions that can return SPI_ERROR_xxx error
codes to throw elog/ereport(ERROR) instead".  Figuring out what fraction
that should be is part of the work --- but just in a quick scan through
spi.c, it seems like there might be a case for deprecating practically
all the SPI_ERROR_xxx codes except for SPI_ERROR_NOATTRIBUTE.
I'd definitely argue that SPI_ERROR_UNCONNECTED and SPI_ERROR_ARGUMENT
deserve that treatment.

I'm for it, if you want to do the work, but I don't speak for everybody.

It's not entirely clear to me whether we ought to change the return
convention to be "returns void" or make it "always returns SPI_OK"
for those functions where the return code becomes trivial.  The
latter would avoid churn for external modules, but it seems not to
have much other attractiveness.

regards, tom lane

Re: FullTransactionId changes are causing portability issues

[Tom Lane]

Andres Freund  writes:
> On 2019-05-22 15:55:50 -0400, Tom Lane wrote:
>>> So the disturbing thing here is that we no longer have any active
>>> buildfarm members that can build HEAD but have the won't-elide-
>>> unused-static-functions problem.  Clearly we'd better close that
>>> gap somehow ... anyone have an idea about how to test it better?

> I'm somewhat inclined to just declare that people using such old
> compilers ought to just use something newer. Having to work around
> broken compilers that are so old that we don't even have a buildfarm
> animal actually exposing that behaviour, seems like wasted effort. IMO
> it'd make sense to just treat this as part of the requirements for a C99
> compiler.

TBH, I too supposed the requirement for this had gone away with the C99
move.  But according to the discussion today on -packagers, there are
still supported variants of Solaris that have compilers that speak C99
but don't have this behavior.  Per Bjorn's report:

>> The compiler used in Sun Studio 12u1, very old and and I can try to
>> upgrade and see if that helps.
> I tried Sun Studio 12u2 and then a more drastic upgrade to Developer
> Studio 12.5 but both had the same effect.

It doesn't sound like "use a newer compiler" is going to be a helpful
answer there.

(It is annoying that nobody is running such a platform in the buildfarm,
I agree.  But I don't have the resources to spin up a real-Solaris
buildfarm member.)

regards, tom lane

Re: Teach pg_upgrade test to honor NO_TEMP_INSTALL

[Andres Freund]

On 2019-05-22 16:04:34 -0400, Andrew Dunstan wrote:
> 
> On 5/22/19 2:42 PM, Andres Freund wrote:
> > Hi,
> >
> > On 2019-05-22 14:27:51 -0400, Tom Lane wrote:
> >> Andres Freund  writes:
> >>> On 2019-05-22 14:06:47 -0400, Tom Lane wrote:
>  Not sure about that last bit.  pg_upgrade has the issue of possibly
>  wanting to deal with 2 installations, unlike the rest of the tree,
>  so I'm not sure that fixing its problem means there's something we
>  need to change everywhere else.
> >>> I'm not quite following? We need to move it into global scope to fix the
> >>> issue at hand (namely that we currently need to make install first, just
> >>> to get psql).  And at which scope could it be in master, other than
> >>> global?
> >> Maybe I misunderstood you --- I thought you were talking about something
> >> like defining EXTRA_REGRESS_OPTS in Makefile.global.  If you mean
> >> running this unconditionally within test.sh, I've got no objection
> >> to that.
> > Oh, yes, that's what I meant.
> >
> >
> >>> I do think we will have to move it to the global scope in the back
> >>> branches too, because NO_TEMP_INSTALL does indeed fail without a global
> >>> install first (rather than using the temp install, as intended):
> >> Agreed, we should fix it in all branches, because it seems like it's
> >> probably testing the wrong thing, ie using the later branch's psql
> >> to run the earlier branch's regression tests.
> 
> 
> 
> If I disable install, the buildfarm fails the upgrade check even when
> not using NO_TEMP_INSTALL.
> 
> 
> excerpts from the log:
> 
> 
> 
> rm -rf '/home/pgl/npgl/pg_head/bfroot/HEAD/pgsql.build'/tmp_install
> /bin/mkdir -p
> '/home/pgl/npgl/pg_head/bfroot/HEAD/pgsql.build'/tmp_install/log
> make -C '../../..'
> DESTDIR='/home/pgl/npgl/pg_head/bfroot/HEAD/pgsql.build'/tmp_install
> install
> >'/home/pgl/npgl/pg_head/bfroot/HEAD/pgsql.build'/tmp_install/log/install.log
> 2>&1
> make -j1  checkprep
> >>'/home/pgl/npgl/pg_head/bfroot/HEAD/pgsql.build'/tmp_install/log/install.log
> 2>&1
> MAKE=make
> PATH="/home/pgl/npgl/pg_head/bfroot/HEAD/pgsql.build/tmp_install/home/pgl/npgl/pg_head/bfroot/HEAD/inst/bin:$PATH"
> LD_LIBRARY_PATH="/home/pgl/npgl/pg_head/bfroot/HEAD/pgsql.build/tmp_install/home/pgl/npgl/pg_head/bfroot/HEAD/inst/lib"
>  
> bindir=/home/pgl/npgl/pg_h
> ead/bfroot/HEAD/pgsql.build/tmp_install//home/pgl/npgl/pg_head/bfroot/HEAD/inst/bin
> EXTRA_REGRESS_OPTS="--port=5678" /bin/sh
> /home/pgl/npgl/pg_head/src/bin/pg_upgrade/test.sh
> 
> 
> rm -rf ./testtablespace
> mkdir ./testtablespace
> ../../../src/test/regress/pg_regress
> --inputdir=/home/pgl/npgl/pg_head/src/test/regress
> --bindir='/home/pgl/npgl/pg_head/bfroot/HEAD/inst/bin'   --port=5678
> --port=54464 --dlpath=. --max-concurrent-tests=20 --port=5678
> --port=54464
> --schedule=/home/pgl/npgl/pg_head/src/test/regress/parallel_schedule 
> (using postmaster on /tmp/pg_upgrade_check-GCUkGu, port 54464)
> == dropping database "regression" ==
> sh: /home/pgl/npgl/pg_head/bfroot/HEAD/inst/bin/psql: No such file or
> directory
> command failed: "/home/pgl/npgl/pg_head/bfroot/HEAD/inst/bin/psql" -X -c
> "DROP DATABASE IF EXISTS \"regression\"" "postgres"
> make[1]: *** [GNUmakefile:141: installcheck-parallel] Error 2
> make[1]: Leaving directory
> '/home/pgl/npgl/pg_head/bfroot/HEAD/pgsql.build/src/test/regress'
> make: *** [GNUmakefile:68: installcheck-parallel] Error 2
> make: Leaving directory '/home/pgl/npgl/pg_head/bfroot/HEAD/pgsql.build'
> waiting for server to shut down done
> server stopped
> make: *** [Makefile:48: check] Error 1
> 

That's the issue I was talking to Tom about above. Need to
unconditionally have

+# We need to make pg_regress use psql from the desired installation
+# (likely a temporary one), because otherwise the installcheck run
+# below would try to use psql from the proper installation directory,
+# which might be outdated or missing. But don't override anything else
+# that's already in EXTRA_REGRESS_OPTS.
+EXTRA_REGRESS_OPTS="$EXTRA_REGRESS_OPTS --bindir='$oldbindir'"
+export EXTRA_REGRESS_OPTS

in all branches (i.e. ressurect in master, do it not just in the
--install case in the back branches, and reference $oldbindir rather
than $bindir in all branches).


> It looks to me like the bindir needs to be passed to the make called by
> test.sh (maybe LD_LIBRARY_PATH too?)

Think we don't need LD_LIBRARY_PATH, due to the $(with_temp_install)
logic in the makefile. In the back branches the --install branch
contains adjustments to LD_LIBRARY_PATH (but still references $bindir
rather than $oldbindr).

Greetings,

Andres Freund

Re: Teach pg_upgrade test to honor NO_TEMP_INSTALL

[Andrew Dunstan]

On 5/22/19 2:42 PM, Andres Freund wrote:
> Hi,
>
> On 2019-05-22 14:27:51 -0400, Tom Lane wrote:
>> Andres Freund  writes:
>>> On 2019-05-22 14:06:47 -0400, Tom Lane wrote:
 Not sure about that last bit.  pg_upgrade has the issue of possibly
 wanting to deal with 2 installations, unlike the rest of the tree,
 so I'm not sure that fixing its problem means there's something we
 need to change everywhere else.
>>> I'm not quite following? We need to move it into global scope to fix the
>>> issue at hand (namely that we currently need to make install first, just
>>> to get psql).  And at which scope could it be in master, other than
>>> global?
>> Maybe I misunderstood you --- I thought you were talking about something
>> like defining EXTRA_REGRESS_OPTS in Makefile.global.  If you mean
>> running this unconditionally within test.sh, I've got no objection
>> to that.
> Oh, yes, that's what I meant.
>
>
>>> I do think we will have to move it to the global scope in the back
>>> branches too, because NO_TEMP_INSTALL does indeed fail without a global
>>> install first (rather than using the temp install, as intended):
>> Agreed, we should fix it in all branches, because it seems like it's
>> probably testing the wrong thing, ie using the later branch's psql
>> to run the earlier branch's regression tests.



If I disable install, the buildfarm fails the upgrade check even when
not using NO_TEMP_INSTALL.


excerpts from the log:



rm -rf '/home/pgl/npgl/pg_head/bfroot/HEAD/pgsql.build'/tmp_install
/bin/mkdir -p
'/home/pgl/npgl/pg_head/bfroot/HEAD/pgsql.build'/tmp_install/log
make -C '../../..'
DESTDIR='/home/pgl/npgl/pg_head/bfroot/HEAD/pgsql.build'/tmp_install
install
>'/home/pgl/npgl/pg_head/bfroot/HEAD/pgsql.build'/tmp_install/log/install.log
2>&1
make -j1  checkprep
>>'/home/pgl/npgl/pg_head/bfroot/HEAD/pgsql.build'/tmp_install/log/install.log
2>&1
MAKE=make
PATH="/home/pgl/npgl/pg_head/bfroot/HEAD/pgsql.build/tmp_install/home/pgl/npgl/pg_head/bfroot/HEAD/inst/bin:$PATH"
LD_LIBRARY_PATH="/home/pgl/npgl/pg_head/bfroot/HEAD/pgsql.build/tmp_install/home/pgl/npgl/pg_head/bfroot/HEAD/inst/lib"
 
bindir=/home/pgl/npgl/pg_h
ead/bfroot/HEAD/pgsql.build/tmp_install//home/pgl/npgl/pg_head/bfroot/HEAD/inst/bin
EXTRA_REGRESS_OPTS="--port=5678" /bin/sh
/home/pgl/npgl/pg_head/src/bin/pg_upgrade/test.sh


rm -rf ./testtablespace
mkdir ./testtablespace
../../../src/test/regress/pg_regress
--inputdir=/home/pgl/npgl/pg_head/src/test/regress
--bindir='/home/pgl/npgl/pg_head/bfroot/HEAD/inst/bin'   --port=5678
--port=54464 --dlpath=. --max-concurrent-tests=20 --port=5678
--port=54464
--schedule=/home/pgl/npgl/pg_head/src/test/regress/parallel_schedule 
(using postmaster on /tmp/pg_upgrade_check-GCUkGu, port 54464)
== dropping database "regression" ==
sh: /home/pgl/npgl/pg_head/bfroot/HEAD/inst/bin/psql: No such file or
directory
command failed: "/home/pgl/npgl/pg_head/bfroot/HEAD/inst/bin/psql" -X -c
"DROP DATABASE IF EXISTS \"regression\"" "postgres"
make[1]: *** [GNUmakefile:141: installcheck-parallel] Error 2
make[1]: Leaving directory
'/home/pgl/npgl/pg_head/bfroot/HEAD/pgsql.build/src/test/regress'
make: *** [GNUmakefile:68: installcheck-parallel] Error 2
make: Leaving directory '/home/pgl/npgl/pg_head/bfroot/HEAD/pgsql.build'
waiting for server to shut down done
server stopped
make: *** [Makefile:48: check] Error 1



It looks to me like the bindir needs to be passed to the make called by
test.sh (maybe LD_LIBRARY_PATH too?)


cheers


andrew


-- 

Andrew Dunstanhttps://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: FullTransactionId changes are causing portability issues

[Andres Freund]

Hi,

On 2019-05-22 15:55:50 -0400, Tom Lane wrote:
> I wrote:
> >> Our Solaris packager reports that 12beta1 is failing to build for him
> >> on some Solaris variants:
> >>> The link failure is:
> >>> Undefined first referenced
> >>> symbolin file
> >>> ReadNextFullTransactionId   pg_checksums.o
> 
> > On looking closer, the fix is simple and matches what we've done
> > elsewhere: transam.h needs to have "#ifndef FRONTEND" to protect
> > its static inline function from being compiled into frontend code.
> 
> > So the disturbing thing here is that we no longer have any active
> > buildfarm members that can build HEAD but have the won't-elide-
> > unused-static-functions problem.  Clearly we'd better close that
> > gap somehow ... anyone have an idea about how to test it better?

I'm somewhat inclined to just declare that people using such old
compilers ought to just use something newer. Having to work around
broken compilers that are so old that we don't even have a buildfarm
animal actually exposing that behaviour, seems like wasted effort. IMO
it'd make sense to just treat this as part of the requirements for a C99
compiler.


> Ah-hah --- some study of the gcc manual finds that modern versions
> of gcc have
> 
> `-fkeep-inline-functions'
>  In C, emit `static' functions that are declared `inline' into the
>  object file, even if the function has been inlined into all of its
>  callers.  This switch does not affect functions using the `extern
>  inline' extension in GNU C89.  In C++, emit any and all inline
>  functions into the object file.
> 
> This seems to do exactly what we need to test for this problem.
> I've confirmed that with it turned on, a modern platform finds
> the ReadNextFullTransactionId problem with yesterday's sources,
> and that everything seems green as of HEAD.
> 
> So, we'd obviously not want to turn this on for normal builds,
> but could we get a buildfarm animal or two to use this switch?

I could easily add that to one of mine, if we decide to go for that.

Greetings,

Andres Freund

Re: FullTransactionId changes are causing portability issues

[Tom Lane]

I wrote:
>> Our Solaris packager reports that 12beta1 is failing to build for him
>> on some Solaris variants:
>>> The link failure is:
>>> Undefined   first referenced
>>> symbol  in file
>>> ReadNextFullTransactionId   pg_checksums.o

> On looking closer, the fix is simple and matches what we've done
> elsewhere: transam.h needs to have "#ifndef FRONTEND" to protect
> its static inline function from being compiled into frontend code.

> So the disturbing thing here is that we no longer have any active
> buildfarm members that can build HEAD but have the won't-elide-
> unused-static-functions problem.  Clearly we'd better close that
> gap somehow ... anyone have an idea about how to test it better?

Ah-hah --- some study of the gcc manual finds that modern versions
of gcc have

`-fkeep-inline-functions'
 In C, emit `static' functions that are declared `inline' into the
 object file, even if the function has been inlined into all of its
 callers.  This switch does not affect functions using the `extern
 inline' extension in GNU C89.  In C++, emit any and all inline
 functions into the object file.

This seems to do exactly what we need to test for this problem.
I've confirmed that with it turned on, a modern platform finds
the ReadNextFullTransactionId problem with yesterday's sources,
and that everything seems green as of HEAD.

So, we'd obviously not want to turn this on for normal builds,
but could we get a buildfarm animal or two to use this switch?

regards, tom lane

Re: Why could GEQO produce plans with lower costs than the standard_join_search?

[Donald Dong]

On May 22, 2019, at 11:42 AM, Tom Lane  wrote:
> 
> Donald Dong  writes:
>> I find the cost from cheapest_total_path->total_cost is different
>> from the cost from  queryDesc->planstate->total_cost. What I saw was
>> that GEQO tends to form paths with lower
>> cheapest_total_path->total_cost (aka the fitness of the children).
>> However, standard_join_search is more likely to produce a lower
>> queryDesc->planstate->total_cost, which is the cost we get using
>> explain.
> 
>> I wonder why those two total costs are different? If the total_cost
>> from the planstate is more accurate, could we use that instead as the
>> fitness in geqo_eval?
> 
> You're still asking us to answer hypothetical questions unsupported
> by evidence.  In what case does that really happen?

Hi,

My apologies if this is not the minimal necessary set up. But here's
more information about what I saw using the following query
(JOB/1a.sql):

SELECT MIN(mc.note) AS production_note,
   MIN(t.title) AS movie_title,
   MIN(t.production_year) AS movie_year
FROM company_type AS ct,
 info_type AS it,
 movie_companies AS mc,
 movie_info_idx AS mi_idx,
 title AS t
WHERE ct.kind = 'production companies'
  AND it.info = 'top 250 rank'
  AND mc.note NOT LIKE '%(as Metro-Goldwyn-Mayer Pictures)%'
  AND (mc.note LIKE '%(co-production)%'
   OR mc.note LIKE '%(presents)%')
  AND ct.id = mc.company_type_id
  AND t.id = mc.movie_id
  AND t.id = mi_idx.movie_id
  AND mc.movie_id = mi_idx.movie_id
  AND it.id = mi_idx.info_type_id;

I attached the query plan and debug_print_rel output for GEQO and
standard_join_search.

planstate->total_cost   cheapest_total_path
GEQO54190.1354239.03
STD 54179.0254273.73

Here I observe GEQO  produces a lower
cheapest_total_path->total_cost, but its planstate->total_cost is higher
than what standard_join_search produces.

Regards,
Donald Dong

 Finalize Aggregate  (cost=54190.12..54190.13 rows=1 width=68)
   ->  Gather  (cost=54189.90..54190.11 rows=2 width=68)
 Workers Planned: 2
 ->  Partial Aggregate  (cost=53189.90..53189.91 rows=1 width=68)
   ->  Nested Loop  (cost=15318.71..53189.55 rows=46 width=45)
 Join Filter: (mc.movie_id = t.id)
 ->  Hash Join  (cost=15318.28..53162.39 rows=46 width=32)
   Hash Cond: (mc.company_type_id = ct.id)
   ->  Parallel Hash Join  (cost=15317.21..53160.33 
rows=185 width=36)
 Hash Cond: (mc.movie_id = mi_idx.movie_id)
 ->  Parallel Seq Scan on movie_companies mc  
(cost=0.00..37814.90 rows=7320 width=32)
   Filter: (((note)::text !~~ '%(as 
Metro-Goldwyn-Mayer Pictures)%'::text) AND (((note)::text ~~ 
'%(co-production)%'::text) OR ((note)::text ~~ ' %(presents)%'::text)))
 ->  Parallel Hash  (cost=15253.60..15253.60 
rows=5089 width=4)
   ->  Hash Join  (cost=2.43..15253.60 
rows=5089 width=4)
 Hash Cond: (mi_idx.info_type_id = 
it.id)
 ->  Parallel Seq Scan on 
movie_info_idx mi_idx  (cost=0.00..13685.15 rows=575015 width=8)
 ->  Hash  (cost=2.41..2.41 rows=1 
width=4)
   ->  Seq Scan on info_type it 
 (cost=0.00..2.41 rows=1 width=4)
 Filter: ((info)::text 
= 'top 250 rank'::text)
   ->  Hash  (cost=1.05..1.05 rows=1 width=4)
 ->  Seq Scan on company_type ct  
(cost=0.00..1.05 rows=1 width=4)
   Filter: ((kind)::text = 'production 
companies'::text)
 ->  Index Scan using title_pkey on title t  
(cost=0.43..0.58 rows=1 width=25)
   Index Cond: (id = mi_idx.movie_id)
 Finalize Aggregate  (cost=54179.01..54179.02 rows=1 width=68)
   ->  Gather  (cost=54178.79..54179.00 rows=2 width=68)
 Workers Planned: 2
 ->  Partial Aggregate  (cost=53178.79..53178.80 rows=1 width=68)
   ->  Nested Loop  (cost=37881.27..53178.44 rows=46 width=45)
 Join Filter: (mc.movie_id = t.id)
 ->  Parallel Hash Join  (cost=37880.84..53151.28 rows=46 
width=32)
   Hash Cond: (mi_idx.movie_id = mc.movie_id)
   ->  Hash Join  (cost=2.43..15253.60 rows=5089 
width=4)
 Hash Cond: (mi_idx.info_type_id = it.id)
 ->  Parallel Seq Scan on movie_info_idx mi_idx 
 (cost=0.00..13685.15 rows=575015 width=8)
 ->  Hash  

Re: PostgreSQL 12 Beta 1 press release draft

[Andres Freund]

Hi,

> ### Indexing Performance, Functionality, and Management
> 
> PostgreSQL 12 improves the overall performance of the standard B-tree indexes
> with improvements to the overall space management of these indexes as well.
> These improvements also provide an overall reduction of bloating when using
> B-tree for specific use cases, in addition to a performance gain.

I'm not sure everyone will understand bloating as a term? Perhaps just
saying 'also reduce index size when the index is modified frequently' or
such?



> ### Inlined WITH queries (Common table expressions)
> 
> Common table expressions (aka `WITH` queries) can now be automatically inlined
> in a query if they are a) not recursive, b) do not have any side-effects and
> c) are only referenced once in a later part of a query. These removes a known
> "optimization fence" that has existed since the introduction of the `WITH`
> clause in PostgreSQL 8.4
> 
> You can force a `WITH` query to be inlined using the `NOT MATERIALIZED` 
> clause,
> e.g.
> 
> ```
> WITH c AS NOT MATERIALIZED (
> SELECT * FROM a WHERE a.x % 4
> )
> SELECT * FROM c JOIN d ON d.y = a.x;
> ```

Wouldn't it be more important to reference how they can be *forced* to
be materialized? Because that'll be what users will need. And I think if
we reference NOT MATERIALIZED it also sounds like CTEs will not
automatically inlined ever.


> ### Pluggable Table Storage Interface
> 
> PostgreSQL 12 introduces the pluggable table storage interface that allows for
> the creation and use of different storage mechanisms for table storage. New
> access methods can be added to a PostgreSQL cluster using the  [`CREATE 
> ACCESS 
> METHOD`](https://www.postgresql.org/docs/devel/sql-create-access-method.html)
> and subsequently added to tables with the new `USING` clause on `CREATE 
> TABLE`.
> 
> A table storage interface can be defined by creating a new [table access 
> method](https://www.postgresql.org/docs/devel/tableam.html).
> 
> In PostgreSQL 12, the storage interface that is used by default is the `heap`
> access method, which is currently the only supported method.

I think s/which is currently the only supported method/which currently
is the only built-in method/ or such would be good. I don't know what
"supported" would actually mean here.


> ### Authentication
> 
> GSSAPI now supports client and server-side encryption and can be specified in
> the 
> [`pg_hba.conf`](https://www.postgresql.org/docs/devel/auth-pg-hba-conf.html)
> file using the `hostgssenc` and `hostnogssenc` record types. PostgreSQL 12 
> also
> allows for LDAP servers to be discovered based on `DNS SRV` records if
> PostgreSQL was compiled with OpenLDAP.

Is this really accurately categorized under authentication? Because it's
really ongoing encryption, as an alternative to tls?


> Noted Behavior Changes
> --
> 
> There are several changes introduced in PostgreSQL 12 that can affect the
> behavior as well as management of your ongoing operations. A few of these are
> noted below; for information about other changes, please review the
> "Migrating to Version 12" section of the [release 
> notes](https://www.postgresql.org/docs/devel/release-12.html).
> 
> 1. The `recovery.conf` configuration file is now merged into the main
> `postgresql.conf` file. PostgreSQL will not start if it detects that
> `recovery.conf` is present. To put PostgreSQL into a non-primary mode, you can
> use the `recovery.signal` and the `standby.signal` files.
> 
> You can read more about [archive 
> recovery](https://www.postgresql.org/docs/devel/runtime-config-wal.html#RUNTIME-CONFIG-WAL-ARCHIVE-RECOVERY)
>  here:
> 
> https://www.postgresql.org/docs/devel/runtime-config-wal.html#RUNTIME-CONFIG-WAL-ARCHIVE-RECOVERY
> 
> 2. Just-in-Time (JIT) compilation is now enabled by default.

I think we should probably list the removal of WITH OIDs.


Greetings,

Andres Freund

Re: pgindent run next week?

[Tom Lane]

Peter Eisentraut  writes:
> In my experience, changes to function declarations in header files
> happen a lot in forks.  So applying the pgindent change to backbranches
> would cause some trouble.

> On the other hand, it seems to me that patches that we backpatch between
> PostgreSQL branches should normally not touch function declarations in
> header files, since that would be an ABI break.  So by not applying the
> pgindent change in backbranches we don't lose anything.  And so it would
> be better to just leave things as they are.

Maybe we could wait awhile and see how much pain we find in back-patching
across this change.  I have to admit that the v10 pgindent changes have
not been as painful as I expected them to be, so maybe this round will
also prove to be just an annoyance not a major PITA for that.

Another thought is that, at least in principle, we could re-indent only
.c files not .h files in the back branches.  But I'm not sure I believe
your argument that forks are more likely to touch exposed extern
declarations than local static declarations.

regards, tom lane

Re: "long" type is not appropriate for counting tuples

[Tom Lane]

Peter Eisentraut  writes:
> Could we define int64 to be long long int on all platforms and just
> always use %lld?

Hmmm ... maybe.  Once upon a time we had to cope with compilers
that didn't have "long long", but perhaps that time is past.

Another conceivable hazard is somebody deciding that the world
needs a platform where "long long" is 128 bits.  I don't know
how likely that is to happen.

As a first step, we could try asking configure to compute
sizeof(long long) and seeing what the buildfarm makes of that.

regards, tom lane

Re: pgindent run next week?

[Peter Eisentraut]

On 2019-05-21 23:46, Tom Lane wrote:
>> Would we want to also apply this to the back branches to avoid spurious
>> conflicts?
> I think we should hold off on any talk of that until we get some results
> from Mark Dilger (or anyone else) on how much pain it would cause for
> people carrying private patches.

In my experience, changes to function declarations in header files
happen a lot in forks.  So applying the pgindent change to backbranches
would cause some trouble.

On the other hand, it seems to me that patches that we backpatch between
PostgreSQL branches should normally not touch function declarations in
header files, since that would be an ABI break.  So by not applying the
pgindent change in backbranches we don't lose anything.  And so it would
be better to just leave things as they are.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Is it safe to ignore the return value of SPI_finish and SPI_execute?

[Mark Dilger]

On Fri, May 17, 2019 at 6:12 PM Tom Lane  wrote:
>
> Mark Dilger  writes:
> > most places that use SPI_connect ... SPI_finish check the
> > return value of SPI_finish and elog if it failed.  There
> > are a few places that do not, and it is unclear to me
> > why this is safe.  SPI_finish appears to be needed to
> > clean up memory contexts.
>
> Well, looking through spi.c, the only failure return that SPI_finish
> actually has right now is from _SPI_begin_call:
>
> if (_SPI_current == NULL)
> return SPI_ERROR_UNCONNECTED;
>
> and if you're willing to posit that those callers did call SPI_connect,
> that's unreachable for them.  The more interesting cases such as
> failure within memory context cleanup would throw elog/ereport, so
> they're not at issue here.
>
> But I agree that not checking is crap coding practice, because there is
> certainly no reason why SPI_finish could not have other error-return
> cases in future.

Agreed.

> One reasonable solution would be to change the callers that got this
> wrong.  Another one would be to reconsider whether the error-return-code
> convention makes any sense at all here.  If we changed the above-quoted
> bit to be an ereport(ERROR), then we could say that SPI_finish either
> returns 0 or throws error, making it moot whether callers check, and
> allowing removal of now-useless checks from all the in-core callers.

Does this proposal of yours seem good enough for me to make a patch
based on this design?

> I don't think that actually doing that would be a great idea unless
> we went through all of the SPI functions and did it for every "unexpected"
> error case.  Is it worth the trouble?  Maybe, but I don't wanna do
> the legwork.

I would like to clean this up and submit a patch, so long as the general
solution seems acceptable to the pgsql-hackers list.

Just as background information:

I only hit this issue because I have been auditing the version 12 code
and adding __attribute__((warn_unused_result)) on non-void functions in
the tree and then checking each one that gets compiler warnings to see
if there is a bug inherent in the way it is being used.  These SPI_* functions
are the first ones I found where it seemed clearly wrong to me that the
return values were being ignored.  There have been many others where
ignoring the return value seemed acceptable given the way the function
is designed to work, and though I am not always happy with the design,
I'm not trying to go so far as redesigning large sections of the code.

> > The return value of SPI_execute is ignored in one spot:
> >   src/backend/utils/adt/xml.c circa line 2465.
>
> That seems like possibly a real bug.  It's certainly poor practice
> as things stand.

mark

Re: "long" type is not appropriate for counting tuples

[Peter Eisentraut]

On 2019-05-22 17:52, Tom Lane wrote:
> I don't really see how controlling snprintf is enough to get somewhere
> on this.  Sure we could invent some new always-64-bit length modifier
> and teach snprintf.c about it, but none of the other tools we use
> would know about it.  I don't want to give up compiler cross-checking
> of printf formats, do you?

Could we define int64 to be long long int on all platforms and just
always use %lld?

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: "long" type is not appropriate for counting tuples

[Peter Eisentraut]

On 2019-05-22 18:59, Andres Freund wrote:
> On May 22, 2019 7:40:28 AM PDT, Peter Eisentraut 
>  wrote:
>> On 2019-04-29 19:52, Andres Freund wrote:
>>> Hm. It appears that gettext supports expanding PRId64 PRIu64 etc in
>>> translated strings.
>>
>> That won't work in non-GNU gettext.
> 
> Which of those do currently work with postgres?

I don't know what the current situation is, but in the past we've been
getting complaints when using GNU-specific features, mostly from Solaris
I think.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: pgindent run next week?

[Mark Dilger]

On Wed, May 22, 2019 at 10:07 AM Tom Lane  wrote:
>
> I wrote:
> > Hearing no objections, I'll plan on running pgindent tomorrow sometime.
>
> And done.
>
> > The new underlying pg_bsd_indent (2.1) is available now from
> > https://git.postgresql.org/git/pg_bsd_indent.git
>
> Please update your local copy if you have one.
>
> regards, tom lane
>

I cloned, built and used the new pg_bsd_indent to format my fork of
PostgreSQL 11 (not the 9.1 or 9.5 forks I previously mentioned) and
it caused me no problems whatsoever.  I don't have a strong preference,
but I would vote in favor of running pgindent on the back branches
rather than against, since to the extent that I might need to move
patches between forks of different versions, it will be easier to do if
they have the same indentation.  (In practice, this probably won't
come up for me, since the older forks are defunct and unlikely to
be patched by me.)

mark

Re: pg_dump throwing "column number -1 is out of range 0..36" on HEAD

[Andrew Dunstan]

On 5/22/19 9:46 AM, Tom Lane wrote:
> Michael Paquier  writes:
>> Trying to do pg_dump[all] on a 9.5 or older server results in spurious
>> failures:
>> pg_dump: column number -1 is out of range 0..36
>> After looking around, the problem comes from
>> check_tuple_field_number(), more specifically from getTables() where
>> someone has forgotten to add NULL values for amname when querying
>> older server versions.
>> Attached is a patch to fix that.  I am not seeing other failures with
>> an instance that includes all the contents of installcheck, so it
>> seems that the rest is fine.
> Looks like the right fix.  I'm sad that the buildfarm did not catch
> this ... why wouldn't the cross-version-upgrade tests have seen it?


That's a good question.


It's in the output - see for example

and scroll down a bit.


But since this doesn't cause pg_dumpall to fail, it doesn't on its own
cause the buildfarm to fail either, and this is apparently sufficiently
benign to allow the tests to succeed.



cheers


andrew


-- 
Andrew Dunstanhttps://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Teach pg_upgrade test to honor NO_TEMP_INSTALL

[Andres Freund]

Hi,

On 2019-05-22 14:27:51 -0400, Tom Lane wrote:
> Andres Freund  writes:
> > On 2019-05-22 14:06:47 -0400, Tom Lane wrote:
> >> Not sure about that last bit.  pg_upgrade has the issue of possibly
> >> wanting to deal with 2 installations, unlike the rest of the tree,
> >> so I'm not sure that fixing its problem means there's something we
> >> need to change everywhere else.
> 
> > I'm not quite following? We need to move it into global scope to fix the
> > issue at hand (namely that we currently need to make install first, just
> > to get psql).  And at which scope could it be in master, other than
> > global?
> 
> Maybe I misunderstood you --- I thought you were talking about something
> like defining EXTRA_REGRESS_OPTS in Makefile.global.  If you mean
> running this unconditionally within test.sh, I've got no objection
> to that.

Oh, yes, that's what I meant.


> > I do think we will have to move it to the global scope in the back
> > branches too, because NO_TEMP_INSTALL does indeed fail without a global
> > install first (rather than using the temp install, as intended):
> 
> Agreed, we should fix it in all branches, because it seems like it's
> probably testing the wrong thing, ie using the later branch's psql
> to run the earlier branch's regression tests.

Ok, will do.

Greetings,

Andres Freund

Re: Why could GEQO produce plans with lower costs than the standard_join_search?

[Tom Lane]

Donald Dong  writes:
> I find the cost from cheapest_total_path->total_cost is different
> from the cost from  queryDesc->planstate->total_cost. What I saw was
> that GEQO tends to form paths with lower
> cheapest_total_path->total_cost (aka the fitness of the children).
> However, standard_join_search is more likely to produce a lower
> queryDesc->planstate->total_cost, which is the cost we get using
> explain.

> I wonder why those two total costs are different? If the total_cost
> from the planstate is more accurate, could we use that instead as the
> fitness in geqo_eval?

You're still asking us to answer hypothetical questions unsupported
by evidence.  In what case does that really happen?

regards, tom lane

Re: pg_dump throwing "column number -1 is out of range 0..36" on HEAD

[Tom Lane]

Andres Freund  writes:
> On 2019-05-22 14:17:41 -0400, Tom Lane wrote:
>> FWIW, I think that's a pretty awful idea, and the fact that some
>> people have had it before doesn't make it less awful.  It's giving
>> up the ability to detect errors-of-omission, which might easily
>> be harmful rather than harmless errors.

> Well, if we explicitly have to check for -1, it's not really an error of
> omission for everything. Yes, we could forget returning the amname in a
> newer version of the query, but given that we usually just forward copy
> the query that's not that likely.

No, the concerns I have are about (1) failure to insert the extra return
column into all branches where it's needed; (2) some unexpected run-time
problem causing the PGresult to not have the expected column.

I think we've had some discussions about restructuring those giant
if-nests so that they build up the query in pieces, making it possible
to write things along the lines of

appendPQExpBuffer(query,
  "SELECT c.tableoid, c.oid, c.relname, "
  // version-independent stuff here
  ...);
...
if (fout->remoteVersion >= 12)
appendPQExpBuffer(query, "am.amname, ");
else
appendPQExpBuffer(query, "NULL as amname, ");
...

I'm not sure if it'd work quite that cleanly when we need changes in the
FROM part, but certainly for newly-added result fields this would be
hugely better than repeating the whole query.  And yes, I'd still insist
that explicitly providing the alternative NULL value is not optional.


>> Hm.  But shouldn't we have gotten garbage output from the pg_dump run?

> What garbage? We'd take the column as NULL, and assume it doesn't have
> an assigned AM. Which is the right behaviour when dumping from < 12?

Oh, I see:

int
PQgetisnull(const PGresult *res, int tup_num, int field_num)
{
if (!check_tuple_field_number(res, tup_num, field_num))
return 1;/* pretend it is null */

which just happens to be the right thing --- in this case --- for
the back branches.

regards, tom lane

Re: ACL dump ordering broken as well for tablespaces

[Bossart, Nathan]

On 5/22/19, 12:16 AM, "Michael Paquier"  wrote:
> Attached is a patch to fix that, so as pg_dumpall does not complain
> when piling up GRANT commands using WITH GRANT OPTION.  Are there any
> complains to apply that down to 9.6?

The patch looks good to me.

> As the problem is kind of different than the database case, I wanted
> to spawn anyway a new thread, but I got a bonus question: what would
> it take to support pg_init_privs for databases and tablespaces?  If we
> could get that to work, then all the ACL-related queries built for all
> objects could make use of buildACLQueries(), which would avoid extra
> diffs in the dump code for dbs and tbspaces.

A bit of digging led me to the commit that removed databases and
tablespaces from pg_init_privs [0] and to a related thread [1].  IIUC
the problem is that using pg_init_privs for databases is complicated
by the ability to drop and recreate the template databases.

Nathan

[0] 
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=47f5bb9f539a7fff089724b1cbacc31613031895
[1] 
https://www.postgresql.org/message-id/9f25cb66-df67-8d81-ed6a-d18692a03410%402ndquadrant.com

Re: Why could GEQO produce plans with lower costs than the standard_join_search?

[Donald Dong]

Hi,

Thank you very much for the explanation! I think the join order
benchmark I used [1] is somewhat representative, however, I probably
didn't use the most accurate cost estimation.

I find the cost from cheapest_total_path->total_cost is different
from the cost from  queryDesc->planstate->total_cost. What I saw was
that GEQO tends to form paths with lower
cheapest_total_path->total_cost (aka the fitness of the children).
However, standard_join_search is more likely to produce a lower
queryDesc->planstate->total_cost, which is the cost we get using
explain.

I wonder why those two total costs are different? If the total_cost
from the planstate is more accurate, could we use that instead as the
fitness in geqo_eval?

[1] https://github.com/gregrahn/join-order-benchmark

Regards,
Donald Dong

> On May 7, 2019, at 4:44 PM, Tom Lane  wrote:
> 
> Donald Dong  writes:
>> I was expecting the plans generated by standard_join_search to have lower 
>> costs
>> than the plans from GEQO. But after the results I have from a join order
>> benchmark show that GEQO produces plans with lower costs most of the time!
> 
>> I wonder what is causing this observation? From my understanding,
>> standard_join_search is doing a complete search. So I'm not sure how the GEQO
>> managed to do better than that.
> 
> standard_join_search is *not* exhaustive; there's a heuristic that causes
> it not to consider clauseless joins unless it has to.
> 
> For the most part, GEQO uses the same heuristic (cf desirable_join()),
> but given the right sort of query shape you can probably trick it into
> situations where it will be forced to use a clauseless join when the
> core code wouldn't.  It'd still be surprising for that to come out with
> a lower cost estimate than a join order that obeys the heuristic,
> though.  Clauseless joins are generally pretty awful.
> 
> I'm a tad suspicious about the representativeness of your benchmark
> queries if you find this is happening "most of the time".
> 
>   regards, tom lane

Re: Teach pg_upgrade test to honor NO_TEMP_INSTALL

[Tom Lane]

Andres Freund  writes:
> On 2019-05-22 14:06:47 -0400, Tom Lane wrote:
>> Not sure about that last bit.  pg_upgrade has the issue of possibly
>> wanting to deal with 2 installations, unlike the rest of the tree,
>> so I'm not sure that fixing its problem means there's something we
>> need to change everywhere else.

> I'm not quite following? We need to move it into global scope to fix the
> issue at hand (namely that we currently need to make install first, just
> to get psql).  And at which scope could it be in master, other than
> global?

Maybe I misunderstood you --- I thought you were talking about something
like defining EXTRA_REGRESS_OPTS in Makefile.global.  If you mean
running this unconditionally within test.sh, I've got no objection
to that.

> I do think we will have to move it to the global scope in the back
> branches too, because NO_TEMP_INSTALL does indeed fail without a global
> install first (rather than using the temp install, as intended):

Agreed, we should fix it in all branches, because it seems like it's
probably testing the wrong thing, ie using the later branch's psql
to run the earlier branch's regression tests.

regards, tom lane

Re: pg_dump throwing "column number -1 is out of range 0..36" on HEAD

[Andres Freund]

Hi,

On 2019-05-22 14:17:41 -0400, Tom Lane wrote:
> Andres Freund  writes:
> > Wouldn't the better fix be to change
> > if (PQgetisnull(res, i, i_amname))
> > tblinfo[i].amname = NULL;
> > into
> > if (i_amname == -1 || PQgetisnull(res, i, i_amname))
> > tblinfo[i].amname = NULL;
> > it's much more scalable than adding useless columns everywhere, and we
> > already use that approach with i_checkoption (and at a number of other
> > places).
> 
> FWIW, I think that's a pretty awful idea, and the fact that some
> people have had it before doesn't make it less awful.  It's giving
> up the ability to detect errors-of-omission, which might easily
> be harmful rather than harmless errors.

Well, if we explicitly have to check for -1, it's not really an error of
omission for everything. Yes, we could forget returning the amname in a
newer version of the query, but given that we usually just forward copy
the query that's not that likely. And instead having to change a lot of
per-branch queries also adds potential for error, and also makes it more
painful to fix cross-branch bugs etc.


> >> Looks like the right fix.  I'm sad that the buildfarm did not catch
> >> this ... why wouldn't the cross-version-upgrade tests have seen it?
> 
> > I suspect we just didn't notice that it saw that:
> > as it's just a notice, not a failure.
> 
> Hm.  But shouldn't we have gotten garbage output from the pg_dump run?

What garbage? We'd take the column as NULL, and assume it doesn't have
an assigned AM. Which is the right behaviour when dumping from < 12?

Greetings,

Andres Freund

Re: Teach pg_upgrade test to honor NO_TEMP_INSTALL

[Andres Freund]

Hi,

On 2019-05-22 14:06:47 -0400, Tom Lane wrote:
> Andres Freund  writes:
> > Seems what we need to fix the immediate issue is to ressurect:
> 
> > # We need to make it use psql from our temporary installation,
> > # because otherwise the installcheck run below would try to
> > # use psql from the proper installation directory, which might
> > # be outdated or missing. But don't override anything else that's
> > # already in EXTRA_REGRESS_OPTS.
> > EXTRA_REGRESS_OPTS="$EXTRA_REGRESS_OPTS --bindir='$bindir'"
> > export EXTRA_REGRESS_OPTS
> 
> > and put that into global scope.
> 
> Not sure about that last bit.  pg_upgrade has the issue of possibly
> wanting to deal with 2 installations, unlike the rest of the tree,
> so I'm not sure that fixing its problem means there's something we
> need to change everywhere else.

I'm not quite following? We need to move it into global scope to fix the
issue at hand (namely that we currently need to make install first, just
to get psql).  And at which scope could it be in master, other than
global?

I do think we will have to move it to the global scope in the back
branches too, because NO_TEMP_INSTALL does indeed fail without a global
install first (rather than using the temp install, as intended):

On 11:

$ make -j16 -s uninstall
$ make -j16 -s temp-install
$ make -j16 -s -C src/bin/pg_upgrade/ check NO_TEMP_INSTALL=1
...
../../../src/test/regress/pg_regress 
--inputdir=/home/andres/src/postgresql-11/src/test/regress 
--bindir='/home/andres/build/postgres/11-assert//install/bin'--port=60851 
--dlpath=. --max-concurrent-tests=20  --port=60851 
--schedule=/home/andres/src/postgresql-11/src/test/regress/serial_schedule 
(using postmaster on /tmp/pg_upgrade_check-uEwhDs, port 60851)
== dropping database "regression" ==
sh: 1: /home/andres/build/postgres/11-assert//install/bin/psql: not found

$ make -j16 -s install
$ make -j16 -s -C src/bin/pg_upgrade/ check NO_TEMP_INSTALL=1 && echo success
...
success

As you can see it uses pg_regress etc from the temp installation, but
psql from the full installation.


> (IOW, keep an eye on the cross-version-upgrade tests while
> you mess with this...)

I will. If you refer to the buildfarm ones: As far as I can tell they
don't use test.sh at all. Which makes sense, as we need cleanup steps
inbetween the regression run and pg_upgrade, and test.sh doesn't allow
for that.

https://github.com/PGBuildFarm/client-code/blob/master/PGBuild/Modules/TestUpgradeXversion.pm

Greetings,

Andres Freund

Re: pg_dump throwing "column number -1 is out of range 0..36" on HEAD

[Tom Lane]

Andres Freund  writes:
> Wouldn't the better fix be to change
>   if (PQgetisnull(res, i, i_amname))
>   tblinfo[i].amname = NULL;
> into
>   if (i_amname == -1 || PQgetisnull(res, i, i_amname))
>   tblinfo[i].amname = NULL;
> it's much more scalable than adding useless columns everywhere, and we
> already use that approach with i_checkoption (and at a number of other
> places).

FWIW, I think that's a pretty awful idea, and the fact that some
people have had it before doesn't make it less awful.  It's giving
up the ability to detect errors-of-omission, which might easily
be harmful rather than harmless errors.

It does seem like we're overdue to rethink how pg_dump handles
cross-version query differences ... but inconsistently lobotomizing
its internal error detection is not a good start on that.

>> Looks like the right fix.  I'm sad that the buildfarm did not catch
>> this ... why wouldn't the cross-version-upgrade tests have seen it?

> I suspect we just didn't notice that it saw that:
> as it's just a notice, not a failure.

Hm.  But shouldn't we have gotten garbage output from the pg_dump run?

regards, tom lane

Re: Teach pg_upgrade test to honor NO_TEMP_INSTALL

[Tom Lane]

Andres Freund  writes:
> Seems what we need to fix the immediate issue is to ressurect:

> # We need to make it use psql from our temporary installation,
> # because otherwise the installcheck run below would try to
> # use psql from the proper installation directory, which might
> # be outdated or missing. But don't override anything else that's
> # already in EXTRA_REGRESS_OPTS.
> EXTRA_REGRESS_OPTS="$EXTRA_REGRESS_OPTS --bindir='$bindir'"
> export EXTRA_REGRESS_OPTS

> and put that into global scope.

Not sure about that last bit.  pg_upgrade has the issue of possibly
wanting to deal with 2 installations, unlike the rest of the tree,
so I'm not sure that fixing its problem means there's something we
need to change everywhere else.

(IOW, keep an eye on the cross-version-upgrade tests while
you mess with this...)

regards, tom lane

Re: pg_dump throwing "column number -1 is out of range 0..36" on HEAD

[Andres Freund]

Hi,

On 2019-05-22 09:46:19 -0400, Tom Lane wrote:
> Michael Paquier  writes:
> > Trying to do pg_dump[all] on a 9.5 or older server results in spurious
> > failures:
> > pg_dump: column number -1 is out of range 0..36
> 
> > After looking around, the problem comes from
> > check_tuple_field_number(), more specifically from getTables() where
> > someone has forgotten to add NULL values for amname when querying
> > older server versions.

Thanks for catching!


> > Attached is a patch to fix that.

Wouldn't the better fix be to change

if (PQgetisnull(res, i, i_amname))
tblinfo[i].amname = NULL;

into

if (i_amname == -1 || PQgetisnull(res, i, i_amname))
tblinfo[i].amname = NULL;

it's much more scalable than adding useless columns everywhere, and we
already use that approach with i_checkoption (and at a number of other
places).


> > Attached is a patch to fix that.  I am not seeing other failures with
> > an instance that includes all the contents of installcheck, so it
> > seems that the rest is fine.
> 
> Looks like the right fix.  I'm sad that the buildfarm did not catch
> this ... why wouldn't the cross-version-upgrade tests have seen it?

I suspect we just didn't notice that it saw that:

if (field_num < 0 || field_num >= res->numAttributes)
{
pqInternalNotice(>noticeHooks,
 "column number %d is out of 
range 0..%d",
 field_num, res->numAttributes 
- 1);
return false;
}

as it's just a notice, not a failure.

Greetings,

Andres Freund

Re: Teach pg_upgrade test to honor NO_TEMP_INSTALL

[Andres Freund]

Hi,

On 2019-05-22 10:58:54 -0400, Tom Lane wrote:
> Thomas Munro  writes:
> > After these commits (and Tom's commit "Un-break pg_upgrade regression
> > test."), cfbot broke:

I should just have finished working two hours earlier yesterday :(.


> > sh: 1: /usr/local/pgsql/bin/psql: not found
> 
> I can confirm that here: check-world passes as long as I've done
> "make install" beforehand ... but of course that should not be
> necessary.  If I blow away the install tree, pg_upgrade's
> check fails at

> ../../../src/test/regress/pg_regress --inputdir=. 
> --bindir='/home/postgres/testversion/bin'--port=54464 --dlpath=. 
> --max-concurrent-tests=20  --port=54464 --schedule=./parallel_schedule  
> (using postmaster on /tmp/pg_upgrade_check-Nitf3h, port 54464)
> == dropping database "regression" ==
> sh: /home/postgres/testversion/bin/psql: No such file or directory
> command failed: "/home/postgres/testversion/bin/psql" -X -c "DROP DATABASE IF 
> EXISTS \"regression\"" "postgres"
> 
> pg_regress is being told the wrong --bindir, ie
> the final install location not the temp install.

Yea, that's indeed the problem. I suspect that problem already exists in
the NO_TEMP_INSTALL solution committed in
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=47b3c26642e6850e8dfa7afe01db78320b11549e
It's just that nobody noticed that due to:

> (More generally, should we rearrange the buildfarm test
> sequence so it doesn't run "make install" till after the
> tests that aren't supposed to require an installed tree?)

Seems what we need to fix the immediate issue is to ressurect:

# We need to make it use psql from our temporary installation,
# because otherwise the installcheck run below would try to
# use psql from the proper installation directory, which might
# be outdated or missing. But don't override anything else that's
# already in EXTRA_REGRESS_OPTS.
EXTRA_REGRESS_OPTS="$EXTRA_REGRESS_OPTS --bindir='$bindir'"
export EXTRA_REGRESS_OPTS

and put that into global scope. While that'd be unnecessary when
invoking ./test.sh from commandline with explicitly already installed
binaries, it should be harmless there.

I wonder however, shouldn't the above stanza refer to $oldbindir?

I think we need to backpatch the move of the above outside the --install
path, because otherwise the buildfarm will break once we reorder the
buildfarm's scripts to do the make install later. Unless I miss
something?


> (More generally, should we rearrange the buildfarm test
> sequence so it doesn't run "make install" till after the
> tests that aren't supposed to require an installed tree?)

Seems like a good idea. On buildfarm's master the order is:

make_check() unless $delay_check;

# contrib is built under the standard build step for msvc
make_contrib() unless ($using_msvc);

make_testmodules()
  if (!$using_msvc && ($branch eq 'HEAD' || $branch ge 'REL9_5'));

make_doc() if (check_optional_step('build_docs'));

make_install();

# contrib is installed under standard install for msvc
make_contrib_install() unless ($using_msvc);

make_testmodules_install()
  if (!$using_msvc && ($branch eq 'HEAD' || $branch ge 'REL9_5'));

make_check() if $delay_check;

process_module_hooks('configure');

process_module_hooks('build');

process_module_hooks("check") unless $delay_check;

process_module_hooks('install');

process_module_hooks("check") if $delay_check;

run_bin_tests();

run_misc_tests();

... locale tests, ecpg, typedefs

Seems like we ought to at least move run_bin_tests, run_misc_tests up?


I'm not quite sure what the idea of $delay_check is. I found:
>  * a new --delay-check switch delays the check step until after
>install. This helps work around a bug or lack of capacity w.r.t.
>LD_LIBRARY_PATH on Alpine Linux

in an release announcement. But no further details.

Greetings,

Andres Freund

Re: pgindent run next week?

[Tom Lane]

I wrote:
> Hearing no objections, I'll plan on running pgindent tomorrow sometime.

And done.

> The new underlying pg_bsd_indent (2.1) is available now from
> https://git.postgresql.org/git/pg_bsd_indent.git

Please update your local copy if you have one.

regards, tom lane

Re: "long" type is not appropriate for counting tuples

[Andres Freund]

Hi,

On May 22, 2019 7:40:28 AM PDT, Peter Eisentraut 
 wrote:
>On 2019-04-29 19:52, Andres Freund wrote:
>> Hm. It appears that gettext supports expanding PRId64 PRIu64 etc in
>> translated strings.
>
>That won't work in non-GNU gettext.

Which of those do currently work with postgres?

Andres
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: Why does ExecComputeStoredGenerated() form a heap tuple

[Peter Eisentraut]

On 2019-05-20 04:23, David Rowley wrote:
> On Thu, 16 May 2019 at 05:44, Peter Eisentraut
>  wrote:
>> Updated patch attached.
> 
> This patch looks okay to me.

committed

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: VACUUM fails to parse 0 and 1 as boolean value

[Fujii Masao]

On Wed, May 22, 2019 at 4:47 PM Michael Paquier  wrote:
>
> On Wed, May 22, 2019 at 04:32:38AM +0900, Fujii Masao wrote:
> > I found that tab-completion also needs to be updated for ANALYZE
> > boolean options. I added that change for tab-completion into
> > the patch and am thinking to apply the attached patch.
>
> Looks fine to me at quick glance.

Thanks! Committed.

Regards,

-- 
Fujii Masao

Re: "long" type is not appropriate for counting tuples

[Tom Lane]

Andres Freund  writes:
> On May 22, 2019 7:39:41 AM PDT, Peter Eisentraut 
>  wrote:
>> On 2019-04-29 19:32, Tom Lane wrote:
>>> Another problem is that while "%lu" format specifiers are portable,
>>> INT64_FORMAT is a *big* pain, not least because you can't put it into
>>> translatable strings without causing problems.  To the extent that
>>> we could go over to "%zu" instead, maybe this could be finessed,
>>> but blind "s/long/int64/g" isn't going to be any fun.

>> Since we control our own snprintf now, this could probably be addressed
>> somehow, right?

> z is for size_t though? Not immediately first how It'd help us?

Yeah, z doesn't reliably translate to int64 either, so it's only useful
when the number you're trying to print is a memory object size.

I don't really see how controlling snprintf is enough to get somewhere
on this.  Sure we could invent some new always-64-bit length modifier
and teach snprintf.c about it, but none of the other tools we use
would know about it.  I don't want to give up compiler cross-checking
of printf formats, do you?

regards, tom lane

Re: Read-only access to temp tables for 2PC transactions

[Stas Kelvich]

> On 14 May 2019, at 12:53, Stas Kelvich  wrote:
> 
> Hi,
> 
> That is an attempt number N+1 to relax checks for a temporary table access
> in a transaction that is going to be prepared.
> 

Konstantin Knizhnik made off-list review of this patch and spotted
few problems.

* Incorrect reasoning that ON COMMIT DELETE truncate mechanism
should be changed in order to allow preparing transactions with
read-only access to temp relations. It actually can be be leaved
as is. Things done in previous patch for ON COMMIT DELETE may be
a performance win, but not directly related to this topic so I've
deleted that part.

* Copy-paste error with check conditions in
relation_open/relation_try_open.

Fixed version attached.

--
Stas Kelvich
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company



2PC-ro-temprels-v2.patch
Description: Binary data

upgrades in row-level locks can deadlock

[Oleksii Kliukin]

Hello,

I have recently observed a deadlock on one of our production servers related
to locking only a single row in a job table. There were two functions involved
in the deadlock, the first one acquires a “for key share” lock on the row that
represents the job it works on and subsequently updates it with the job’s end
time (we need multiple jobs to be operating on a single row concurrently,
that’s why there is a "for key share" lock). The other function starts by
acquiring the “for update” lock on the job row and then performs actions that
should not be run in parallel with other jobs.

The deadlock can be easily reproduced with the following statements. The
queries run against a table job (id integer primary key, name text) with a
single row of (1,'a'))

X1: select id from job where name = 'a' for key share;
Y: select id from job where name = 'a' for update; -- starts waiting for X1
X2: select id from job where name = 'a' for key share;
X1: update job set name = 'b' where id = 1;
X2: update job set name = 'c' where id = 1; -- starts waiting for X1
X1: rollback;

At this point, Y is terminated by the deadlock detector:

"deadlock detected",
Process 53937 waits for ShareLock on transaction 488; blocked by process 53953.
Process 53953 waits for ExclusiveLock on tuple (0,1) of relation 16386 of 
database 12931;
blocked by process 53937.
Process 53937: select id from job where name = 'a' for update;
Process 53953: update job set name = 'c' where id = 1;",

The deadlock is between X2 and Y. Y waits for X2 to finish, as X2 holds a "key
share" lock, incompatible with "for update" that Y attempts to acquire. On the
other hand, X2 needs to acquire the row lock to perform the update, and that
is a two-phase process: first, get the tuple lock and then wait for
conflicting transactions to finish, releasing the tuple lock afterward. X2
tries to acquire the tuple lock, but it is owned by Y. PostgreSQL detects the
deadlock and terminates Y.

Such a deadlock only occurs when three or more sessions locking the same row
are present and the lock is upgraded in at least one session. With only two
sessions the upgrade does not go through the lock manager, as there are no
conflicts with locks stored in the tuple. 

That gave me an idea on how to change PostgreSQL to avoid deadlocking under
the condition above. When detecting the lock upgrade from the multixact, we
can avoid acquiring the tuple lock; however, we should still wait for the
mutlixact members that hold conflicting locks, to avoid acquiring incompatible
ones.

The patch is attached. I had to tweak heap_update and heap_delete alongside
the heap_lock_tuple, as they acquire row locks as well. I am not very happy
with overloading DoesMultiXactIdConflict with another function to check if
current transaction id is among the multixact members, perhaps it is worth to
have a separate function for this. We can figure this out if we agree this is
the problem that needs to be solved and on the solution. The other possible
objection is related to the statement from README.tuplock that we need to go
through the lock manager to avoid starving waiting exclusive-lockers. Since
this patch omits the tuple lock only when the lock upgrade happens, it does
limit the starvation condition to the cases when the lock compatible with the
one the waiting process asks for is acquired first and then gets upgraded to
the incompatible one. Since under present conditions the same operation will
likely deadlock and cancel the exclusive waiter altogether, I don't see this
as a strong objection.

Cheers,
Oleksii



deadlock_on_row_lock_upgrades_v1.diff
Description: Binary data

Re: "long" type is not appropriate for counting tuples

[Andres Freund]

Hi,

On May 22, 2019 7:39:41 AM PDT, Peter Eisentraut 
 wrote:
>On 2019-04-29 19:32, Tom Lane wrote:
>> Another problem is that while "%lu" format specifiers are portable,
>> INT64_FORMAT is a *big* pain, not least because you can't put it into
>> translatable strings without causing problems.  To the extent that
>> we could go over to "%zu" instead, maybe this could be finessed,
>> but blind "s/long/int64/g" isn't going to be any fun.
>
>Since we control our own snprintf now, this could probably be addressed
>somehow, right?

z is for size_t though? Not immediately first how It'd help us?

Andres
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: PG 12 draft release notes

[Bruce Momjian]

On Tue, May 21, 2019 at 05:00:31PM -0400, Tom Lane wrote:
> Bruce Momjian  writes:
> > On Mon, May 20, 2019 at 08:48:15PM -0400, Tom Lane wrote:
> >> Yes, this should be in "source code".  I think it should be merged
> >> with a391ff3c and 74dfe58a into something like
> >> 
> >> Allow extensions to create planner support functions that
> >> can provide function-specific selectivity, cost, and
> >> row-count estimates that can depend on the function arguments.
> >> Support functions can also transform WHERE clauses involving
> >> an extension's functions and operators into indexable clauses
> >> in ways that the core code cannot for lack of detailed semantic
> >> knowledge of those functions/operators.
> 
> > The new text is:
> 
> > Add support function capability to improve optimizer estimates
> > for functions (Tom Lane)
> 
> > This allows extensions to create planner support functions that
> > can provide function-specific selectivity, cost, and row-count
> > estimates that can depend on the function arguments.  Also, improve
> > in-core estimates for generate_series(),
> > unnest(), and functions that return boolean
> > values.
> 
> Uh ... you completely lost the business about custom indexable clauses.
> I agree with Andres that that's the most important aspect of this.

Oh, I see what you mean now. I have updated the docs and moved the item
to Source Code:

   
Add support function capability to improve optimizer estimates,
inlining, and indexing for functions (Tom Lane)
   

   
This allows extensions to create planner support functions that
can provide function-specific selectivity, cost, and row-count
estimates that can depend on the function's arguments.  Support
functions can also supply simplified representations and index
conditions, greatly expanding optimization possibilities.
   

> > Notice that there are some improvments in in-core functions. Should this
> > still be moved to the source code section?
> 
> I doubt that that's worth mentioning at all.  It certainly isn't a
> reason not to move this to the source-code section, because that's
> where we generally put things that are of interest for improving
> extensions, which is what this mainly is.

In-core function mention removed.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +

Re: "WIP: Data at rest encryption" patch and, PostgreSQL 11-beta3

[Robert Haas]

On Wed, Apr 24, 2019 at 10:48 AM Antonin Houska  wrote:
> Attached is the next version. It tries to address various problems pointed out
> upthread, including documentation.

It's not immediately evident to me, perhaps because I haven't looked
at this stuff in a while, what the purpose of 0001 and 0002 is.  I
think it'd be helpful if you would generate these patches with 'git
format-patch' and include a meaningful commit message in each patch of
the series.

+   
+This setting specifies path to executable file that returns
+either encryption_key= for key
+and encryption_password= for password.
+   

This is incomprehensible.  Executables don't return strings.  Also,
you can return "either a or b" or you can return "both a and b," but
you can't return "either a and b".

- Build with support for SSL (encrypted)
- connections. This requires the OpenSSL
- package to be installed.  configure will check
- for the required header files and libraries to make sure that
+ Build with support for on-disk data encryption
+ or SSL (encrypted) connections. This requires
+ the OpenSSL package to be
+ installed.  configure will check for the
+ required header files and libraries to make sure that

I think we should instead just remove the word 'connections'.  We
don't want to have multiple places in the documentation listing all
the things for which OpenSSL is required.

Generally, the documentation changes here have a lot of stuff about
key-or-password, which is quite confusing.  It seems like the idea is
that you accept either a key or a password from which a key is
derived, but I don't think that's explained super-clearly, and I
wonder whether it's unnecessary complexity.  Why not just require a
key always?

Another general point is that the documentation, comments, and README
need some work on the English.  They contain lots of information, but
they read somewhat awkwardly and are hard to understand in some
places.

Wherever you are adding a large hunk of code into the middle of an
existing function (e.g. XLogWrite), please consider putting the logic
in a new function and calling it from the original function, instead
of just inlining the entire thing.

xloginsert.c adds a new #include even though there are no other
changes to that file.  That should not be necessary.  If it is, you've
broken the principle the header files are compilable independently of
other header files (except postgres.h, which is a prerequisite for
every header file).

-XLogRead(char *buf, int segsize, TimeLineID tli, XLogRecPtr startptr,
- Size count)
+XLogRead(char *buf, int segsize, TimeLineID tli, XLogRecPtr startptr,
Size count)

Spurious hunk.  Please try to avoid unnecessary changes.

+ * XXX Currently the function is only called with startptr at page
+ * boundary. If it should change, encryption needs to reflect this fact,
+ * i.e. read data from the beginning of the page. Actually this is a
+ * reason to adjust and use walsender.c:XLogRead().

This comment and the similar comment at the end of the function are
not very clear.   It sorta sounds, though, like maybe the decryption
logic should move from here to the caller or something.  You seem to
be saying that, after your changes, this function depends on the
caller using it in a certain way, which is often a sign that you
haven't abstracted things in the right way.  And if walsender.c's
version of the logic is better, why not also do that stuff here?

@@ -52,7 +54,6 @@

 uint32 bootstrap_data_checksum_version = 0; /* No checksum */

-
 #define ALLOC(t, c) \
  ((t *) MemoryContextAllocZero(TopMemoryContext, (unsigned)(c) * sizeof(t)))

Another useless hunk.

+ {
+ RelFileNode fromNode = {srctablespace, src_dboid, InvalidOid};
+ RelFileNode toNode = {dsttablespace, dboid, InvalidOid};
+
+ copydir(srcpath, dstpath, , );
+ }

I suggest instead declaring these variables in the surrounding scope
and initializing them here.  This style is unlike most PostgreSQL
code.

+ case WAIT_EVENT_KDF_FILE_READ:
+ event_name = "KDFFileRead";
+ break;

New wait events need documentation.

+are encrypted differently. Furthermore, as the Postgres page starts with LSN,
+even if only the last encryption block of the page changes, the whole cipher
+text of the page will be different after the next encryption.

But for a hint-bit-only change, this would not be true, unless we
force wal_log_hints.  Sounds like we're doing that, but it might be
worth mentioning it here also.

There's also the question of unlogged tables, which do not have an
LSN.  This logic wouldn't apply there, which might be a problem.

+If the encryption is enabled, the following requirements need to be taken into
+account:
+
+1. The file buffer cannot be positioned at arbitrary offset of the file. If

I think it's no good at all to think about enforcing requirements like
this only when data_encryption is used.  That's just going to lead to

Re: Patch to fix write after end of array in hashed agg initialization

[Tom Lane]

Colm McHugh  writes:
> Attached is a patch for a write after allocated memory which we found in
> testing. Its an obscure case but can happen if the same column is used in
> different grouping keys, as in the example below, which uses tables from
> the regress test suite (build with --enable-cassert in order to turn on
> memory warnings). Patch is against master.

I confirm the appearance of the memory-overwrite warnings in HEAD.

It looks like the bad code is (mostly) the fault of commit b5635948.
Andrew, can you take a look at this fix?

regards, tom lane

Re: Teach pg_upgrade test to honor NO_TEMP_INSTALL

[Tom Lane]

Thomas Munro  writes:
> After these commits (and Tom's commit "Un-break pg_upgrade regression
> test."), cfbot broke:

> sh: 1: /usr/local/pgsql/bin/psql: not found

I can confirm that here: check-world passes as long as I've done
"make install" beforehand ... but of course that should not be
necessary.  If I blow away the install tree, pg_upgrade's
check fails at

../../../src/test/regress/pg_regress --inputdir=. 
--bindir='/home/postgres/testversion/bin'--port=54464 --dlpath=. 
--max-concurrent-tests=20  --port=54464 --schedule=./parallel_schedule  
(using postmaster on /tmp/pg_upgrade_check-Nitf3h, port 54464)
== dropping database "regression" ==
sh: /home/postgres/testversion/bin/psql: No such file or directory
command failed: "/home/postgres/testversion/bin/psql" -X -c "DROP DATABASE IF 
EXISTS \"regression\"" "postgres"

pg_regress is being told the wrong --bindir, ie
the final install location not the temp install.

(More generally, should we rearrange the buildfarm test
sequence so it doesn't run "make install" till after the
tests that aren't supposed to require an installed tree?)

regards, tom lane

Re: PostgreSQL 12 Beta 1 press release draft

[Robbie Harwood]

"Jonathan S. Katz"  writes:

> Attached is a draft of the PG12 Beta 1 press release that is going out
> this Thursday. The primary goals of this release announcement are to
> introduce new features, enhancements, and changes that are available in
> PG12, as well as encourage our users to test and provide feedback to
> help ensure the stability of the release.

Awesome!

> ### Authentication
>
> GSSAPI now supports client and server-side encryption and can be
> specified in the
> [`pg_hba.conf`](https://www.postgresql.org/docs/devel/auth-pg-hba-conf.html)
> file using the `hostgssenc` and `hostnogssenc` record
> types. PostgreSQL 12 also allows for LDAP servers to be discovered
> based on `DNS SRV` records if PostgreSQL was compiled with OpenLDAP.

Maybe a better title for this section would be "Authentication /
Encryption" or maybe even "Connection security"?  I get that this is a
press release though, so feel free to disregard.

Thanks,
--Robbie


signature.asc
Description: PGP signature

Re: FullTransactionId changes are causing portability issues

[Tom Lane]

I wrote:
> Our Solaris packager reports that 12beta1 is failing to build for him
> on some Solaris variants:

>> The link failure is:
>> ---
>> Undefinedfirst referenced
>> symbol   in file
>> ReadNextFullTransactionId   pg_checksums.o
>> ld: fatal: symbol referencing errors. No output written to pg_checksums
>> ---

On looking closer, the fix is simple and matches what we've done
elsewhere: transam.h needs to have "#ifndef FRONTEND" to protect
its static inline function from being compiled into frontend code.

So the disturbing thing here is that we no longer have any active
buildfarm members that can build HEAD but have the won't-elide-
unused-static-functions problem.  Clearly we'd better close that
gap somehow ... anyone have an idea about how to test it better?

regards, tom lane

Re: "long" type is not appropriate for counting tuples

[Peter Eisentraut]

On 2019-04-29 19:52, Andres Freund wrote:
> Hm. It appears that gettext supports expanding PRId64 PRIu64 etc in
> translated strings.

That won't work in non-GNU gettext.

> Perhaps we should implement them in our printf, and
> then replace all use of INT64_FORMAT with that?

But this might.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: "long" type is not appropriate for counting tuples

[Peter Eisentraut]

On 2019-04-29 19:32, Tom Lane wrote:
> Another problem is that while "%lu" format specifiers are portable,
> INT64_FORMAT is a *big* pain, not least because you can't put it into
> translatable strings without causing problems.  To the extent that
> we could go over to "%zu" instead, maybe this could be finessed,
> but blind "s/long/int64/g" isn't going to be any fun.

Since we control our own snprintf now, this could probably be addressed
somehow, right?

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

FullTransactionId changes are causing portability issues

[Tom Lane]

Our Solaris packager reports that 12beta1 is failing to build for him
on some Solaris variants:

> The link failure is:

> ---
> Undefined first referenced
>  symbol   in file
> ReadNextFullTransactionId   pg_checksums.o
> ld: fatal: symbol referencing errors. No output written to pg_checksums
> ---

> Now, ReadNextFullTransactionId() is implemented in
> src/backend/access/transam/varsup.c but I cannot see varsop.o being
> included in any of the libraries pg_checksum is linked against
> (libpgcommon.a and libpgport.a).

> When I check the pg_checksum.o I find that it references
> ReadNextFullTransactionId on the platforms that fail but not where it
> doesn't. The failed platforms are all sparc variants plus 64-bit x86
> on Solaris 11.

> The compiler used in Sun Studio 12u1, very old and and I can try to
> upgrade and see if that helps.
> [ it didn't ]

I'm a bit mystified why we did not see this problem in the buildfarm,
especially since we have at least one critter (damselfly) running an
OpenSolaris variant.  Nonetheless, it sure looks like a "somebody
was sloppy about frontend/backend separation" problem.

Fix ideas anyone?  I think we need to not only solve the immediate
problem (which might just take an #ifndef FRONTEND somewhere) but
also close the testing gap so we don't get blindsided like this
again.

regards, tom lane

Re: pg_dump throwing "column number -1 is out of range 0..36" on HEAD

[Tom Lane]

Michael Paquier  writes:
> Trying to do pg_dump[all] on a 9.5 or older server results in spurious
> failures:
> pg_dump: column number -1 is out of range 0..36

> After looking around, the problem comes from
> check_tuple_field_number(), more specifically from getTables() where
> someone has forgotten to add NULL values for amname when querying
> older server versions.

> Attached is a patch to fix that.  I am not seeing other failures with
> an instance that includes all the contents of installcheck, so it
> seems that the rest is fine.

Looks like the right fix.  I'm sad that the buildfarm did not catch
this ... why wouldn't the cross-version-upgrade tests have seen it?

regards, tom lane

Re: SQL statement PREPARE does not work in ECPG

[Tom Lane]

Michael Paquier  writes:
> This patch seems to have little incidence on the stability, but FWIW I
> am not cool with the concept of asking for objections and commit a
> patch only 4 hours after-the-fact, particularly after feature freeze.

FWIW, I figured it was okay since ECPG has essentially no impact on
the rest of the system.  The motivation for having feature freeze is
to get us to concentrate on stability, but any new bugs in ECPG
aren't going to affect the stability of anything else.

Also, I don't think it's that hard to look at this as a bug fix
rather than a new feature.  The general expectation is that ECPG
can parse any command the backend can --- that's why we went to
all the trouble of automatically building its grammar from the
backend's.  So I was surprised to hear that it didn't work on
some EXECUTE variants, and filling in that gap doesn't seem like a
"new feature" to me.  Note the lack of any documentation additions
in the patch.

regards, tom lane

Re: POC: Cleaning up orphaned files using undo logs

[Amit Kapila]

On Wed, May 22, 2019 at 5:47 PM Robert Haas  wrote:
>
> On Wed, May 22, 2019 at 7:17 AM Amit Kapila  wrote:
> > > > +/* Extract xid from a value comprised of epoch and xid  */
> > > > +#define GetXidFromEpochXid(epochxid) \
> > > > + ((uint32) (epochxid) & 0X)
> > > > +
> > > > +/* Extract epoch from a value comprised of epoch and xid  */
> > > > +#define GetEpochFromEpochXid(epochxid)   \
> > > > + ((uint32) ((epochxid) >> 32))
> > > > +
> > >
> > > Why do these exist?
> > >
> >
> > We don't need the second one (GetEpochFromEpochXid), but the first one
> > is required.  Basically, the oldestXidHavingUndo computation does
> > consider oldestXmin (which is still a TransactionId) as we can't
> > retain undo which is 2^31 transactions old due to other limitations
> > like clog/snapshots still has a limit of 4-byte transaction ids.
> > Slightly unrelated, but we do want to improve the undo retention in a
> > subsequent version such that we won't allow pending undo for
> > transaction whose age is more than 2^31.
>
> The point is that we now have EpochFromFullTransactionId and
> XidFromFullTransactionId.  You shouldn't be inventing your own version
> of that infrastructure.  Use FullTransactionId, not a uint64, and then
> use the functions for dealing with full transaction IDs from
> transam.h.
>

Okay, I misunderstood the comment.  I'll change accordingly.  Thanks
for pointing out.



-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Re: POC: Cleaning up orphaned files using undo logs

[Robert Haas]

On Wed, May 22, 2019 at 7:17 AM Amit Kapila  wrote:
> > > +/* Extract xid from a value comprised of epoch and xid  */
> > > +#define GetXidFromEpochXid(epochxid) \
> > > + ((uint32) (epochxid) & 0X)
> > > +
> > > +/* Extract epoch from a value comprised of epoch and xid  */
> > > +#define GetEpochFromEpochXid(epochxid)   \
> > > + ((uint32) ((epochxid) >> 32))
> > > +
> >
> > Why do these exist?
> >
>
> We don't need the second one (GetEpochFromEpochXid), but the first one
> is required.  Basically, the oldestXidHavingUndo computation does
> consider oldestXmin (which is still a TransactionId) as we can't
> retain undo which is 2^31 transactions old due to other limitations
> like clog/snapshots still has a limit of 4-byte transaction ids.
> Slightly unrelated, but we do want to improve the undo retention in a
> subsequent version such that we won't allow pending undo for
> transaction whose age is more than 2^31.

The point is that we now have EpochFromFullTransactionId and
XidFromFullTransactionId.  You shouldn't be inventing your own version
of that infrastructure.  Use FullTransactionId, not a uint64, and then
use the functions for dealing with full transaction IDs from
transam.h.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: Remove page-read callback from XLogReaderState.

[Antonin Houska]

Kyotaro HORIGUCHI  wrote:

> Hello. Thank you for looking this.
> ...
> Yeah, I'll register this, maybe the week after next week.

I've checked the new version. One more thing I noticed now is that XLR_STATE.j
is initialized to zero, either by XLogReaderAllocate() which zeroes the whole
reader state, or later by XLREAD_RESET. This special value then needs to be
handled here:

#define XLR_SWITCH()\
do {\
if ((XLR_STATE).j)  \
goto *((void *) (XLR_STATE).j); \
XLR_CASE(XLR_INIT_STATE);   \
} while (0)

I think it's better to set the label always to (&_INIT_STATE) so that
XLR_SWITCH can perform the jump unconditionally.

Attached is also an (unrelated) comment fix proposal.

-- 
Antonin Houska
Web: https://www.cybertec-postgresql.com

diff --git a/src/include/access/xlogreader.h b/src/include/access/xlogreader.h
index 262bf0e77f..cb864a86d8 100644
--- a/src/include/access/xlogreader.h
+++ b/src/include/access/xlogreader.h
@@ -124,7 +124,7 @@ struct XLogReaderState
 	XLogRecPtr	currRecPtr;		/* beginning of the WAL record being read */
 
 	/* return from page reader */
-	int32		readLen;		/* bytes acutually read, must be larger than
+	int32		readLen;		/* bytes acutually read, must be at least
  * loadLen. -1 on error. */
 	TimeLineID	readPageTLI;	/* TLI for data currently in readBuf */
 

Re: accounting for memory used for BufFile during hash joins

[Tomas Vondra]

On Tue, May 21, 2019 at 05:38:50PM -0700, Melanie Plageman wrote:

On Wed, May 8, 2019 at 8:08 AM Tomas Vondra 
wrote:


On Tue, May 07, 2019 at 05:30:27PM -0700, Melanie Plageman wrote:
>   One thing I was a little confused by was the nbatch_inmemory member
>   of the hashtable.  The comment in ExecChooseHashTableSize says that
>   it is determining the number of batches we can fit in memory.  I
>   thought that the problem was the amount of space taken up by the
>   BufFile data structure itself--which is related to the number of
>   open BufFiles you need at a time. This comment in
>   ExecChooseHashTableSize makes it sound like you are talking about
>   fitting more than one batch of tuples into memory at a time. I was
>   under the impression that you could only fit one batch of tuples in
>   memory at a time.

I suppose you mean this chunk:

/*
 * See how many batches we can fit into memory (driven mostly by size
 * of BufFile, with PGAlignedBlock being the largest part of that).
 * We need one BufFile for inner and outer side, so we count it twice
 * for each batch, and we stop once we exceed (work_mem/2).
 */
while ((nbatch_inmemory * 2) * sizeof(PGAlignedBlock) * 2
   <= (work_mem * 1024L / 2))
nbatch_inmemory *= 2;

Yeah, that comment is a bit confusing. What the code actually does is
computing the largest "slice" of batches for which we can keep the
BufFile structs in memory, without exceeding work_mem/2.

Maybe the nbatch_inmemory should be renamed to nbatch_slice, not sure.



I definitely would prefer to see hashtable->nbatch_inmemory renamed to
hashtable->nbatch_slice--or maybe hashtable->nbuff_inmemory?

I've been poking around the code for awhile today, and, even though I
know that the nbatch_inmemory is referring to the buffiles that can
fit in memory, I keep forgetting and thinking it is referring to the
tuple data that can fit in memory.



That's a fair point. I think nbatch_slice is a good name.


It might be worth explicitly calling out somewhere in the comments
that overflow slices will only be created either when the number of
batches was underestimated as part of ExecHashIncreaseNumBatches and
the new number of batches exceeds the value for
hashtable->nbatch_inmemory or when creating the hashtable initially
and the number of batches exceeds the value for
hashtable->nbatch_inmemory (the name confuses this for me at hashtable
creation time especially) -- the number of actual buffiles that can be
managed in memory.



Yes, this definitely needs to be explained somewhere - possibly in a
comment at the beginning of nodeHash.c or something like that.

FWIW I wonder if this "slicing" would be useful even with correct
estimates. E.g. let's say we can fit 128 batches into work_mem, but we
expect to need 256 (and it's accurate). At that point it's probably too
aggressive to disable hash joins - a merge join is likely more expensive
than just using the slicing. But that should be a cost-based decision.





Attached is an updated patch, fixing this. I tried to clarify some of
the comments too, and I fixed another bug I found while running the
regression tests. It's still very much a crappy PoC code, though.



So, I ran the following example on master and with your patch.

drop table foo;
drop table bar;
create table foo(a int, b int);
create table bar(c int, d int);
insert into foo select i, i from generate_series(1,1)i;
insert into bar select 1, 1 from generate_series(1,1000)i;
insert into bar select i%3, i%3 from generate_series(1000,1)i;
insert into foo select 1,1 from generate_series(1,1000)i;
analyze foo; analyze bar;
set work_mem=64;

On master, explain analyze looked like this

postgres=# explain analyze verbose select * from foo, bar where a = c;
   QUERY PLAN

--
Hash Join  (cost=339.50..53256.27 rows=4011001 width=16) (actual
time=28.962..1048.442 rows=4008001 loops=1)
  Output: foo.a, foo.b, bar.c, bar.d
  Hash Cond: (bar.c = foo.a)
  ->  Seq Scan on public.bar  (cost=0.00..145.01 rows=10001 width=8)
(actual time=0.030..1.777 rows=10001 loops=1)
Output: bar.c, bar.d
  ->  Hash  (cost=159.00..159.00 rows=11000 width=8) (actual
time=12.285..12.285 rows=11000 loops=1)
Output: foo.a, foo.b
Buckets: 2048 (originally 2048)  Batches: 64 (originally 16)
Memory Usage: 49kB
->  Seq Scan on public.foo  (cost=0.00..159.00 rows=11000 width=8)
(actual time=0.023..3.786 rows=11000 loops=1)
  Output: foo.a, foo.b
Planning Time: 0.435 ms
Execution Time: 1206.904 ms
(12 rows)

and with your patch, it looked like this.

postgres=# explain analyze verbose select * from foo, bar where a = c;
   QUERY PLAN

Re: POC: Cleaning up orphaned files using undo logs

[Amit Kapila]

On Tue, May 21, 2019 at 10:47 PM Andres Freund  wrote:
>
> Hi,
>
> On 2019-05-05 10:28:21 +0530, Amit Kapila wrote:
> > From 5d9e179bd481b5ed574b6e7117bf3eb62b5dc003 Mon Sep 17 00:00:00 2001
> > From: Amit Kapila 
> > Date: Sat, 4 May 2019 16:52:01 +0530
> > Subject: [PATCH] Allow undo actions to be applied on rollbacks and discard
> >  unwanted undo.
>
> I think this needs to be split into some constituent parts, to be
> reviewable.

Okay.

> Discussing 270kb of patch at once is just too much. My first
> guess for a viable split would be:
>
> 1) undoaction related infrastructure
> 2) xact.c integration et al
> 3) binaryheap changes etc
> 4) undo worker infrastructure
>
> It probably should be split even further, by moving things like:
> - oldestXidHavingUndo infrastructure
> - discard infrastructure
>

Okay, I will think about this and split the patch.

> Some small remarks:
>
> >
> > + {
> > + {"disable_undo_launcher", PGC_POSTMASTER, DEVELOPER_OPTIONS,
> > + gettext_noop("Decides whether to launch an undo 
> > worker."),
> > + NULL,
> > + GUC_NOT_IN_SAMPLE
> > + },
> > + _undo_launcher,
> > + false,
> > + NULL, NULL, NULL
> > + },
> > +
>
> We don't normally formulate GUCs in the negative like that. C.F.
> autovacuum etc.
>

Okay, will change.  Actually, this is just for development purpose.
It can help us in testing cases where we have pushed the undo, but it
won't apply, so whenever the foreground process encounter such a
transaction, it will perform the page-wise undo.  I am not 100% sure
if we need this for the final version.  Similarly, for testing
purpose, we might need enable_discard_worker to test the cases where
discard doesn't happen for a long time.

>
> > +/* Extract xid from a value comprised of epoch and xid  */
> > +#define GetXidFromEpochXid(epochxid) \
> > + ((uint32) (epochxid) & 0X)
> > +
> > +/* Extract epoch from a value comprised of epoch and xid  */
> > +#define GetEpochFromEpochXid(epochxid)   \
> > + ((uint32) ((epochxid) >> 32))
> > +
>
> Why do these exist?
>

We don't need the second one (GetEpochFromEpochXid), but the first one
is required.  Basically, the oldestXidHavingUndo computation does
consider oldestXmin (which is still a TransactionId) as we can't
retain undo which is 2^31 transactions old due to other limitations
like clog/snapshots still has a limit of 4-byte transaction ids.
Slightly unrelated, but we do want to improve the undo retention in a
subsequent version such that we won't allow pending undo for
transaction whose age is more than 2^31.

>
> >   /* End-of-list marker */
> >   {
> >   {NULL, 0, 0, NULL, NULL}, NULL, false, NULL, NULL, NULL
> > @@ -2923,6 +2935,16 @@ static struct config_int ConfigureNamesInt[] =
> >   5000, 1, INT_MAX,
> >   NULL, NULL, NULL
> >   },
> > + {
> > + {"rollback_overflow_size", PGC_USERSET, RESOURCES_MEM,
> > + gettext_noop("Rollbacks greater than this size are 
> > done lazily"),
> > + NULL,
> > + GUC_UNIT_MB
> > + },
> > + _overflow_size,
> > + 64, 0, MAX_KILOBYTES,
> > + NULL, NULL, NULL
> > + },
>
> rollback_foreground_size? rollback_background_size? I don't think
> overflow is particularly clear.
>

How about rollback_undo_size or abort_undo_size or
undo_foreground_size or pending_undo_size?

>
> > @@ -1612,6 +1635,85 @@ FinishPreparedTransaction(const char *gid, bool 
> > isCommit)
> >
> >   MyLockedGxact = NULL;
> >
> > + /*
> > +  * Perform undo actions, if there are undologs for this transaction. 
> > We
> > +  * need to perform undo actions while we are still in transaction. 
> > Never
> > +  * push rollbacks of temp tables to undo worker.
> > +  */
> > + for (i = 0; i < UndoPersistenceLevels; i++)
> > + {
>
> This should be in a separate function. And it'd be good if more code
> between this and ApplyUndoActions() would be shared.
>

makes sense, will try.

>
> > + /*
> > +  * Here, we just detect whether there are any pending undo actions so 
> > that
> > +  * we can skip releasing the locks during abort transaction.  We don't
> > +  * release the locks till we execute undo actions otherwise, there is 
> > a
> > +  * risk of deadlock.
> > +  */
> > + SetUndoActionsInfo();
>
> This function name is so generic that it gives the reader very little
> information about why it's called here (and in other similar places).
>

NeedToPerformUndoActions()? UndoActionsRequired()?

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

RE: Global shared meta cache

[Ideriha, Takeshi]

>From: Ideriha, Takeshi [mailto:ideriha.take...@jp.fujitsu.com]
>[TL; DR]
>The basic idea is following 4 points:
>A. User can choose which database to put a cache (relation and catalog) on 
>shared
>memory and how much memory is used 
>B. Caches of committed data are on the
>shared memory. Caches of uncommitted data are on the local memory.
>C. Caches on the shared memory have xid information (xmin, xmax) 
>D. Evict not recently used cache from shared memory

I updated some thoughts about B and C for CatCache.
I would be very happy if you put some comments.

>[B & C]
>Regarding B & C, the motivation is we don't want other backends to see 
>uncommitted
>tables.
>Search order is local memory -> shared memory -> disk.
>Local process searches cache in shared memory based from its own snapshot and 
>xid
>of cache.
>When cache is not found in shared memory, cache with xmin is made in shared
>memory ( but not in local one).
>
>When cache definition is changed by DDL, new cache is created in local one, 
>and thus
>next commands refer to local cache if needed.
>When it's committed, local cache is cleared and shared cache is updated. This 
>update
>is done by adding xmax to old cache and also make a new one with xmin. The idea
>behind adding a new one is that newly created cache (new table or altered 
>table) is
>likely to be used in next transactions. At this point maybe we can make use of 
>current
>invalidation mechanism, even though invalidation message to other backends is 
>not
>sent.

My current thoughts:
- Each catcache has (maybe partial) HeapTupleHeader
- put every catcache on shared memory and no local catcache
- but catcache for aborted tuple is not put on shared memory
- Hash table exists per kind of CatCache
- These hash tables exists for each database and shared
  - e.g) there is a hash table for pg_class of a DB

Why I'm leaning toward not to use local cache follows:
- At commit moment you need to copy local cache to global cache. This would 
delay
  the response time.
- Even if uncommitted catcache is on shared memory, other transaction cannot 
  see the cache. In my idea they have xid information and visibility is checked 
  by comparing xmin, xmax of catcache and snapshot.  

OK, then if we put catcache on shared memory, we need to check their visibility.
But if we use the exact same visibility check mechanism as heap tuple,
it takes much more steps compared to current local catcache search.
Current visibility check is based on snapshot check and commit/abort check.
So I'm thinking to only put in-progress caches or committed one. This would
save time for checking catcache status (commit/abort) while searching cache.
But basically I'm going to use current visibility check mechanism except commit/
abort check (in other words check of clog).

These are how it works.
- When creating a catcache, copy heap tuple with heapTupleHeader 
- When update/delete command for catalog tuple is finished, 
  update xmax of corresponding cache 
- If there is a cache whose xmin is aborted xid, delete the cache
- If there is a cache whose xmax is aborted xid, initialize xmax information
- At commit time, there is no action to the shared cache

Pending items are
- thoughts about shared relcache
- "vacuum" process for shared cache

Regards,
Ideriha Takeshi

Re: SQL statement PREPARE does not work in ECPG

[Michael Meskes]

> This patch seems to have little incidence on the stability, but FWIW
> I
> am not cool with the concept of asking for objections and commit a
> patch only 4 hours after-the-fact, particularly after feature freeze.

This was only done to beat the pg_indent run as Tom pointed out. I
figured worse case we can revert the patch if people object.

Michael
-- 
Michael Meskes
Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org)
Meskes at (Debian|Postgresql) dot Org
Jabber: michael at xmpp dot meskes dot org
VfL Borussia! Força Barça! SF 49ers! Use Debian GNU/Linux, PostgreSQL


signature.asc
Description: This is a digitally signed message part

Re: Minimal logical decoding on standbys

[Amit Khandekar]

On Tue, 9 Apr 2019 at 22:23, Amit Khandekar  wrote:
>
> On Sat, 6 Apr 2019 at 04:45, Andres Freund  wrote:
> > > diff --git a/src/backend/replication/slot.c 
> > > b/src/backend/replication/slot.c
> > > index 006446b..5785d2f 100644
> > > --- a/src/backend/replication/slot.c
> > > +++ b/src/backend/replication/slot.c
> > > @@ -1064,6 +1064,85 @@ ReplicationSlotReserveWal(void)
> > >   }
> > >  }
> > >
> > > +void
> > > +ResolveRecoveryConflictWithSlots(Oid dboid, TransactionId xid)
> > > +{
> > > + int i;
> > > + boolfound_conflict = false;
> > > +
> > > + if (max_replication_slots <= 0)
> > > + return;
> > > +
> > > +restart:
> > > + if (found_conflict)
> > > + {
> > > + CHECK_FOR_INTERRUPTS();
> > > + /*
> > > +  * Wait awhile for them to die so that we avoid flooding an
> > > +  * unresponsive backend when system is heavily loaded.
> > > +  */
> > > + pg_usleep(10);
> > > + found_conflict = false;
> > > + }
> > > +
> > > + LWLockAcquire(ReplicationSlotControlLock, LW_SHARED);
> > > + for (i = 0; i < max_replication_slots; i++)
> > > + {
> > > + ReplicationSlot *s;
> > > + NameDataslotname;
> > > + TransactionId slot_xmin;
> > > + TransactionId slot_catalog_xmin;
> > > +
> > > + s = >replication_slots[i];
> > > +
> > > + /* cannot change while ReplicationSlotCtlLock is held */
> > > + if (!s->in_use)
> > > + continue;
> > > +
> > > + /* not our database, skip */
> > > + if (s->data.database != InvalidOid && s->data.database != 
> > > dboid)
> > > + continue;
> > > +
> > > + SpinLockAcquire(>mutex);
> > > + slotname = s->data.name;
> > > + slot_xmin = s->data.xmin;
> > > + slot_catalog_xmin = s->data.catalog_xmin;
> > > + SpinLockRelease(>mutex);
> > > +
> > > + if (TransactionIdIsValid(slot_xmin) && 
> > > TransactionIdPrecedesOrEquals(slot_xmin, xid))
> > > + {
> > > + found_conflict = true;
> > > +
> > > + ereport(WARNING,
> > > + (errmsg("slot %s w/ xmin %u 
> > > conflicts with removed xid %u",
> > > + NameStr(slotname), 
> > > slot_xmin, xid)));
> > > + }
> > > +
> > > + if (TransactionIdIsValid(slot_catalog_xmin) && 
> > > TransactionIdPrecedesOrEquals(slot_catalog_xmin, xid))
> > > + {
> > > + found_conflict = true;
> > > +
> > > + ereport(WARNING,
> > > + (errmsg("slot %s w/ catalog xmin %u 
> > > conflicts with removed xid %u",
> > > + NameStr(slotname), 
> > > slot_catalog_xmin, xid)));
> > > + }
> > > +
> > > +
> > > + if (found_conflict)
> > > + {
> > > + elog(WARNING, "Dropping conflicting slot %s", 
> > > s->data.name.data);
> > > + LWLockRelease(ReplicationSlotControlLock);  /* 
> > > avoid deadlock */
> > > + ReplicationSlotDropPtr(s);
> > > +
> > > + /* We released the lock above; so re-scan the 
> > > slots. */
> > > + goto restart;
> > > + }
> > > + }
> > >
> > I think this should be refactored so that the two found_conflict cases
> > set a 'reason' variable (perhaps an enum?) to the particular reason, and
> > then only one warning should be emitted.  I also think that LOG might be
> > more appropriate than WARNING - as confusing as that is, LOG is more
> > severe than WARNING (see docs about log_min_messages).
>
> What I have in mind is :
>
> ereport(LOG,
> (errcode(ERRCODE_INTERNAL_ERROR),
> errmsg("Dropping conflicting slot %s", s->data.name.data),
> errdetail("%s, removed xid %d.", conflict_str, xid)));
> where conflict_str is a dynamically generated string containing
> something like : "slot xmin : 1234, slot catalog_xmin: 5678"
> So for the user, the errdetail will look like :
> "slot xmin: 1234, catalog_xmin: 5678, removed xid : 9012"
> I think the user can figure out whether it was xmin or catalog_xmin or
> both that conflicted with removed xid.
> If we don't do this way, we may not be able to show in a single
> message if both xmin and catalog_xmin are conflicting at the same
> time.
>
> Does this message look good to you, or you had in mind something quite
> different ?

The above one is yet another point that needs to be concluded on. Till
then I will use the above way to display the error message in the
upcoming patch version.

-- 
Thanks,
-Amit Khandekar
EnterpriseDB Corporation
The Postgres Database Company

Re: Minimal logical decoding on standbys

[Amit Khandekar]

Hi,

I am going through you comments. Meanwhile, attached is a rebased
version of the v4 patch.

On Tue, 21 May 2019 at 21:49, Andres Freund  wrote:
>
> Hi,
>
> Sorry for the late response.
>
> On 2019-04-16 12:27:46 +0530, Amit Khandekar wrote:
> > On Sat, 13 Apr 2019 at 00:57, Andres Freund  wrote:
> > > > Not sure why this is happening. On slave, wal_level is logical, so
> > > > logical records should have tuple data. Not sure what does that have
> > > > to do with wal_level of master. Everything should be there on slave
> > > > after it replays the inserts; and also slave wal_level is logical.
> > >
> > > The standby doesn't write its own WAL, only primaries do. I thought we
> > > forbade running with wal_level=logical on a standby, when the primary is
> > > only set to replica.  But that's not what we do, see
> > > CheckRequiredParameterValues().
> > >
> > > I've not yet thought this through, but I think we'll have to somehow
> > > error out in this case.  I guess we could just check at the start of
> > > decoding what ControlFile->wal_level is set to,
> >
> > By "start of decoding", I didn't get where exactly. Do you mean
> > CheckLogicalDecodingRequirements() ?
>
> Right.
>
>
> > > and then raise an error
> > > in decode.c when we pass an XLOG_PARAMETER_CHANGE record that sets
> > > wal_level to something lower?
> >
> > Didn't get where exactly we should error out. We don't do
> > XLOG_PARAMETER_CHANGE handling in decode.c , so obviously you meant
> > something else, which I didn't understand.
>
> I was indeed thinking of checking XLOG_PARAMETER_CHANGE in
> decode.c. Adding handling for that, and just checking wal_level, ought
> to be fairly doable? But, see below:
>
>
> > What I am thinking is :
> > In CheckLogicalDecodingRequirements(), besides checking wal_level,
> > also check ControlFile->wal_level when InHotStandby. I mean, when we
> > are InHotStandby, both wal_level and ControlFile->wal_level should be
> > >= WAL_LEVEL_LOGICAL. This will allow us to error out when using logical
> > slot when master has incompatible wal_level.
>
> That still allows the primary to change wal_level after logical decoding
> has started, so we need the additional checks.
>
> I'm not yet sure how to best deal with the fact that wal_level might be
> changed by the primary at basically all times. We would eventually get
> an error when logical decoding reaches the XLOG_PARAMETER_CHANGE. But
> that's not necessarily sufficient - if a primary changes its wal_level
> to lower, it could remove information logical decoding needs *before*
> logical decoding reaches the XLOG_PARAMETER_CHANGE record.
>
> So I suspect we need conflict handling in xlog_redo's
> XLOG_PARAMETER_CHANGE case. If we there check against existing logical
> slots, we ought to be safe.
>
> Therefore I think the check in CheckLogicalDecodingRequirements() needs
> to be something like:
>
> if (RecoveryInProgress())
> {
> if (!InHotStandby)
> ereport(ERROR, "logical decoding on a standby required hot_standby to 
> be enabled");
> /*
>  * This check is racy, but whenever XLOG_PARAMETER_CHANGE indicates that
>  * wal_level has changed, we verify that there are no existin glogical
>  * replication slots. And to avoid races around creating a new slot,
>  * CheckLogicalDecodingRequirements() is called once before creating the 
> slot,
>  * andd once when logical decoding is initially starting up.
>  */
> if (ControlFile->wal_level != LOGICAL)
> ereport(ERROR, "...");
> }
>
> And then add a second CheckLogicalDecodingRequirements() call into
> CreateInitDecodingContext().
>
> What do you think?
>
> Greetings,
>
> Andres Freund



-- 
Thanks,
-Amit Khandekar
EnterpriseDB Corporation
The Postgres Database Company


logical-decoding-on-standby_v4_rebased.patch
Description: Binary data

Re: Teach pg_upgrade test to honor NO_TEMP_INSTALL

[Thomas Munro]

On Wed, May 22, 2019 at 7:41 AM Andres Freund  wrote:>
> On 2019-05-21 12:19:18 -0700, Andres Freund wrote:
> > Roughly like in the attached?
>
> > -check: test.sh all
> > - MAKE=$(MAKE) bindir="$(tbindir)" libdir="$(tlibdir)" 
> > EXTRA_REGRESS_OPTS="$(EXTRA_REGRESS_OPTS)" $(SHELL) $< $(DOINST)
> > +check: test.sh all temp-install
> > + MAKE=$(MAKE) $(with_temp_install) 
> > bindir=$(abs_top_builddir)/tmp_install/$(bindir) MAKE=$(MAKE) 
> > EXTRA_REGRESS_OPTS="$(EXTRA_REGRESS_OPTS)" $(SHELL) $< $(DOINST)
>
> minus the duplicated MAKE=$(MAKE) of course.

After these commits (and Tom's commit "Un-break pg_upgrade regression
test."), cfbot broke:

(using postmaster on /tmp/pg_upgrade_check-YGuskp, port 54464)
== dropping database "regression" ==
sh: 1: /usr/local/pgsql/bin/psql: not found
command failed: "/usr/local/pgsql/bin/psql" -X -c "DROP DATABASE IF
EXISTS \"regression\"" "postgres"
make[1]: *** [installcheck-parallel] Error 2
make[1]: Leaving directory
`/home/travis/build/postgresql-cfbot/postgresql/src/test/regress'

Before that it had been running happily like this:

./configure --enable-debug --enable-cassert --enable-tap-tests
--with-tcl --with-python --with-perl --with-ldap --with-openssl
--with-gssapi --with-icu && echo "COPT=-Wall -Werror" >
src/Makefile.custom && make -j4 all contrib docs && make check-world

I added --prefix=$HOME/something and added "make install" before "make
check-world", and now it's happy again.  Was that expected?

-- 
Thomas Munro
https://enterprisedb.com

Re: pg_dump throwing "column number -1 is out of range 0..36" on HEAD

[Dmitry Dolgov]

> On Wed, May 22, 2019 at 10:34 AM Michael Paquier  wrote:
>
> Trying to do pg_dump[all] on a 9.5 or older server results in spurious
> failures:
> pg_dump: column number -1 is out of range 0..36
>
> After looking around, the problem comes from
> check_tuple_field_number(), more specifically from getTables() where
> someone has forgotten to add NULL values for amname when querying
> older server versions.

Yeah, sorry, looks like it was my fault.

> Attached is a patch to fix that.  I am not seeing other failures with
> an instance that includes all the contents of installcheck, so it
> seems that the rest is fine.
>
> This needs to be applied to HEAD, so I am adding an open item.
>
> Any objections to the attached?

I've checked it too (on 9.4), don't see any issues after applying this patch,
so +1.

pg_dump throwing "column number -1 is out of range 0..36" on HEAD

[Michael Paquier]

Hi all,

Trying to do pg_dump[all] on a 9.5 or older server results in spurious
failures:
pg_dump: column number -1 is out of range 0..36

After looking around, the problem comes from
check_tuple_field_number(), more specifically from getTables() where
someone has forgotten to add NULL values for amname when querying
older server versions.

Attached is a patch to fix that.  I am not seeing other failures with
an instance that includes all the contents of installcheck, so it
seems that the rest is fine.

This needs to be applied to HEAD, so I am adding an open item.

Any objections to the attached?
--
Michael
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index e8ce719a0a..a17f6490a4 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -6112,6 +6112,7 @@ getTables(Archive *fout, int *numTables)
 		  "tc.relminmxid AS tminmxid, "
 		  "c.relpersistence, c.relispopulated, "
 		  "c.relreplident, c.relpages, "
+		  "NULL AS amname, "
 		  "CASE WHEN c.reloftype <> 0 THEN c.reloftype::pg_catalog.regtype ELSE NULL END AS reloftype, "
 		  "d.refobjid AS owning_tab, "
 		  "d.refobjsubid AS owning_col, "
@@ -6161,6 +6162,7 @@ getTables(Archive *fout, int *numTables)
 		  "tc.relminmxid AS tminmxid, "
 		  "c.relpersistence, c.relispopulated, "
 		  "c.relreplident, c.relpages, "
+		  "NULL AS amname, "
 		  "CASE WHEN c.reloftype <> 0 THEN c.reloftype::pg_catalog.regtype ELSE NULL END AS reloftype, "
 		  "d.refobjid AS owning_tab, "
 		  "d.refobjsubid AS owning_col, "
@@ -6210,6 +6212,7 @@ getTables(Archive *fout, int *numTables)
 		  "tc.relminmxid AS tminmxid, "
 		  "c.relpersistence, c.relispopulated, "
 		  "'d' AS relreplident, c.relpages, "
+		  "NULL AS amname, "
 		  "CASE WHEN c.reloftype <> 0 THEN c.reloftype::pg_catalog.regtype ELSE NULL END AS reloftype, "
 		  "d.refobjid AS owning_tab, "
 		  "d.refobjsubid AS owning_col, "
@@ -6259,6 +6262,7 @@ getTables(Archive *fout, int *numTables)
 		  "0 AS tminmxid, "
 		  "c.relpersistence, 't' as relispopulated, "
 		  "'d' AS relreplident, c.relpages, "
+		  "NULL AS amname, "
 		  "CASE WHEN c.reloftype <> 0 THEN c.reloftype::pg_catalog.regtype ELSE NULL END AS reloftype, "
 		  "d.refobjid AS owning_tab, "
 		  "d.refobjsubid AS owning_col, "
@@ -6306,6 +6310,7 @@ getTables(Archive *fout, int *numTables)
 		  "0 AS tminmxid, "
 		  "'p' AS relpersistence, 't' as relispopulated, "
 		  "'d' AS relreplident, c.relpages, "
+		  "NULL AS amname, "
 		  "CASE WHEN c.reloftype <> 0 THEN c.reloftype::pg_catalog.regtype ELSE NULL END AS reloftype, "
 		  "d.refobjid AS owning_tab, "
 		  "d.refobjsubid AS owning_col, "
@@ -6352,6 +6357,7 @@ getTables(Archive *fout, int *numTables)
 		  "0 AS tminmxid, "
 		  "'p' AS relpersistence, 't' as relispopulated, "
 		  "'d' AS relreplident, c.relpages, "
+		  "NULL AS amname, "
 		  "NULL AS reloftype, "
 		  "d.refobjid AS owning_tab, "
 		  "d.refobjsubid AS owning_col, "
@@ -6398,6 +6404,7 @@ getTables(Archive *fout, int *numTables)
 		  "0 AS tminmxid, "
 		  "'p' AS relpersistence, 't' as relispopulated, "
 		  "'d' AS relreplident, c.relpages, "
+		  "NULL AS amname, "
 		  "NULL AS reloftype, "
 		  "d.refobjid AS owning_tab, "
 		  "d.refobjsubid AS owning_col, "
@@ -6443,6 +6450,7 @@ getTables(Archive *fout, int *numTables)
 		  "0 AS tfrozenxid, 0 AS tminmxid,"
 		  "'p' AS relpersistence, 't' as relispopulated, "
 		  "'d' AS relreplident, relpages, "
+		  "NULL AS amname, "
 		  "NULL AS reloftype, "
 		  "d.refobjid AS owning_tab, "
 		  "d.refobjsubid AS owning_col, "


signature.asc
Description: PGP signature

Re: Ought to use heap_multi_insert() for pg_attribute/depend insertions?

[Daniel Gustafsson]

> On 13 Feb 2019, at 19:27, Andres Freund  wrote:
> 
> Hi,
> 
> Turns out in portions of the regression tests a good chunk of the
> runtime is inside AddNewAttributeTuples() and
> recordMultipleDependencies()'s heap insertions. Looking at a few
> profiles I had lying around I found that in some production cases
> too. ISTM we should use heap_multi_insert() for both, as the source
> tuples ought to be around reasonably comfortably.
> 
> For recordMultipleDependencies() it'd obviously better if we collected
> all dependencies for new objects, rather than doing so separately. Right
> now e.g. the code for a new table looks like:
> 
>   recordDependencyOn(, , DEPENDENCY_NORMAL);
> 
>   recordDependencyOnOwner(RelationRelationId, relid, ownerid);
> 
>   recordDependencyOnNewAcl(RelationRelationId, relid, 0, ownerid, 
> relacl);
> 
>   recordDependencyOnCurrentExtension(, false);
> 
>   if (reloftypeid)
>   {
>   referenced.classId = TypeRelationId;
>   referenced.objectId = reloftypeid;
>   referenced.objectSubId = 0;
>   recordDependencyOn(, , 
> DEPENDENCY_NORMAL);
>   }
> 
> and it'd obviously be more efficient to do that once if we went to using
> heap_multi_insert() in the dependency code. But I suspect even if we
> just used an extended API in AddNewAttributeTuples() (for the type /
> collation dependencies), it'd be a win.

When a colleague was looking at heap_multi_insert in the COPY codepath I
remembered this and took a stab at a WIP patch inspired by this email, while
not following it to the letter.  It’s not going the full route of collecting
all the dependencies for creating a table, but adding ways to perform
multi_heap_insert in the existing codepaths as it seemed like a good place to
start.

It introduces a new function CatalogMultiInsertWithInfo which takes a set of
slots for use in heap_multi_insert, used from recordMultipleDependencies and
InsertPgAttributeTuples (which replace calling InsertPgAttributeTuple
repeatedly).  The code is still a WIP with some kludges, following the show-
early philosophy.

It passes make check and some light profiling around regress suites indicates
that it does improve a bit by reducing the somewhat costly calls.  Is this
along the lines of what you were thinking or way off?

cheers ./daniel



catalog_multi_insert.patch
Description: Binary data

Re: PG 12 draft release notes

[Ian Barwick]

On 5/22/19 4:26 PM, Michael Paquier wrote:

On Wed, May 22, 2019 at 09:19:53AM +0900, Ian Barwick wrote:

the last two items are performance improvements not related to authentication;
presumably the VACUUM item would be better off in the "Utility Commands"
section and the TRUNCATE item in "General Performance"?


I agree with removing them from authentication, but these are not
performance-related items.  Instead I think that "Utility commands" is
a place where they can live better.

I am wondering if we should insist on the DOS attacks on a server, as
non-authorized users are basically able to block any tables, and
authorization is only a part of it, one of the worst parts
actually...  Anyway, I think that "This prevents unauthorized locking
delays." does not provide enough details.  What about reusing the
first paragraph of the commits?  Here is an idea:
"A caller of TRUNCATE/VACUUM/ANALYZE could previously queue for an
access exclusive lock on a relation it may not have permission to
truncate/vacuum/analyze, potentially interfering with users authorized
to work on it.  This could prevent users from accessing some relations
they have access to, in some cases preventing authentication if a
critical catalog relation was blocked."


Ah, if that's the intent behind/use for those changes (I haven't looked at them
in any detail, was just scanning the release notes) then it certainly needs some
explanation along those lines.


Regards

Ian Barwick


--
 Ian Barwick   https://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Re: VACUUM fails to parse 0 and 1 as boolean value

[Michael Paquier]

On Wed, May 22, 2019 at 04:32:38AM +0900, Fujii Masao wrote:
> I found that tab-completion also needs to be updated for ANALYZE
> boolean options. I added that change for tab-completion into
> the patch and am thinking to apply the attached patch.

Looks fine to me at quick glance.
--
Michael


signature.asc
Description: PGP signature

Re: Should MSVC 2019 support be an open item for v12?

[Michael Paquier]

On Tue, May 21, 2019 at 11:53:51PM +0200, Juan José Santamaría Flecha wrote:
> El mar., 21 may. 2019 23:06, Tom Lane  escribió:
>> Given the number of different people that have sent in patches
>> for building with VS2019, it doesn't seem to me that we ought
>> to let that wait for v13.
> 
> I am not so sure if there are actually that many people or it's just me
> making too much noise about this single issue, if that is the case I want
> to apologize.

Well, you are the second person caring enough about that matter and
post a patch on the lists, so my take is that there is no need to wait
for v13 to open, and that we should do that now also because support
for new MSVC versions gain back-patching.  Something I think we should
have is also a new animal running VS2019 (no plans to maintain one
myself).  I can take care of this patch, I just need to set up a VM
with this version of MSVC to make sure that it works..  One thing we
need to be careful is handling of local on Windows, this stuff changes
more or less at each release of VS.
--
Michael


signature.asc
Description: PGP signature

Re: PG 12 draft release notes

[Michael Paquier]

On Wed, May 22, 2019 at 09:19:53AM +0900, Ian Barwick wrote:
> the last two items are performance improvements not related to authentication;
> presumably the VACUUM item would be better off in the "Utility Commands"
> section and the TRUNCATE item in "General Performance"?

I agree with removing them from authentication, but these are not
performance-related items.  Instead I think that "Utility commands" is
a place where they can live better.

I am wondering if we should insist on the DOS attacks on a server, as
non-authorized users are basically able to block any tables, and
authorization is only a part of it, one of the worst parts
actually...  Anyway, I think that "This prevents unauthorized locking
delays." does not provide enough details.  What about reusing the
first paragraph of the commits?  Here is an idea:
"A caller of TRUNCATE/VACUUM/ANALYZE could previously queue for an
access exclusive lock on a relation it may not have permission to
truncate/vacuum/analyze, potentially interfering with users authorized
to work on it.  This could prevent users from accessing some relations
they have access to, in some cases preventing authentication if a
critical catalog relation was blocked."
--
Michael


signature.asc
Description: PGP signature

ACL dump ordering broken as well for tablespaces

[Michael Paquier]

Hi all,

As some may have noticed, I have been looking at the ACL dump ordering
for databases, and I have noticed the same issue with tablespaces:
https://www.postgresql.org/message-id/20190522062626.gc1...@paquier.xyz

For the sake of avoiding looking at the other email, here is how to
reproduce the issue:
1) First issue those SQLs:
\! rm -rf /tmp/tbspc/
\! mkdir -p /tmp/tbspc/
CREATE ROLE a_user;
CREATE ROLE b_user WITH SUPERUSER;
CREATE ROLE c_user;
CREATE TABLESPACE poo LOCATION '/tmp/tbspc/';
SET SESSION AUTHORIZATION b_user;
REVOKE ALL ON TABLESPACE poo FROM public;
GRANT CREATE ON TABLESPACE poo TO c_user WITH GRANT OPTION;
SET SESSION AUTHORIZATION c_user;
GRANT CREATE ON TABLESPACE poo TO a_user
2) Use pg_dumpall -g, where you would notice the following set of
GRANT queries:
CREATE TABLESPACE poo OWNER postgres LOCATION '/tmp/tbspc';
SET SESSION AUTHORIZATION c_user;
GRANT ALL ON TABLESPACE poo TO a_user;
RESET SESSION AUTHORIZATION;
GRANT ALL ON TABLESPACE poo TO c_user WITH GRANT OPTION;
3) Trying to restore results in a failure for the first GRANT query,
as the second one has not set yet the authorizations for c_user.

Attached is a patch to fix that, so as pg_dumpall does not complain
when piling up GRANT commands using WITH GRANT OPTION.  Are there any
complains to apply that down to 9.6?

When applying the patch, the set of GRANT queries is reordered:
 CREATE TABLESPACE poo OWNER postgres LOCATION '/tmp/tbspc';
+GRANT ALL ON TABLESPACE poo TO c_user WITH GRANT OPTION;
 SET SESSION AUTHORIZATION c_user;
 GRANT ALL ON TABLESPACE poo TO a_user;
 RESET SESSION AUTHORIZATION;
-GRANT ALL ON TABLESPACE poo TO c_user WITH GRANT OPTION;

As the problem is kind of different than the database case, I wanted
to spawn anyway a new thread, but I got a bonus question: what would
it take to support pg_init_privs for databases and tablespaces?  If we
could get that to work, then all the ACL-related queries built for all
objects could make use of buildACLQueries(), which would avoid extra
diffs in the dump code for dbs and tbspaces.

Thoughts?
--
Michael
diff --git a/src/bin/pg_dump/pg_dumpall.c b/src/bin/pg_dump/pg_dumpall.c
index 102731ea0c..e833db4533 100644
--- a/src/bin/pg_dump/pg_dumpall.c
+++ b/src/bin/pg_dump/pg_dumpall.c
@@ -1166,19 +1166,38 @@ dumpTablespaces(PGconn *conn)
 	 *
 	 * See buildACLQueries() and buildACLCommands().
 	 *
+	 * The order in which privileges are in the ACL string (the order they
+	 * have been GRANT'd in, which the backend maintains) must be preserved to
+	 * ensure that GRANTs WITH GRANT OPTION and subsequent GRANTs based on
+	 * those are dumped in the correct order.
+	 *
 	 * Note that we do not support initial privileges (pg_init_privs) on
-	 * tablespaces.
+	 * tablespaces, so this logic cannot make use of buildACLQueries().
 	 */
 	if (server_version >= 90600)
 		res = executeQuery(conn, "SELECT oid, spcname, "
 		   "pg_catalog.pg_get_userbyid(spcowner) AS spcowner, "
 		   "pg_catalog.pg_tablespace_location(oid), "
-		   "(SELECT pg_catalog.array_agg(acl) FROM (SELECT pg_catalog.unnest(coalesce(spcacl,pg_catalog.acldefault('t',spcowner))) AS acl "
-		   "EXCEPT SELECT pg_catalog.unnest(pg_catalog.acldefault('t',spcowner))) as foo)"
-		   "AS spcacl,"
-		   "(SELECT pg_catalog.array_agg(acl) FROM (SELECT pg_catalog.unnest(pg_catalog.acldefault('t',spcowner)) AS acl "
-		   "EXCEPT SELECT pg_catalog.unnest(coalesce(spcacl,pg_catalog.acldefault('t',spcowner as foo)"
-		   "AS rspcacl,"
+		   "(SELECT array_agg(acl ORDER BY row_n) FROM "
+		   "  (SELECT acl, row_n FROM "
+		   " unnest(coalesce(spcacl,acldefault('t',spcowner))) "
+		   " WITH ORDINALITY AS perm(acl,row_n) "
+		   "   WHERE NOT EXISTS ( "
+		   " SELECT 1 "
+		   " FROM unnest(acldefault('t',spcowner)) "
+		   "   AS init(init_acl) "
+		   " WHERE acl = init_acl)) AS spcacls) "
+		   " AS spcacl, "
+		   "(SELECT array_agg(acl ORDER BY row_n) FROM "
+		   "  (SELECT acl, row_n FROM "
+		   " unnest(acldefault('t',spcowner)) "
+		   " WITH ORDINALITY AS initp(acl,row_n) "
+		   "   WHERE NOT EXISTS ( "
+		   " SELECT 1 "
+		   " FROM unnest(coalesce(spcacl,acldefault('t',spcowner))) "
+		   "   AS permp(orig_acl) "
+		   " WHERE acl = orig_acl)) AS rspcacls) "
+		   " AS rspcacl, "
 		   "array_to_string(spcoptions, ', '),"
 		   "pg_catalog.shobj_description(oid, 'pg_tablespace') "
 		   "FROM pg_catalog.pg_tablespace "


signature.asc
Description: PGP signature

Re: PostgreSQL 12 Beta 1 press release draft

[Amit Langote]

On 2019/05/22 15:47, David Rowley wrote:
> On Wed, 22 May 2019 at 15:39, Jonathan S. Katz  wrote:
>> PostgreSQL 12 also provides improvements to the performance of both
>> using `COPY` with a partitioned table as well as the `ATTACH PARTITION`
>> operation. Additionally, the ability to use foreign keys to reference
>> partitioned tables is now allowed in PostgreSQL 12.
> 
> I'd say nothing has been done to improve the performance of ATTACH
> PARTITION. Robert did reduce the lock level required for that
> operation, but that should make it any faster.

Maybe you meant "..., but that shouldn't make it any faster."

Thanks,
Amit

Re: PostgreSQL 12 Beta 1 press release draft

[David Rowley]

On Wed, 22 May 2019 at 15:39, Jonathan S. Katz  wrote:
> Speaking of feedback, please provide me with your feedback on the
> technical correctness of this announcement so I can incorporate changes
> prior to the release.

Seems like a pretty good summary. Thanks for writing that up.

Couple notes from my read through:

> help with length index rebuilds that could cause potential downtime evens when
> administration a PostgreSQL database in a production environment.

length -> lengthy?
evens -> events? (mentioned by Justin)
administration -> administering? (mentioned by Justin)

> PostgreSQL 12 also provides improvements to the performance of both
> using `COPY` with a partitioned table as well as the `ATTACH PARTITION`
> operation. Additionally, the ability to use foreign keys to reference
> partitioned tables is now allowed in PostgreSQL 12.

I'd say nothing has been done to improve the performance of ATTACH
PARTITION. Robert did reduce the lock level required for that
operation, but that should make it any faster.

I think it would be good to write:

PostgreSQL 12 also provides improvements to the performance of both
`INSERT` and `COPY` into a partitioned table.  `ATTACH PARTITION` can
now also be performed without blocking concurrent queries on the
partitioned table.  Additionally, the ability to use foreign keys to
reference partitioned tables is now allowed in PostgreSQL 12.


> ### Most-common Value Statistics

I think this might be better titled:

### Most-common Value Extended Statistics

which is slightly different from what Alexander mentioned. I think we
generally try to call them "extended statistics", even if the name of
the command does not quite agree. git grep -i "extended stat" shows
more interesting results than git grep -i "column stat" when done in
the doc directory.  Either way, I think it's slightly confusing to
title this the way it is since we already have MCV stats and have had
for a long time.

-- 
 David Rowley   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Re: SQL statement PREPARE does not work in ECPG

[Michael Paquier]

On Wed, May 22, 2019 at 05:10:14AM +0200, Michael Meskes wrote:
> Thanks for the heads-up Tom, pushed.
> 
> And thanks to Matsumura-san for the patch.

This patch seems to have little incidence on the stability, but FWIW I
am not cool with the concept of asking for objections and commit a
patch only 4 hours after-the-fact, particularly after feature freeze.
--
Michael


signature.asc
Description: PGP signature