Re: [HACKERS] CTE inlining

[Thomas Kellerer]

> 1) we switch unmarked CTEs as inlineable by default in pg11. 

+1 from me for option 1




--
View this message in context: 
http://www.postgresql-archive.org/CTE-inlining-tp5958992p5959615.html
Sent from the PostgreSQL - hackers mailing list archive at Nabble.com.


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] transition table behavior with inheritance appears broken (was: Declarative partitioning - another take)

[Thomas Munro]

On Thu, May 4, 2017 at 4:02 AM, Alvaro Herrera  wrote:
> Robert Haas wrote:
>> I suspect that most users would find it more useful to capture all of
>> the rows that the statement actually touched, regardless of whether
>> they hit the named table or an inheritance child.
>
> Yes, agreed.  For the plain inheritance cases each row would need to
> have an indicator of which relation it comes from (tableoid); I'm not
> sure if such a thing would be useful in the partitioning case.

On Thu, May 4, 2017 at 4:26 AM, David Fetter  wrote:
> +1 on the not-duct-tape view of partitioned tables.

Hmm.  Ok.  Are we talking about PG10 or PG11 here?  Does this approach
makes sense?

1.  Remove the prohibition on creating transition-capturing triggers
on a partitioned table.

2.  Make sure that the ExecAR* functions call AfterTriggerSaveEvent
when modifying partition tables if the explicitly named parent
partitioned table has after triggers with transition tables.  Not sure
how exactly how but doesn't seem difficult.

3.  Convert tuples to the TupleDesc of the relation that owns the
statement trigger (ie the partitioned table) when inserting them into
the tuplestore.  One way to do that might be to build an array of
TupleConversionMap objects that does the opposite of the conversions
done by tup_conv_maps.  While tup_conv_maps is for converting tuples
to the layout needed for a partition, tup_unconv_maps (or better name)
would be for converting the old and new tuples to the TupleDesc of the
partitioned table.  Then the appropriate TupleConversionMap could be
passed into the ExecAR* functions as a new argument 'transition_map'.
AfterTriggerSaveEvent would use 'oldtup' and 'newtup' directly for ROW
triggers, but convert using the passed in map if it needs to insert
them into the transition tuplestores.

The same thing could work for inheritance, if tupconvert.c had a new
kind of conversion that allows slicing of tuples (converting a wider
child table's tuples to the parent's subset of columns) rather the
just conversion between logically equivalent TupleDescs.

To avoid the whiff of duct tape, we'd probably also want to make ROW
triggers created on the partitioned table(s) containing partition to
fire too, with appropriate TypeConversionMap treatment.  Not sure what
exactly is involved there.

On the other hand, doing that with inheritance hierarchies would be an
incompatible behavioural change, which I guess people don't want -- am
I right?

-- 
Thomas Munro
http://www.enterprisedb.com


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] CTE inlining

[Gavin Flower]

On 04/05/17 05:33, Alvaro Herrera wrote:
> David Fetter wrote:
>
>> When we add a "temporary" GUC, we're taking on a gigantic burden.
>> Either we support it forever somehow, or we put it on a deprecation
>> schedule immediately and expect to be answering questions about it for
>> years after it's been removed.
>>
>> -1 for the GUC.
>
> Absolutely.
>
> So ISTM we have three choices:
>
> 1) we switch unmarked CTEs as inlineable by default in pg11. What seems
> likely to happen for a user that upgrades to pg11 is that 5 out of 10
> CTE-using queries are going to become faster than with pg10, and they
> are going to be happy; 4 out of five are going to see no difference, but
> they didn't have to do anything about it; and the remaining query is
> going to become slower, either indistinguishably so (in which case they
> don't care and they remain happy because of the other improvements) or
> notably so, in which case they can easily figure where to add the
> MATERIALIZED option and regain the original performance.

+1 (Mario, proxied by me)

I've been asked to pass on comments by my colleague Mario of True Group
regarding the pain of the current behaviour of CTE's being optimisation
fences (as he doesn't normally follow this email group, I'll pass on any
feedback):

"Greetings,

Jumping onto the bandwagon here. At True Group in Auckland, we use
PostgreSQL as the platform for large-scale software development, often
with extensive and complex schemas, views and queries.

We frequently encounter poor query performance due to CTEs being an
optimisation barrier. We are forced to use workarounds such as a
set-returning function (parameterised view) in place of a view, and
manually placing the needed quals (WHERE clauses) into each CTE. This
creates headaches in large systems, where writing expressive SQL is
essential, rather than employing workarounds that make code harder to
understand and reuse.

Our general assumption is that we should write SQL that describes what
we want to achieve. The planner's job is to determine how to do that
efficiently.

There is an argument that pushing quals into CTEs could reduce
performance for some queries, especially if the qual is expensive. My
answer is twofold:

a) most queries will either experience no change or benefit from qual
push-down, as usually the expensive part is the query subtree, not the
top-level qual.

b) if a small proportion of queries are negatively affected, this is
better addressed by improving the planner's cost estimation. At worst,
an explicit OPTIMIZATION BARRIER hint could be added. But I know there
is much philosophical objection in the PostgreSQL community to planner
hints. The irony is that a main argument for retaining current CTE
behaviour is that people rely on CTEs as implicit barrier hints!

As an aside, we also encounter several other instances where qual
push-down fails, including where rewording a query in a way that is
semantically identical can change whether push-down takes place. But
probably the greatest bugbear is inability to push-down a qual into 
more

than one subtree, meaning if a query has two or more subtrees, each of
which is expensive, but only one row is needed from each, determined by
one qual at the top level, one subtree will get the qual pushed into it
and run fast, while the others will do a full (expensive) table 
scan and
be filtered only afterwards. There are also gains to be had in 
improving

qual push-down in sub-selects with aggregates, and others. But I think
these issues are only due to lack of resource to implement, rather than
philosophical/political objections.

We would like to see a Postgres version in the future that does much
better planning in the areas I've mentioned, including but not limited
to the present issue of CTEs. Our organisation may be able to 
contribute

materially to this, if the political will is there, and others affected
pitch in to achieve common goals. Not being expert in Postgres
internals, I am not certain how difficult each of the problems is.

I hope we have contributed usefully to the conversation, and invite
feedback.


Sincerely,
Mario"


Cheers,
Gavin


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] delta relations in AFTER triggers

[Thomas Munro]

On Thu, May 4, 2017 at 9:12 PM, Prabhat Sahu
 wrote:
> I have been testing this for a while and observed a server crash while 
> referencing table column value in a trigger procedure for AFTER DELETE 
> trigger.
>
> -- Steps to reproduce:
> CREATE TABLE t1(c1 int);
> CREATE TABLE t2(cc1 int);
> INSERT INTO t1 VALUES (10);
> INSERT INTO t2 VALUES (10);
>
> CREATE OR REPLACE FUNCTION trig_func() RETURNS trigger AS
> $$ BEGIN
> DELETE FROM t1 WHERE c1 IN (select OLD.cc1 from my_old);
> RETURN OLD;
> END; $$ LANGUAGE PLPGSQL;
>
> CREATE TRIGGER trg1
>   AFTER DELETE ON t2
> REFERENCING OLD TABLE AS my_old
> FOR EACH ROW
>   EXECUTE PROCEDURE trig_func();
>
> DELETE FROM t2 WHERE cc1 =10;
> server closed the connection unexpectedly
> This probably means the server terminated abnormally
> before or while processing the request.
> The connection to the server was lost. Attempting reset: Failed.

Reproduced here.  The stack looks like this:

frame #3: 0x00010f06f8b0
postgres`ExceptionalCondition(conditionName="!(readptr->eflags &
0x0002)", errorType="FailedAssertion", fileName="tuplestore.c",
lineNumber=1237) + 128 at assert.c:54
frame #4: 0x00010f0cbc85
postgres`tuplestore_rescan(state=0x7ff219840200) + 85 at
tuplestore.c:1237
frame #5: 0x00010eced9b1
postgres`ExecReScanNamedTuplestoreScan(node=0x7ff21d007840) + 81
at nodeNamedtuplestorescan.c:197
frame #6: 0x00010eca46a6
postgres`ExecReScan(node=0x7ff21d007840) + 822 at execAmi.c:216
frame #7: 0x00010ece7eca
postgres`ExecNestLoop(node=0x7ff21d006310) + 538 at
nodeNestloop.c:148

I think the problem is that the tuplestore read pointer hasn't been
opened with the "rewindable" flag.  It works for me with the attached.

-- 
Thomas Munro
http://www.enterprisedb.com


fix-named-tuplestore-rescan.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] CTE inlining

[Gavin Flower]

On 30/04/17 16:28, Tom Lane wrote:

Craig Ringer  writes:

- as you noted, it is hard to decide when it's worth inlining vs
materializing for CTE terms referenced more than once.

[ raised eyebrow... ]  Please explain why the answer isn't trivially
"never".

There's already a pretty large hill to climb here in the way of
breaking peoples' expectations about CTEs being optimization
fences.  Breaking the documented semantics about CTEs being
single-evaluation seems to me to be an absolute non-starter.

regards, tom lane


Could not each CTE be only evaluated once, but restricted (as far as is 
practicable) to the rows actually needed by the body of the SELECT?



Cheers,
Gavin



--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] statement_timeout is not working as expected with postgres_fdw

[tushar]

On 05/04/2017 08:01 AM, Robert Haas wrote:

Patch attached.


I tried at my end after applying the patch against PG HEAD,

Case 1 - without setting statement_timeout i.e default

X machine -
create table test1(a int);

Y machine -
CREATE SERVER myserver_ppas FOREIGN DATA WRAPPER postgres_fdw OPTIONS 
(host 'X', dbname 'postgres', port '5432', connect_timeout '3');
CREATE USER MAPPING FOR centos SERVER myserver_ppas OPTIONS (user 
'centos', password 'adminedb');
create foreign table ft_test_ppas (a  int ) server myserver_ppas options 
(table_name 'test1');

 statement_timeout =0;
\timing
insert  into ft_test_ppas  values  (generate_series(1,1000));

X machine-
disconnect network

Y machine -
postgres=# insert  into ft_test_ppas  values (generate_series(1,1000));
^CCancel request sent
^CCancel request sent
^CCancel request sent
^CCancel request sent
^CCancel request sent
^CCancel request sent
^CCancel request sent
^CCancel request sent
^CCancel request sent
^CCancel request sent
WARNING:  could not send cancel request: PQcancel() -- connect() failed: 
Connection timed out


ERROR:  canceling statement due to user request
Time: 81073.872 ms (01:21.074)

Case 2- when statement_timeout=6000

Y machine -
CREATE SERVER myserver FOREIGN DATA WRAPPER postgres_fdw OPTIONS (host 
'X', dbname 'postgres', port '5432',keepalives '1', keepalives_interval 
'3',keepalives_idle '3', keepalives_count '1');
CREATE USER MAPPING FOR centos SERVER myserver OPTIONS (user 'centos', 
password 'adminedb');
create foreign table ft_test_ppas1 (a  int ) server myserver options 
(table_name 'test1');

set statement_timeout=6000;
\timing
insert  into ft_test_ppas1  values  (generate_series(1,1000));

X machine-
disconnect network

Y machine
postgres=# insert  into ft_test_ppas1  values 
(generate_series(1,1000));
WARNING:  could not send cancel request: PQcancel() -- connect() failed: 
Connection timed out


ERROR:  canceling statement due to statement timeout
Time: 69009.875 ms (01:09.010)
postgres=#

Case 3-when statement_timeout=2

Y machine -
CREATE SERVER myserver FOREIGN DATA WRAPPER postgres_fdw OPTIONS (host 
'X', dbname 'postgres', port '5432',keepalives '1', keepalives_interval 
'3',keepalives_idle '3', keepalives_count '1');
CREATE USER MAPPING FOR centos SERVER myserver OPTIONS (user 'centos', 
password 'adminedb');
create foreign table ft_test_ppas1 (a  int ) server myserver options 
(table_name 'test1');

set statement_timeout=2;
\timing
insert  into ft_test_ppas1  values  (generate_series(1,1000));

X machine-
disconnect network

Y machine -
postgres=# insert  into ft_test_ppas1  values 
(generate_series(1,1000));
WARNING:  could not send cancel request: PQcancel() -- connect() failed: 
Connection timed out

ERROR:  canceling statement due to statement timeout
Time: 83014.503 ms (01:23.015)

We can see statement_timeout is working but it is taking some extra 
time,not sure this is an expected behavior in above case or not.


--
regards,tushar
EnterpriseDB  https://www.enterprisedb.com/
The Enterprise PostgreSQL Company



--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Adding support for Default partition in partitioning

[amul sul]

On Tue, May 2, 2017 at 9:33 PM, Rahila Syed  wrote:
> Please find attached updated patch with review comments by Robert and Jeevan
> implemented.
>
Patch v8 got clean apply on latest head but server got crash at data
insert in the following test:

-- Create test table
CREATE TABLE test ( a int, b date) PARTITION BY LIST (a);
CREATE TABLE p1 PARTITION OF test FOR VALUES IN  (DEFAULT) PARTITION BY LIST(b);
CREATE TABLE p11 PARTITION OF p1 FOR VALUES IN (DEFAULT);

-- crash
INSERT INTO test VALUES (210,'1/1/2002');

Regards,
Amul


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] statement_timeout is not working as expected with postgres_fdw

[tushar]

On 05/04/2017 03:53 PM, tushar wrote:
We can see statement_timeout is working but it is taking some extra 
time,not sure this is an expected behavior in above case or not. 
This is only when remote server is involved . in case when both the 
servers are on the same machine , then this is working as expected.


d1=# CREATE SERVER myserver_ppas FOREIGN DATA WRAPPER postgres_fdw 
OPTIONS (host 'localhost', dbname 'postgres', port '5432', 
connect_timeout '3');

CREATE SERVER
d1=# CREATE USER MAPPING FOR centos SERVER myserver_ppas OPTIONS (user 
'centos', password 'adminedb');

CREATE USER MAPPING
d1=# create foreign table ft_test_ppas (a  int ) server myserver_ppas 
options (table_name 'test1');

CREATE FOREIGN TABLE
d1=#
d1=# insert into ft_test_ppas values (1);
INSERT 0 1
Case 1-
d1=# \timing
Timing is on.
d1=# set statement_timeout =6000;
SET
Time: 0.360 ms
d1=# insert  into ft_test_ppas  values (generate_series(1,1000));
ERROR:  canceling statement due to statement timeout
Time: 6002.509 ms (00:06.003)
d1=#
Case 2 -
d1=# set statement_timeout =2;
SET
Time: 0.693 ms
d1=# insert  into ft_test_ppas  values (generate_series(1,1000));
ERROR:  canceling statement due to statement timeout
Time: 20001.741 ms (00:20.002)
d1=#

--
regards,tushar
EnterpriseDB  https://www.enterprisedb.com/
The Enterprise PostgreSQL Company



--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] statement_timeout is not working as expected with postgres_fdw

[Robert Haas]

On Thu, May 4, 2017 at 6:23 AM, tushar  wrote:
> We can see statement_timeout is working but it is taking some extra time,not
> sure this is an expected behavior in above case or not.

Yeah, that's expected.  To fix that, we'd need libpq to have an async
equivalent of PQcancel(), which doesn't currently exist.  I think it
would be a good idea to add that, but that's another discussion.  With
this patch:

1. If postgres_fdw is waiting for one of the cleanup queries to
execute, you can cancel it, and things like statement_timeout will
also work.

2. If the cancel fails or any of the cleanup queries fail,
postgres_fdw will forcibly close the connection and force the current
transaction and all subtransactions to abort.  This isn't wonderful
behavior, but if we can't talk to the remote server any more there
doesn't seem to be any other real alternative.  (We could improve
this, I think, by tracking whether the connection had ever been use by
an outer transaction level, and if not, allowing the transaction to
commit if it never again tried to access the failed connection, but I
left the design and installation of such a mechanism to a future
patch.)

3. But if you're stuck trying to send the cancel request itself, you
still have to wait for that to fail before anything else happens.
Once it does, we'll proceed as outlined in point #2.  This stinks, but
it's the inevitable consequence of having no async equivalent of
PQcancel().

My goal is to make things noticeably better with this patch, not to
fix them completely.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] statement_timeout is not working as expected with postgres_fdw

[Amit Kapila]

On Thu, May 4, 2017 at 1:19 AM, Robert Haas  wrote:
> On Thu, Apr 20, 2017 at 10:27 AM, Ashutosh Bapat
>  wrote:
>> The logs above show that 34 seconds elapsed between starting to abort
>> the transaction and knowing that the foreign server is unreachable. It
>> looks like it took that much time for the local server to realise that
>> the foreign server is not reachable. Looking at PQcancel code, it
>> seems to be trying to connect to the foreign server to cancel the
>> query. But somehow it doesn't seem to honor connect_timeout setting.
>> Is that expected?
>
> Well, there's no code to do anything else.  Regular connections go
> through connectDBComplete() which uses non-blocking mode and timed
> waits to respect connection_timeout.  PQcancel() has no such handling.
> internal_cancel just opens a socket and, without setting any options
> on it, calls connect().  No loop, no timed waits, nada.  So it's going
> to take as long as it takes for the operating system to notice.
>
>> Irrespective of what PQcancel does, it looks like postgres_fdw should
>> just slam the connection if query is being aborted because of
>> statement_timeout. But then pgfdw_xact_callback() doesn't seem to have
>> a way to know whether this ABORT is because of user's request to
>> cancel the query, statement timeout, an abort because of some other
>> error or a user requested abort. Except statement timeout (may be
>> user's request to cancel the query?), it should try to keep the
>> connection around to avoid any future reconnection. But I am not able
>> to see how can we provide that information to pgfdw_xact_callback().
>
> I don't think we can.  In general, PostgreSQL has no facility for
> telling error cleanup handlers why the abort happened, and it wouldn't
> really be the right thing anyway.  The fact that statement_timeout
> fired doesn't necessarily mean that the connection is dead; it could
> equally well mean that the query ran for a long time.  I think we
> basically have two choices.  One is to bound the amount of time we
> spend performing error cleanup, and the other is just to always drop
> the connection.  A problem with the latter is that it might do the
> wrong thing if we're aborting a subtransaction but not the whole
> transaction.  In that case, we need to undo changes since the relevant
> savepoint, but not the ones made before that; closing the connection
> amounts to a full rollback.
>
> Therefore, I think the patch you proposed in
> https://www.postgresql.org/message-id/CAFjFpRdcWw4h0a-zrL-EiaekkPj8O0GR2M1FwZ1useSRfRm3-g%40mail.gmail.com
> isn't correct, because if the cancel fails it will slam the connection
> shut regardless of whether we're in pgfdw_xact_callback or
> pgfdw_subxact_callback.
>
> It looks to me like there's a fairly lengthy list of conceptually
> separate but practically related problems here:
>
> 1. PQcancel() ignores the keepalive parameters, because it makes no
> attempt to set the relevant socket options before connecting.
>
> 2. PQcancel() ignores connection_timeout, because it doesn't use
> non-blocking mode and has no wait loop.
>
> 3. There is no asynchronous equivalent of PQcancel(), so we can't use
> a loop to allow responding to interrupts while the cancel is
> outstanding (see pgfdw_get_result for an example).
>
> 4. pgfdw_xact_callback() and pgfdw_subxact_callback() use PQexec()
> rather than the asynchronous interfaces for sending queries and
> checking for results, so the SQL commands they send are not
> interruptible.
>
> 5. pgfdw_xact_callback() and pgfdw_subxact_callback() try to get the
> connection back to a good state by using ABORT TRANSACTION for the
> former and ROLLBACK TO SAVEPOINT and RELEASE SAVEPOINT for the latter.
> But if it doesn't work, then they just emit a WARNING and continue on
> as if they'd succeeded.  That seems highly likely to make the next use
> of that connection fail in unpredictable ways.
>

In pgfdw_xact_callback, if the execution of ABORT TRANSACTION fails
due to any reason then I think it will close the connection.  The
relavant code is:
if (PQstatus(entry->conn) != CONNECTION_OK ||
PQtransactionStatus(entry->conn) != PQTRANS_IDLE)

Basically, if abort transaction fails then transaction status won't be
PQTRANS_IDLE.  Also, does it matter if pgfdw_subxact_callback fails
during execution of "ROLLBACK TO SAVEPOINT", because anyway user needs
to execute rollback to savepoint command before proceeding and that
should be good enough?

About patch:

+ /* Rollback all remote subtransactions during abort */
+ snprintf(sql, sizeof(sql),
+ "ROLLBACK TO SAVEPOINT s%d; RELEASE SAVEPOINT s%d",
+ curlevel, curlevel);
+ if (!pgfdw_exec_cleanup_query(entry->conn, sql))
+ abort_cleanup_failure = true;

If the above execution fails due to any reason, then it won't allow
even committing the mail transaction which I am not sure is the right
thing to do.  I have tried it manually editing the above command in
debugger and the result is as below:

Create a foreign 

Re: [HACKERS] password_encryption, default and 'plain' support

[Heikki Linnakangas]

On 05/03/2017 08:40 PM, Tom Lane wrote:

The other question I can think to ask is what will happen during
pg_upgrade, given an existing installation with one or more passwords
stored plain.  If the answer is "silently convert to MD5", I'd be
good with that.


Yes, it will silently convert to MD5. That happened even on earlier 
versions, if you had password_encryption=on in the new cluster (which 
was the default).


I'm planning to go ahead with the attached patch for this (removing 
password_encryption='plain' support, but keeping the default as 'md5').


- Heikki

>From 62d42e07fe09dcb8586620ee66b1567a8f002f27 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas 
Date: Thu, 4 May 2017 14:31:45 +0300
Subject: [PATCH 1/1] Remove support for password_encryption='off' / 'plain'.

Storing passwords in plaintext hasn't been a good idea for a very long
time, if ever. Now seems like a good time to finally forbid it, since we're
messing with this in PostgreSQL 10 anyway.

This also removes the CREATE/ALTER USER UNENCRYPTED PASSSWORD 'foo' syntax,
since storing passwords unencrypted is no longer supported. ENCRYPTED
PASSWORD 'foo' is still accepted, but ENCRYPTED is now just a noise-word,
it does the same as just PASSWORD 'foo'.

Likewise, remove the --unencrypted option from createuser, but accept
--encrypted as a no-op for backwards-compatibility. AFAICS, --encrypted was
a no-op even before this patch, because createuser encrypted the password
before sending it to the server even if --encrypted was not specified. It
added the ENCRYPTED keyword to the SQL command, but since the password was
already in encrypted form, it didn't matter. The documentation was not
clear on whether that was intended or not, but the point is moot now.

Also, while password_encryption='on' is still accepted as an alias for
'md5', it is now marked as hidden, so that it is not listed as an accepted
value in error hints, for example. That's not directly related to removing
'plain', but it seems better this way.

Discussion: https://www.postgresql.org/message-id/16e9b768-fd78-0b12-cfc1-7b6b7f238...@iki.fi
---
 doc/src/sgml/config.sgml  |  18 +++--
 doc/src/sgml/ref/alter_role.sgml  |   6 +-
 doc/src/sgml/ref/alter_user.sgml  |   2 +-
 doc/src/sgml/ref/create_group.sgml|   2 +-
 doc/src/sgml/ref/create_role.sgml |  34 +++-
 doc/src/sgml/ref/create_user.sgml |   2 +-
 doc/src/sgml/ref/createuser.sgml  |  21 +
 src/backend/commands/user.c   |  34 ++--
 src/backend/libpq/auth-scram.c|  20 +
 src/backend/libpq/auth.c  |  26 ++
 src/backend/libpq/crypt.c | 126 +-
 src/backend/parser/gram.y |  14 +++-
 src/backend/utils/misc/guc.c  |  10 +--
 src/bin/psql/tab-complete.c   |  25 ++
 src/bin/scripts/createuser.c  |  46 ---
 src/include/libpq/crypt.h |   9 ++-
 src/interfaces/libpq/fe-auth.c|   3 +-
 src/test/authentication/t/001_password.pl |  10 +--
 src/test/regress/expected/password.out|  31 
 src/test/regress/sql/password.sql |  23 +++---
 20 files changed, 154 insertions(+), 308 deletions(-)

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 0b9e3002fb..20bc3c61b1 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -1188,14 +1188,16 @@ include_dir 'conf.d'
   

 When a password is specified in  or
- without writing either ENCRYPTED
-or UNENCRYPTED, this parameter determines whether the
-password is to be encrypted. The default value is md5, which
-stores the password as an MD5 hash. Setting this to plain stores
-it in plaintext. on and off are also accepted, as
-aliases for md5 and plain, respectively.  Setting
-this parameter to scram-sha-256 will encrypt the password
-with SCRAM-SHA-256.
+, this parameter determines the algorithm
+to use to encrypt the password. The default value is md5,
+which stores the password as an MD5 hash (on is also
+accepted, as alias for md5). Setting this parameter to
+scram-sha-256 will encrypt the password with SCRAM-SHA-256.
+   
+   
+Note that older clients might lack support for the SCRAM authentication
+mechanism, and hence not work with passwords encrypted with
+SCRAM-SHA-256.

   
  
diff --git a/doc/src/sgml/ref/alter_role.sgml b/doc/src/sgml/ref/alter_role.sgml
index 37fcfb926c..8cd8602bc4 100644
--- a/doc/src/sgml/ref/alter_role.sgml
+++ b/doc/src/sgml/ref/alter_role.sgml
@@ -33,7 +33,7 @@ ALTER ROLE role_specification [ WIT
 | REPLICATION | NOREPLICATION
 | BYPASSRLS | NOBYPASSRLS
 | CONNECTION LIMIT connlimit
-| [ ENCRYPTED | UNENCRYPTED ] PASSWORD 'password'
+| [ ENCRYPTED ] PASSWORD 'password'

Re: [HACKERS] Adding support for Default partition in partitioning

[Rahila Syed]

Hello Amul,

Thanks for reporting. Please find attached an updated patch which fixes the
above.
Also, the attached patch includes changes in syntax proposed upthread.

The syntax implemented in this patch is as follows,

CREATE TABLE p11 PARTITION OF p1 DEFAULT;

Thank you,
Rahila Syed

On Thu, May 4, 2017 at 4:02 PM, amul sul  wrote:

> On Tue, May 2, 2017 at 9:33 PM, Rahila Syed 
> wrote:
> > Please find attached updated patch with review comments by Robert and
> Jeevan
> > implemented.
> >
> Patch v8 got clean apply on latest head but server got crash at data
> insert in the following test:
>
> -- Create test table
> CREATE TABLE test ( a int, b date) PARTITION BY LIST (a);
> CREATE TABLE p1 PARTITION OF test FOR VALUES IN  (DEFAULT) PARTITION BY
> LIST(b);
> CREATE TABLE p11 PARTITION OF p1 FOR VALUES IN (DEFAULT);
>
> -- crash
> INSERT INTO test VALUES (210,'1/1/2002');
>
> Regards,
> Amul
>


default_partition_v9.patch
Description: application/download

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] CTE inlining

[Julien Rouhaud]

On 04/05/2017 08:34, Petr Jelinek wrote:
> On 03/05/17 23:24, Merlin Moncure wrote:
>> On Wed, May 3, 2017 at 12:33 PM, Alvaro Herrera
>>  wrote:
>>> David Fetter wrote:
>>>
 When we add a "temporary" GUC, we're taking on a gigantic burden.
 Either we support it forever somehow, or we put it on a deprecation
 schedule immediately and expect to be answering questions about it for
 years after it's been removed.

 -1 for the GUC.
>>>
>>> Absolutely.
>>>
>>> So ISTM we have three choices:
>>>
>>> 1) we switch unmarked CTEs as inlineable by default in pg11.  What seems
>>> likely to happen for a user that upgrades to pg11 is that 5 out of 10
>>> CTE-using queries are going to become faster than with pg10, and they
>>> are going to be happy; 4 out of five are going to see no difference, but
>>> they didn't have to do anything about it; and the remaining query is
>>> going to become slower, either indistinguishably so (in which case they
>>> don't care and they remain happy because of the other improvements) or
>>> notably so, in which case they can easily figure where to add the
>>> MATERIALIZED option and regain the original performance.
>>
>> +1 for option 1.  This change will be welcome for a large number of
>> queries, but forced materialization is a real need and I use it often.
>> This comes off as a very reasonable compromise in my opinion unless it
>> requires major coding gymnastics to implement.
>>
> 
> +1 to this
> 

+1 too

-- 
Julien Rouhaud
http://dalibo.com - http://dalibo.org


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Error message on missing SCRAM authentication with older clients

[Heikki Linnakangas]

On 05/03/2017 03:12 PM, Aleksander Alekseev wrote:

Hi Heikki,


psql: SCRAM authentication not supported by this version of libpq


Maybe it would be better to specify a minimum required version?


Yeah, that could be helpful. Can you suggest a wording?

My first thought was:

psql: SCRAM authentication not supported by this version of libpq 
(version 10 or above required)


But that's very long. Perhaps:

psql: SCRAM authentication requires libpq version 10 or above

- Heikki


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] Function to move the position of a replication slot

[Magnus Hagander]

PFA a patch that adds a new function, pg_move_replication_slot, that makes
it possible to move the location of a replication slot without actually
consuming all the WAL on it.

This can be useful for example to keep replication slots in sync between
different servers in a replication cluster.

(Obviously this is intended for 11, as we're well into the freeze for 10.
Just to be clear. so I'll go add itto the summer commitfest)

-- 
 Magnus Hagander
 Me: https://www.hagander.net/ 
 Work: https://www.redpill-linpro.com/ 
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index f06d0a9..6b1ff0a 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -18968,6 +18968,24 @@ postgres=# SELECT * FROM pg_walfile_name_offset(pg_stop_backup());
 be called when connected to the same database the slot was created on.

   
+  
+   
+
+ pg_move_replication_slot
+
+pg_move_replication_slot(slot_name name, position pg_lsn)
+   
+   
+bool
+   
+   
+Moves the current restart position of a physical or logical
+replication slot named slot_name.
+The slot will not be moved backwards, and it will not be
+moved beyond the current insert location. Returns true if
+the position was changed.
+   
+  
 
   

diff --git a/src/backend/replication/slotfuncs.c b/src/backend/replication/slotfuncs.c
index 6ee1e68..a9faa10 100644
--- a/src/backend/replication/slotfuncs.c
+++ b/src/backend/replication/slotfuncs.c
@@ -176,6 +176,66 @@ pg_drop_replication_slot(PG_FUNCTION_ARGS)
 }
 
 /*
+ * SQL function for moving the position in a replication slot.
+ */
+Datum
+pg_move_replication_slot(PG_FUNCTION_ARGS)
+{
+	Name		slotname = PG_GETARG_NAME(0);
+	XLogRecPtr	moveto = PG_GETARG_LSN(1);
+	char	   *slotnamestr;
+	bool		changed = false;
+	bool 		backwards = false;
+
+	Assert(!MyReplicationSlot);
+
+	check_permissions();
+
+	if (XLogRecPtrIsInvalid(moveto))
+		ereport(ERROR,
+(errmsg("Invalid target xlog position ")));
+
+	/* Temporarily acquire the slot so we "own" it */
+	ReplicationSlotAcquire(NameStr(*slotname));
+
+	if (moveto > GetXLogWriteRecPtr())
+		/* Can't move past current position, so truncate there */
+		moveto = GetXLogWriteRecPtr();
+
+	/* Now adjust it */
+	SpinLockAcquire(&MyReplicationSlot->mutex);
+	if (MyReplicationSlot->data.restart_lsn != moveto)
+	{
+		/* Never move backwards, because bad things can happen */
+		if (MyReplicationSlot->data.restart_lsn > moveto)
+			backwards = true;
+		else
+		{
+			MyReplicationSlot->data.restart_lsn = moveto;
+			changed = true;
+		}
+	}
+	SpinLockRelease(&MyReplicationSlot->mutex);
+
+	if (backwards)
+		ereport(WARNING,
+(errmsg("Not moving replication slot backwards!")));
+
+
+	if (changed)
+	{
+		ReplicationSlotMarkDirty();
+		ReplicationSlotsComputeRequiredLSN();
+		ReplicationSlotSave();
+	}
+
+	ReplicationSlotRelease();
+
+	PG_RETURN_BOOL(changed);
+}
+
+
+/*
  * pg_get_replication_slots - SQL SRF showing active replication slots.
  */
 Datum
diff --git a/src/include/catalog/pg_proc.h b/src/include/catalog/pg_proc.h
index 82562ad..04e97ff 100644
--- a/src/include/catalog/pg_proc.h
+++ b/src/include/catalog/pg_proc.h
@@ -5289,6 +5289,8 @@ DATA(insert OID = 3779 (  pg_create_physical_replication_slot PGNSP PGUID 12 1 0
 DESCR("create a physical replication slot");
 DATA(insert OID = 3780 (  pg_drop_replication_slot PGNSP PGUID 12 1 0 0 0 f f f f t f v u 1 0 2278 "19" _null_ _null_ _null_ _null_ _null_ pg_drop_replication_slot _null_ _null_ _null_ ));
 DESCR("drop a replication slot");
+DATA(insert OID = 3998 (  pg_move_replication_slot PGNSP PGUID 12 1 0 0 0 f f f f t f v u 2 0 16 "19 3220" _null_ _null_ _null_ _null_ _null_ pg_move_replication_slot _null_ _null_ _null_ ));
+DESCR("move a replication slot position");
 DATA(insert OID = 3781 (  pg_get_replication_slots	PGNSP PGUID 12 1 10 0 0 f f f f f t s s 0 0 2249 "" "{19,19,25,26,16,16,23,28,28,3220,3220}" "{o,o,o,o,o,o,o,o,o,o,o}" "{slot_name,plugin,slot_type,datoid,temporary,active,active_pid,xmin,catalog_xmin,restart_lsn,confirmed_flush_lsn}" _null_ _null_ pg_get_replication_slots _null_ _null_ _null_ ));
 DESCR("information about replication slots currently in use");
 DATA(insert OID = 3786 (  pg_create_logical_replication_slot PGNSP PGUID 12 1 0 0 0 f f f f t f v u 3 0 2249 "19 19 16" "{19,19,16,25,3220}" "{i,i,i,o,o}" "{slot_name,plugin,temporary,slot_name,wal_position}" _null_ _null_ pg_create_logical_replication_slot _null_ _null_ _null_ ));

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Adding support for Default partition in partitioning

[Rajkumar Raghuwanshi]

On Thu, May 4, 2017 at 5:14 PM, Rahila Syed  wrote:

> The syntax implemented in this patch is as follows,
>
> CREATE TABLE p11 PARTITION OF p1 DEFAULT;
>
> Applied v9 patches, table description still showing old pattern of default
partition. Is it expected?

create table lpd (a int, b int, c varchar) partition by list(a);
create table lpd_d partition of lpd DEFAULT;

\d+ lpd
 Table "public.lpd"
 Column |   Type| Collation | Nullable | Default | Storage  |
Stats target | Description
+---+---+--+-+--+--+-
 a  | integer   |   |  | | plain
|  |
 b  | integer   |   |  | | plain
|  |
 c  | character varying |   |  | | extended
|  |
Partition key: LIST (a)
Partitions: lpd_d FOR VALUES IN (DEFAULT)

Re: [HACKERS] Function to move the position of a replication slot

[Craig Ringer]

On 4 May 2017 at 20:05, Magnus Hagander  wrote:
> PFA a patch that adds a new function, pg_move_replication_slot, that makes
> it possible to move the location of a replication slot without actually
> consuming all the WAL on it.

> This can be useful for example to keep replication slots in sync between
> different servers in a replication cluster.

It needs a test to ensure it only operates on physical slots. It
should ERROR on a logical slot, since it has no way of correctly
advancing the catalog_xmin or finding a reasonable restart_lsn  for
logical decoding.

I'm still fine with the name, since I plan to add that capability in
pg11 by running through logical decoding and ReorderBufferSkip()ing
each xact until we reach the target lsn.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Function to move the position of a replication slot

[Magnus Hagander]

On Thu, May 4, 2017 at 2:42 PM, Craig Ringer  wrote:

> On 4 May 2017 at 20:05, Magnus Hagander  wrote:
> > PFA a patch that adds a new function, pg_move_replication_slot, that
> makes
> > it possible to move the location of a replication slot without actually
> > consuming all the WAL on it.
>
> > This can be useful for example to keep replication slots in sync between
> > different servers in a replication cluster.
>
> It needs a test to ensure it only operates on physical slots. It
> should ERROR on a logical slot, since it has no way of correctly
> advancing the catalog_xmin or finding a reasonable restart_lsn  for
> logical decoding.
>

I guess that makes sense, yeah. I didn't try it with that :)



> I'm still fine with the name, since I plan to add that capability in
> pg11 by running through logical decoding and ReorderBufferSkip()ing
> each xact until we reach the target lsn.
>

Cool.

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

Re: [HACKERS] Function to move the position of a replication slot

[Craig Ringer]

On 4 May 2017 at 20:45, Magnus Hagander  wrote:
> On Thu, May 4, 2017 at 2:42 PM, Craig Ringer  wrote:
>>
>> On 4 May 2017 at 20:05, Magnus Hagander  wrote:
>> > PFA a patch that adds a new function, pg_move_replication_slot, that
>> > makes
>> > it possible to move the location of a replication slot without actually
>> > consuming all the WAL on it.
>>
>> > This can be useful for example to keep replication slots in sync between
>> > different servers in a replication cluster.
>>
>> It needs a test to ensure it only operates on physical slots. It
>> should ERROR on a logical slot, since it has no way of correctly
>> advancing the catalog_xmin or finding a reasonable restart_lsn  for
>> logical decoding.
>
>
> I guess that makes sense, yeah. I didn't try it with that :)

Barring that and matching docs changes, looks fine to me at first glance.

Not sure you need to acquire the spinlock on the slot, since you
acquired the slot for your backend. It won't hurt, but I don't think
it's necessary.

I'm not convinced you need a WARNING for moving the slot backwards. If
one is emitted, it should be a proper ereport with current position
and requested position in errdetail. I'm not sure it's a useful
message though.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Reducing runtime of stats regression test

[Alvaro Herrera]

Tom Lane wrote:

> The other significant delay in stats.sql is
> 
> -- force the rate-limiting logic in pgstat_report_stat() to time out
> -- and send a message
> SELECT pg_sleep(1.0);
> 
> Now, we do seem to need a delay there, because the rate-limiting logic
> is unlikely to have permitted the count from the immediately preceding
> statement to have gotten sent right then, and the count won't get
> sent at all while we're inside wait_for_stats (since backends only
> send stats just before going idle).  But there's more than one way
> to skin this cat.  We can just start a new connection with \c, and
> let wait_for_stats wait for the old one to send its stats before quitting.
> Even on my oldest and slowest buildfarm machines, starting a new session
> takes well under one second.

So you changed table prevstats from temp to permanent; perhaps make it
unlogged?

I wonder if it'd be useful to have a "pg_stat_flush" or something, which
sends out whatever is queued in this session.  Then you wouldn't need
the reconnection.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Reducing runtime of stats regression test

[Tom Lane]

Alvaro Herrera  writes:
> Tom Lane wrote:
>> We can just start a new connection with \c, and
>> let wait_for_stats wait for the old one to send its stats before quitting.
>> Even on my oldest and slowest buildfarm machines, starting a new session
>> takes well under one second.

> So you changed table prevstats from temp to permanent;

Right, so it would survive into the new session.

> perhaps make it unlogged?

Hardly seems worth it; there's not much data in it.  And we're not
generally in the habit of marking short-lived tables as unlogged
elsewhere in the regression tests.  (Maybe we should make some
effort to have some of them be so marked, but that seems like
material for its own patch.)

> I wonder if it'd be useful to have a "pg_stat_flush" or something, which
> sends out whatever is queued in this session.  Then you wouldn't need
> the reconnection.

Yes, but that would be getting into the realm of new features, not
post-feature-freeze test adjustments.  It certainly couldn't be
a candidate for back-patching.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] statement_timeout is not working as expected with postgres_fdw

[Robert Haas]

On Thu, May 4, 2017 at 7:13 AM, Amit Kapila  wrote:
> In pgfdw_xact_callback, if the execution of ABORT TRANSACTION fails
> due to any reason then I think it will close the connection.  The
> relavant code is:
> if (PQstatus(entry->conn) != CONNECTION_OK ||
> PQtransactionStatus(entry->conn) != PQTRANS_IDLE)
>
> Basically, if abort transaction fails then transaction status won't be
> PQTRANS_IDLE.

I don't think that's necessarily true.  Look at this code:

 /* Rollback all remote subtransactions during abort */
 snprintf(sql, sizeof(sql),
  "ROLLBACK TO SAVEPOINT s%d; RELEASE SAVEPOINT s%d",
  curlevel, curlevel);
 res = PQexec(entry->conn, sql);
 if (PQresultStatus(res) != PGRES_COMMAND_OK)
 pgfdw_report_error(WARNING, res, entry->conn, true, sql);
 else
 PQclear(res);

If that sql command somehow returns a result status other than
PGRES_COMMAND_OK, like say due to a local OOM failure or whatever, the
connection is PQTRANS_IDLE but the rollback wasn't actually done.

> Also, does it matter if pgfdw_subxact_callback fails
> during execution of "ROLLBACK TO SAVEPOINT", because anyway user needs
> to execute rollback to savepoint command before proceeding and that
> should be good enough?

I don't understand this.  If pgfdw_subxact_callback doesn't roll the
remote side back to the savepoint, how is it going to get done later?
There's no code in postgres_fdw other than that code to issue the
rollback.

> About patch:
>
> + /* Rollback all remote subtransactions during abort */
> + snprintf(sql, sizeof(sql),
> + "ROLLBACK TO SAVEPOINT s%d; RELEASE SAVEPOINT s%d",
> + curlevel, curlevel);
> + if (!pgfdw_exec_cleanup_query(entry->conn, sql))
> + abort_cleanup_failure = true;
>
> If the above execution fails due to any reason, then it won't allow
> even committing the mail transaction which I am not sure is the right
> thing to do.

Well, as I said in my reply to Tushar, I think it's the only correct
thing to do.  Suppose you do the following:

(1) make a change to the foreign table
(2) enter a subtransaction
(3) make another change to the foreign table
(4) roll back the subtransaction
(5) commit the transaction

If the remote rollback gets skipped in step 4, it is dead wrong for
step 5 to commit the remote transaction anyway.  Both the change made
in step (1) and the change made in step (3) would get made on the
remote side, which is 100% wrong.  Given the failure of the rollback
in step 4, there is no way to achieve the desired outcome of
committing the step (1) change and rolling back the step (3) change.
The only way forward is to roll back everything - i.e. force a
toplevel abort.

>  #include "utils/memutils.h"
> -
> +#include "utils/syscache.h"
>
> Looks like spurious line removal

OK.

>> - For bonus points, give pgfdw_exec_query() an optional timeout
>> argument, and set it to 30 seconds or so when we're doing abort
>> cleanup.  If the timeout succeeds, it errors out (which again
>> re-enters the abort path, dropping the connection and forcing outer
>> transaction levels to abort as well).
>
> Why do we need this 30 seconds timeout for abort cleanup failures?
> Isn't it sufficient if we propagate the error only on failures?

Well, the problem is that right now we're waiting for vast amounts of
time when the remote connection dies.  First, the user notices that
the command is running for too long and hits ^C.  At that point, the
connection is probably already dead, and the user's been waiting for a
while already.  Then, we wait for PQcancel() to time out.  Then, we
wait for PQexec() to time out.  The result of that, as Suraj mentioned
in the original email, is that it takes about 16 minutes for the
session to recover when the connection dies, even with keepalive
settings and connection timeout set all the way to the minimum.  The
goal here is to make it take a more reasonable time to recover.
Without modifying libpq, we can't do anything about the need to wait
for PQcancel() to time out, but we can improve the rest of it.  If we
removed that 30-second timeout, we would win in the case where either
ABORT TRANSACTION or ROLLBACK; RELEASE eventually succeeds but takes
more than 30 seconds.  In that case, the change you are proposing
would allow us to reuse the connection instead of dropping it, and
would prevent a forced toplevel abort when subtransactions are in use.
However, the cost would be that when one of those commands never
succeeds, we would wait a lot longer before finishing a transaction
abort.  I argue that if the remote server fails to respond to one of
those commands within 30 seconds, it's probably dead or nearly dead,
and we're probably better off just treating it as dead rather than
waiting longer.  That's a judgement call, of course.

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


-- 
Sent via pgsql-hackers mailin

Re: [HACKERS] transition table behavior with inheritance appears broken (was: Declarative partitioning - another take)

[Robert Haas]

On Thu, May 4, 2017 at 4:46 AM, Thomas Munro
 wrote:
> On Thu, May 4, 2017 at 4:02 AM, Alvaro Herrera  
> wrote:
>> Robert Haas wrote:
>>> I suspect that most users would find it more useful to capture all of
>>> the rows that the statement actually touched, regardless of whether
>>> they hit the named table or an inheritance child.
>>
>> Yes, agreed.  For the plain inheritance cases each row would need to
>> have an indicator of which relation it comes from (tableoid); I'm not
>> sure if such a thing would be useful in the partitioning case.
>
> On Thu, May 4, 2017 at 4:26 AM, David Fetter  wrote:
>> +1 on the not-duct-tape view of partitioned tables.
>
> Hmm.  Ok.  Are we talking about PG10 or PG11 here?  Does this approach
> makes sense?

I was thinking PG10 if it can be done straightforwardly.

> 1.  Remove the prohibition on creating transition-capturing triggers
> on a partitioned table.
>
> 2.  Make sure that the ExecAR* functions call AfterTriggerSaveEvent
> when modifying partition tables if the explicitly named parent
> partitioned table has after triggers with transition tables.  Not sure
> how exactly how but doesn't seem difficult.
>
> 3.  Convert tuples to the TupleDesc of the relation that owns the
> statement trigger (ie the partitioned table) when inserting them into
> the tuplestore.  One way to do that might be to build an array of
> TupleConversionMap objects that does the opposite of the conversions
> done by tup_conv_maps.  While tup_conv_maps is for converting tuples
> to the layout needed for a partition, tup_unconv_maps (or better name)
> would be for converting the old and new tuples to the TupleDesc of the
> partitioned table.  Then the appropriate TupleConversionMap could be
> passed into the ExecAR* functions as a new argument 'transition_map'.
> AfterTriggerSaveEvent would use 'oldtup' and 'newtup' directly for ROW
> triggers, but convert using the passed in map if it needs to insert
> them into the transition tuplestores.
>
> The same thing could work for inheritance, if tupconvert.c had a new
> kind of conversion that allows slicing of tuples (converting a wider
> child table's tuples to the parent's subset of columns) rather the
> just conversion between logically equivalent TupleDescs.
>
> To avoid the whiff of duct tape, we'd probably also want to make ROW
> triggers created on the partitioned table(s) containing partition to
> fire too, with appropriate TypeConversionMap treatment.  Not sure what
> exactly is involved there.
>
> On the other hand, doing that with inheritance hierarchies would be an
> incompatible behavioural change, which I guess people don't want -- am
> I right?

Incompatible with what?  Transition tables haven't been released yet.
If we're going to fix the definition of what they do, now's the time.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patch - Tcl 8.6 version support for PostgreSQL

[Alvaro Herrera]

Tom Lane wrote:

> But I agree with Andres' complaint that just duplicating the code isn't
> the best way.  The configure script has a loop that's basically like
> 
> for f in tclsh tcl tclsh8.6 tclsh86 tclsh8.5 tclsh85 tclsh8.4 tclsh84 
> tclsh8.3 tclsh83
> do
>... break if $f is the right one
> done
> 
> Seems to me that a similar coding pattern in the MSVC script is a
> reasonable way to go.

Something like the (untested) attached perhaps?

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
diff --git a/src/tools/msvc/Mkvcbuild.pm b/src/tools/msvc/Mkvcbuild.pm
index 30c126503b..c0bc25292f 100644
--- a/src/tools/msvc/Mkvcbuild.pm
+++ b/src/tools/msvc/Mkvcbuild.pm
@@ -204,20 +204,24 @@ sub mkvcbuild
 
if ($solution->{options}->{tcl})
{
+   my $found = 0;
my $pltcl =
  $solution->AddProject('pltcl', 'dll', 'PLs', 'src/pl/tcl');
$pltcl->AddIncludeDir($solution->{options}->{tcl} . '/include');
$pltcl->AddReference($postgres);
-   if (-e $solution->{options}->{tcl} . '/lib/tcl85.lib')
+
+   for my $tclver (qw(86t 85 84))
{
-   $pltcl->AddLibrary(
-   $solution->{options}->{tcl} . '/lib/tcl85.lib');
-   }
-   else
-   {
-   $pltcl->AddLibrary(
-   $solution->{options}->{tcl} . '/lib/tcl84.lib');
+   my $tcllib = $solution->{options}->{tcl} . 
"/lib/tcl$tclver.lib";
+   if (-e $tcllib)
+   {
+   $pltcl->AddLibrary($tcllib);
+   $found = 1;
+   last;
+   }
}
+   die "Unable to find 
$solution->{options}->{tcl}/lib/tcl.lib"
+   unless $found;
}
 
$libpq = $solution->AddProject('libpq', 'dll', 'interfaces',

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patch - Tcl 8.6 version support for PostgreSQL

[Tom Lane]

Alvaro Herrera  writes:
> Something like the (untested) attached perhaps?

Looks plausible, I'm not in a position to test though.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patch - Tcl 8.6 version support for PostgreSQL

[Dave Page]

On Thu, May 4, 2017 at 3:54 PM, Tom Lane  wrote:

> Alvaro Herrera  writes:
> > Something like the (untested) attached perhaps?
>
> Looks plausible, I'm not in a position to test though.


Sandeep/Paresh - can you test please?

-- 
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

[HACKERS] idea: custom log_line_prefix components besides application_name

[Chapman Flack]

Hi,

At $work I am often entertained by log entries like:

invalid input syntax for integer: "21' && 1=2)) Uni/**/ON
SEl/**/eCT 0x646665743166657274,0x646665743266657274,
0x646665743366657274 -- "

They're entertaining mostly because I know our web guy has heard
of SQL injection and doesn't write stuff where it would work. So
I'm mostly just entertained to see what wacky tactics the lowlifes
come up with.

But suppose I wanted to log more information about the origins
of the attempts. As is often the case with web apps, %r/%h/%u
won't give me anything useful, because %h will always be our
own server hosting the app, and %u is the hardcoded name the app
uses to connect. Obviously, each app itself would need to be tweaked
to pass along what I really care about, the actual remote host and
the user identity within the app.

That could be done today by having the app construct some 64-
character string out of the app name, remote IP, and identity,
and setting that as application_name, and I could log %a.

I just wonder if anybody thinks web apps, and therefore this
scenario, are common enough these days to maybe justify one
or two more GUCs with their own log_line_prefix escapes, such
as app_client_addr or app_user. Naturally they would only be
as reliable as the app setting them, and uninterpreted by
PostgreSQL itself, and so functionally no different from the
uninterpreted string already available as application_name.
The benefit is perhaps to be clearer than just overloading
application_name to carry two or three pieces of information
(and perhaps privacy, if you care about app user identities and
source IPs showing up in ps titles).

Worth considering, or is application_name Good Enough?

-Chap


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] CTE inlining

[Serge Rielau]

> On May 4, 2017, at 3:02 AM, Gavin Flower  
> wrote:
> 
> On 30/04/17 16:28, Tom Lane wrote:
>> Craig Ringer  writes:
>>> - as you noted, it is hard to decide when it's worth inlining vs
>>> materializing for CTE terms referenced more than once.
>> [ raised eyebrow... ]  Please explain why the answer isn't trivially
>> "never".
>> 
>> There's already a pretty large hill to climb here in the way of
>> breaking peoples' expectations about CTEs being optimization
>> fences.  Breaking the documented semantics about CTEs being
>> single-evaluation seems to me to be an absolute non-starter.
>> 
>>  regards, tom lane
>> 
>> 
> Could not each CTE be only evaluated once, but restricted (as far as is 
> practicable) to the rows actually needed by the body of the SELECT?
> 
Tom,

Are you worried about semantics or performance?
With proper detection of mutating functions and snapshot isolation I do not see 
how a user would detect “lack of” single evaluation.
As for performance we’d be talking about what? An uncorrelated inner of a 
nested loop join?

Anyway it seems to me that there a multiple properties at play here which are 
quite orthogonal.

1. Full materialization/Slow materialization/pipelining
  I cannot come up with any scenario where full materialization would be 
beneficial from a performance point of view (which speaks to Gavin’s view).
  I can see it from a semantic point of view when order of execution may matter 
(for example with embedded DML and triggers present).
  As soon as semantics are at play having syntax is absolutely the right thing: 
+1 for MATERIALIZE
2.Pushing predicates (or other operators) into the CTE.
   All this can ever do is reduce the number of rows being looked at.
   As long as the optimizer is careful, not to do what it isn’t supposed to do 
in a nested query (push past a mutating function) I don’t see the harm
3. Replicating the CTE to push distinct operators from different consumers.
   Again this can only be done if there are no mutators or non deterministic 
operators.

To summarize: big +1 to preserve the existing behavior with MATERIALIZE and to 
set CTE’s free by default with the onus on the optimizer to prove semantic 
equivalence.

Cheers
Serge Rielau
Salesforce.com

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] CTE inlining

[Tom Lane]

Serge Rielau  writes:
>> On May 4, 2017, at 3:02 AM, Gavin Flower  
>> wrote:
>> On 30/04/17 16:28, Tom Lane wrote:
>>> There's already a pretty large hill to climb here in the way of
>>> breaking peoples' expectations about CTEs being optimization
>>> fences.  Breaking the documented semantics about CTEs being
>>> single-evaluation seems to me to be an absolute non-starter.

> Are you worried about semantics or performance?
> With proper detection of mutating functions and snapshot isolation I do not 
> see how a user would detect “lack of” single evaluation.

I do not think a user will have any trouble "detecting" what we did
if his query runs for two hours, rather than two minutes, because we
executed some expensive function 100 times rather than the one time
he expected it to run.

Now you could argue that that's user error because he should have
marked the expensive function with a sufficiently high cost to
discourage us from flattening the CTE.  But not everyone will have
done that (and I'm not sure we have any planner smarts that would
respond to such cases anyway).  So what I'm saying is that if you're
promising there will be no visible bad consequences, you're wrong.

It may be worth breaking some peoples' queries to make other peoples'
queries faster, but I think we need to tread pretty carefully there.
And we definitely can't change any case where there would be visible
semantic differences.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Should pg_current_wal_location() become pg_current_wal_lsn()

[David Rowley]

On 2 May 2017 at 00:10, David Rowley  wrote:
> On 20 April 2017 at 07:29, Euler Taveira  wrote:
>> 2017-04-19 1:32 GMT-03:00 Michael Paquier :
>>>
>>> I vote for "location" -> "lsn". I would expect complains about the
>>> current inconsistency at some point, and the function names have been
>>> already changed for this release..
>
> OK, so I've created a draft patch which does this.

I ended up adding this to the open items list.  I feel it's best to be
on there so that we don't forget about this.

If we decide not to do it then we can just remove it from the list,
but it would be a shame to release the beta having forgotten to make
this change.

Do any of the committers who voted for this change feel inclined to
pick this patch up?

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PROPOSAL] Use SnapshotAny in get_actual_variable_range

[Dmitriy Sarafannikov]

> Maybe we need another type of snapshot that would accept any
> non-vacuumable tuple.  I really don't want SnapshotAny semantics here,
> but a tuple that was live more recently than the xmin horizon seems
> like it's acceptable enough.  HeapTupleSatisfiesVacuum already
> implements the right behavior, but we don't have a Snapshot-style
> interface for it.


I have tried to implement this new type of snapshot that accepts any
non-vacuumable tuples.
We have tried this patch in our load environment. And it has smoothed out
and reduced magnitude of the cpu usage peaks.
But this snapshot does not solve the problem completely.

Patch is attached.



snapshot_non_vacuumable.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] statement_timeout is not working as expected with postgres_fdw

[Amit Kapila]

On Thu, May 4, 2017 at 8:08 PM, Robert Haas  wrote:
> On Thu, May 4, 2017 at 7:13 AM, Amit Kapila  wrote:
>
>>> - For bonus points, give pgfdw_exec_query() an optional timeout
>>> argument, and set it to 30 seconds or so when we're doing abort
>>> cleanup.  If the timeout succeeds, it errors out (which again
>>> re-enters the abort path, dropping the connection and forcing outer
>>> transaction levels to abort as well).
>>
>> Why do we need this 30 seconds timeout for abort cleanup failures?
>> Isn't it sufficient if we propagate the error only on failures?
>
> Well, the problem is that right now we're waiting for vast amounts of
> time when the remote connection dies.  First, the user notices that
> the command is running for too long and hits ^C.  At that point, the
> connection is probably already dead, and the user's been waiting for a
> while already.  Then, we wait for PQcancel() to time out.  Then, we
> wait for PQexec() to time out.  The result of that, as Suraj mentioned
> in the original email, is that it takes about 16 minutes for the
> session to recover when the connection dies, even with keepalive
> settings and connection timeout set all the way to the minimum.  The
> goal here is to make it take a more reasonable time to recover.
> Without modifying libpq, we can't do anything about the need to wait
> for PQcancel() to time out, but we can improve the rest of it.  If we
> removed that 30-second timeout, we would win in the case where either
> ABORT TRANSACTION or ROLLBACK; RELEASE eventually succeeds but takes
> more than 30 seconds.  In that case, the change you are proposing
> would allow us to reuse the connection instead of dropping it, and
> would prevent a forced toplevel abort when subtransactions are in use.
> However, the cost would be that when one of those commands never
> succeeds, we would wait a lot longer before finishing a transaction
> abort.
>

As soon as the first command fails due to timeout, we will set
'abort_cleanup_failure' which will make a toplevel transaction to
abort and also won't allow other statements to execute.  The patch is
trying to enforce a 30-second timeout after statement execution, so it
has to anyway wait till the command execution completes (irrespective
of whether the command succeeds or fail).  It is quite possible I am
missing some point, is it possible for you to tell in somewhat more
detail in which exact case 30-second timeout will allow us to abort
the toplevel transaction if we already do that in the case of
statement failure case?


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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] CTE inlining

[Andrew Dunstan]

On 05/04/2017 11:36 AM, Tom Lane wrote:
> Serge Rielau  writes:
>>> On May 4, 2017, at 3:02 AM, Gavin Flower  
>>> wrote:
>>> On 30/04/17 16:28, Tom Lane wrote:
 There's already a pretty large hill to climb here in the way of
 breaking peoples' expectations about CTEs being optimization
 fences.  Breaking the documented semantics about CTEs being
 single-evaluation seems to me to be an absolute non-starter.
>> Are you worried about semantics or performance?
>> With proper detection of mutating functions and snapshot isolation I do not 
>> see how a user would detect “lack of” single evaluation.
> I do not think a user will have any trouble "detecting" what we did
> if his query runs for two hours, rather than two minutes, because we
> executed some expensive function 100 times rather than the one time
> he expected it to run.
>
> Now you could argue that that's user error because he should have
> marked the expensive function with a sufficiently high cost to
> discourage us from flattening the CTE.  But not everyone will have
> done that (and I'm not sure we have any planner smarts that would
> respond to such cases anyway).  So what I'm saying is that if you're
> promising there will be no visible bad consequences, you're wrong.
>
> It may be worth breaking some peoples' queries to make other peoples'
> queries faster, but I think we need to tread pretty carefully there.
> And we definitely can't change any case where there would be visible
> semantic differences.
>

Yeah, the idea that this won't cause possibly significant pain is quite wrong. 
Quite by accident I came across an example just this morning where rewriting as 
a CTE makes a big improvement.

I wrote this query:

select (json_populate_record(null::mytype, myjson)).*
from mytable;


It turned out that this was an order of magnitude faster:

with r as
(
   select json_populate_record(null::mytype, myjson) as x
   from mytable
)
select (x).*
from r;


cheers

andrew



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



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] CTE inlining

[David G. Johnston]

On Thu, May 4, 2017 at 9:22 AM, Andrew Dunstan <
andrew.duns...@2ndquadrant.com> wrote:

>
> Yeah, the idea that this won't cause possibly significant pain is quite
> wrong. Quite by accident I came across an example just this morning where
> rewriting as a CTE makes a big improvement.
>
> I wrote this query:
>
> select (json_populate_record(null::mytype, myjson)).*
> from mytable;
>
>
> It turned out that this was an order of magnitude faster:
>
> with r as
> (
>select json_populate_record(null::mytype, myjson) as x
>from mytable
> )
> select (x).*
> from r;
>

​Except I suspect we at least have a chance to detect the above and not
de-optimize it by evaluating "json_populate_record" once for every column
in mytype.

The now idiomatic solution​ to the above is to use LATERAL so the above CTE
is no longer actually a required workaround.

David J.

Re: [HACKERS] CTE inlining

[Andres Freund]

On 2017-05-04 09:34:19 -0700, David G. Johnston wrote:
> On Thu, May 4, 2017 at 9:22 AM, Andrew Dunstan <
> andrew.duns...@2ndquadrant.com> wrote:
> 
> >
> > Yeah, the idea that this won't cause possibly significant pain is quite
> > wrong. Quite by accident I came across an example just this morning where
> > rewriting as a CTE makes a big improvement.
> >
> > I wrote this query:
> >
> > select (json_populate_record(null::mytype, myjson)).*
> > from mytable;
> >
> >
> > It turned out that this was an order of magnitude faster:
> >
> > with r as
> > (
> >select json_populate_record(null::mytype, myjson) as x
> >from mytable
> > )
> > select (x).*
> > from r;

That's a very specific issue, that you can just as well solve with CTEs
though.  IIRC Tom and I were discussing changing this - independently of
anything CTE releated - so it doesn't end up calling the function
multiple times. We e.g. could now quite easily put the
json_populate_record(null::mytype, myjson) into something *like* a
nodeProjectSet node, and handle the column accesses via the nromal
projection.


> ​Except I suspect we at least have a chance to detect the above and not
> de-optimize it by evaluating "json_populate_record" once for every column
> in mytype.

That's what happens with the non CTE version - don't think it's super
likely we'd end up doing that for CTEs; I'd even consider it a bug.

Greetings,

Andres Freund


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] statement_timeout is not working as expected with postgres_fdw

[Robert Haas]

On Thu, May 4, 2017 at 12:18 PM, Amit Kapila  wrote:
> As soon as the first command fails due to timeout, we will set
> 'abort_cleanup_failure' which will make a toplevel transaction to
> abort and also won't allow other statements to execute.  The patch is
> trying to enforce a 30-second timeout after statement execution, so it
> has to anyway wait till the command execution completes (irrespective
> of whether the command succeeds or fail).

I don't understand what you mean by this.  If the command doesn't
finish within 30 seconds, we *don't* wait for it to finish.  To avoid
getting inconsistent results in consequence, we set
abort_cleanup_failure.

> It is quite possible I am
> missing some point, is it possible for you to tell in somewhat more
> detail in which exact case 30-second timeout will allow us to abort
> the toplevel transaction if we already do that in the case of
> statement failure case?

Suppose the user's original connection is stuck for some reason but
the postmaster is still accepting new connections - e.g. somebody
attached gdb to the remote server process and it is therefore stopped.
The cancel request gets sent OK, but without the 30-second timeout,
we'll hang forever (or until gdb continues or detaches the processes)
waiting for it to take effect.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Reducing runtime of stats regression test

[Robert Haas]

On Thu, May 4, 2017 at 10:22 AM, Tom Lane  wrote:
> Yes, but that would be getting into the realm of new features, not
> post-feature-freeze test adjustments.  It certainly couldn't be
> a candidate for back-patching.

I'm not sure there's some bright line between adding a new
SQL-callable function to cut down the test time and any other
tinkering we might do to reduce the regression test time.  I think
there's a pretty good argument that all of the recent changes you made
in this area constitute strictly optional tinkering.  I'm haven't been
objecting because they don't seem likely to destabilize anything, but
I don't see that they're really helping us get ready for beta either,
which is presumably what we ought to be focusing on at this point.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] statement_timeout is not working as expected with postgres_fdw

[Amit Kapila]

On Thu, May 4, 2017 at 10:18 PM, Robert Haas  wrote:
> On Thu, May 4, 2017 at 12:18 PM, Amit Kapila  wrote:
>> As soon as the first command fails due to timeout, we will set
>> 'abort_cleanup_failure' which will make a toplevel transaction to
>> abort and also won't allow other statements to execute.  The patch is
>> trying to enforce a 30-second timeout after statement execution, so it
>> has to anyway wait till the command execution completes (irrespective
>> of whether the command succeeds or fail).
>
> I don't understand what you mean by this.  If the command doesn't
> finish within 30 seconds, we *don't* wait for it to finish.
>

+ /*
+ * Submit a query.  Since we don't use non-blocking mode, this also can
+ * block.  But its risk is relatively small, so we ignore that for now.
+ */
+ if (!PQsendQuery(conn, query))
+ {
+ pgfdw_report_error(WARNING, NULL, conn, false, query);
+ return false;
+ }
+
+ /* Get the result of the query. */
+ if (pgfdw_get_cleanup_result(conn, endtime, &result))
+ return false;

The 30 seconds logic is in function pgfdw_get_cleanup_result, can't
the command hang in PQsendQuery?



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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] statement_timeout is not working as expected with postgres_fdw

[Robert Haas]

On Thu, May 4, 2017 at 1:04 PM, Amit Kapila  wrote:
> On Thu, May 4, 2017 at 10:18 PM, Robert Haas  wrote:
>> On Thu, May 4, 2017 at 12:18 PM, Amit Kapila  wrote:
>>> As soon as the first command fails due to timeout, we will set
>>> 'abort_cleanup_failure' which will make a toplevel transaction to
>>> abort and also won't allow other statements to execute.  The patch is
>>> trying to enforce a 30-second timeout after statement execution, so it
>>> has to anyway wait till the command execution completes (irrespective
>>> of whether the command succeeds or fail).
>>
>> I don't understand what you mean by this.  If the command doesn't
>> finish within 30 seconds, we *don't* wait for it to finish.
>>
>
> + /*
> + * Submit a query.  Since we don't use non-blocking mode, this also can
> + * block.  But its risk is relatively small, so we ignore that for now.
> + */
> + if (!PQsendQuery(conn, query))
> + {
> + pgfdw_report_error(WARNING, NULL, conn, false, query);
> + return false;
> + }
> +
> + /* Get the result of the query. */
> + if (pgfdw_get_cleanup_result(conn, endtime, &result))
> + return false;
>
> The 30 seconds logic is in function pgfdw_get_cleanup_result, can't
> the command hang in PQsendQuery?

Sure.  I thought about trying to fix that too, by using
PQsetnonblocking(), but I thought the patch was doing enough already.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] CTE inlining

[Andrew Dunstan]

On 05/04/2017 12:34 PM, David G. Johnston wrote:
> On Thu, May 4, 2017 at 9:22 AM, Andrew Dunstan
>  >wrote:
>
>
> Yeah, the idea that this won't cause possibly significant pain is
> quite wrong. Quite by accident I came across an example just this
> morning where rewriting as a CTE makes a big improvement.
>
> I wrote this query:
>
> select (json_populate_record(null::mytype, myjson)).*
> from mytable;
>
>
> It turned out that this was an order of magnitude faster:
>
> with r as
> (
>select json_populate_record(null::mytype, myjson) as x
>from mytable
> )
> select (x).*
> from r;
>
>
> ​Except I suspect we at least have a chance to detect the above and
> not de-optimize it by evaluating "json_populate_record" once for every
> column in mytype.
>
> The now idiomatic solution​ to the above is to use LATERAL so the
> above CTE is no longer actually a required workaround.


Hadn't though about LATERAL, good point. Still, there will be other cases.

cheers

andrew


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



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] json_agg produces nonstandard json

[Jordan Deitch]

Hello!

I apologize in advanced if this has been previously discussed;

A json(b)_agg() will produce the following result when no results are
passed in:

"[null]"

per:

select jsonb_agg((select 1 where false));

I believe, generally speaking, '[]' would be the more appropriate output.

Would postgres welcome a patch to handle the empty case of json(b)_agg?

Thanks!

---
Jordan Deitch

Re: [HACKERS] CTE inlining

[Alvaro Herrera]

Andrew Dunstan wrote:

> Hadn't though about LATERAL, good point. Still, there will be other cases.

I'm not sure what your point is.  We know that for some cases the
optimization barrier semantics are useful, which is why the proposal is
to add a keyword to install one explicitely:

 with materialized r as
 (
select json_populate_record(null::mytype, myjson) as x
from mytable
 )
 select (x).*
 from r;

this would preserve the current semantics.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] CTE inlining

[Andrew Dunstan]

On 05/04/2017 01:33 PM, Alvaro Herrera wrote:
> Andrew Dunstan wrote:
>
>> Hadn't though about LATERAL, good point. Still, there will be other cases.
> I'm not sure what your point is.  We know that for some cases the
> optimization barrier semantics are useful, which is why the proposal is
> to add a keyword to install one explicitely:
>
>  with materialized r as
>  (
> select json_populate_record(null::mytype, myjson) as x
> from mytable
>  )
>  select (x).*
>  from r;
>
> this would preserve the current semantics.
>


I understand. Some people appear to think hardly anyone will actually
need this - that's what I was arguing against.

cheers

andrew

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



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP Patch: Precalculate stable functions, infrastructure v1

[Marina Polyakova]

and here I send infrastructure patch which includes <...>


Next 2 patches:

Patch 'planning and execution', which includes:
- replacement nonvolatile functions and operators by appropriate cached 
expressions;

- planning and execution cached expressions;
- regression tests.

Patch 'costs', which includes cost changes for cached expressions 
(according to their behaviour).


--
Marina Polyakova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres CompanyFrom cf446cbfc8625701f9e3f32d1870b47de869802a Mon Sep 17 00:00:00 2001
From: Marina Polyakova 
Date: Thu, 4 May 2017 19:36:05 +0300
Subject: [PATCH 3/3] Precalculate stable functions, costs v1

Now in Postgresql only immutable functions are precalculated; stable functions
are calculated for every row so in fact they don't differ from volatile
functions.

This patch includes:
- cost changes for cached expressions (according to their behaviour)
---
 src/backend/optimizer/path/costsize.c | 58 ---
 1 file changed, 53 insertions(+), 5 deletions(-)

diff --git a/src/backend/optimizer/path/costsize.c b/src/backend/optimizer/path/costsize.c
index 52643d0..34707fa 100644
--- a/src/backend/optimizer/path/costsize.c
+++ b/src/backend/optimizer/path/costsize.c
@@ -140,6 +140,7 @@ static MergeScanSelCache *cached_scansel(PlannerInfo *root,
 			   PathKey *pathkey);
 static void cost_rescan(PlannerInfo *root, Path *path,
 			Cost *rescan_startup_cost, Cost *rescan_total_cost);
+static double cost_eval_cacheable_expr_per_tuple(Node *node);
 static bool cost_qual_eval_walker(Node *node, cost_qual_eval_context *context);
 static void get_restriction_qual_cost(PlannerInfo *root, RelOptInfo *baserel,
 		  ParamPathInfo *param_info,
@@ -3464,6 +3465,44 @@ cost_qual_eval_node(QualCost *cost, Node *qual, PlannerInfo *root)
 	*cost = context.total;
 }
 
+/*
+ * cost_eval_cacheable_expr_per_tuple
+ *		Evaluate per tuple cost for expressions that can be cacheable.
+ *
+ * This function was created to not duplicate code for some expression and
+ * cached some expression.
+ */
+static double
+cost_eval_cacheable_expr_per_tuple(Node *node)
+{
+	double result;
+
+	/*
+	 * For each operator or function node in the given tree, we charge the
+	 * estimated execution cost given by pg_proc.procost (remember to multiply
+	 * this by cpu_operator_cost).
+	 */
+	if (IsA(node, FuncExpr))
+	{
+		result = get_func_cost(((FuncExpr *) node)->funcid) * cpu_operator_cost;
+	}
+	else if (IsA(node, OpExpr))
+	{
+		OpExpr *opexpr = (OpExpr *) node;
+
+		/* rely on struct equivalence to treat these all alike */
+		set_opfuncid(opexpr);
+
+		result = get_func_cost(opexpr->opfuncid) * cpu_operator_cost;
+	}
+	else
+	{
+		elog(ERROR, "non cacheable expression node type: %d", (int) nodeTag(node));
+	}
+
+	return result;
+}
+
 static bool
 cost_qual_eval_walker(Node *node, cost_qual_eval_context *context)
 {
@@ -3537,13 +3576,22 @@ cost_qual_eval_walker(Node *node, cost_qual_eval_context *context)
 	 * moreover, since our rowcount estimates for functions tend to be pretty
 	 * phony, the results would also be pretty phony.
 	 */
-	if (IsA(node, FuncExpr))
+	if (IsA(node, FuncExpr) ||
+		IsA(node, OpExpr))
 	{
-		context->total.per_tuple +=
-			get_func_cost(((FuncExpr *) node)->funcid) * cpu_operator_cost;
+		context->total.per_tuple += cost_eval_cacheable_expr_per_tuple(node);
+	}
+	else if (IsA(node, CachedExpr))
+	{	
+		/* 
+		 * Calculate subexpression cost per tuple as usual and add it to startup
+		 * cost (because subexpression will be executed only once for all
+		 * tuples).
+		 */
+		context->total.startup += cost_eval_cacheable_expr_per_tuple(
+			get_subexpr((CachedExpr *) node));
 	}
-	else if (IsA(node, OpExpr) ||
-			 IsA(node, DistinctExpr) ||
+	else if (IsA(node, DistinctExpr) ||
 			 IsA(node, NullIfExpr))
 	{
 		/* rely on struct equivalence to treat these all alike */
-- 
1.9.1

From 508f8b959ff9d1ab78dfc79ab4657b4c10a11690 Mon Sep 17 00:00:00 2001
From: Marina Polyakova 
Date: Thu, 4 May 2017 19:09:51 +0300
Subject: [PATCH 2/3] Precalculate stable functions, planning and execution v1

Now in Postgresql only immutable functions are precalculated; stable functions
are calculated for every row so in fact they don't differ from volatile
functions.

This patch includes:
- replacement nonvolatile functions and operators by appropriate cached
expressions
- planning and execution cached expressions
- regression tests
---
 src/backend/executor/execExpr.c|  70 ++
 src/backend/executor/execExprInterp.c  | 191 +
 src/backend/optimizer/path/allpaths.c  |   9 +-
 src/backend/optimizer/path/clausesel.c |  13 +
 src/backend/optimizer/plan/planagg.c   |   1 +
 src/backend/optimizer/plan/planner.c   |  28 +
 src/backend/optimizer/util/clauses.c   |  43 ++
 src/backend/utils/adt/ruleutils.c  |   5 +
 src/include/executor/execExpr.h   

Re: [HACKERS] CTE inlining

[Joe Conway]

On 05/04/2017 10:33 AM, Alvaro Herrera wrote:
> I'm not sure what your point is.  We know that for some cases the
> optimization barrier semantics are useful, which is why the proposal is
> to add a keyword to install one explicitely:
> 
>  with materialized r as
>  (
> select json_populate_record(null::mytype, myjson) as x
> from mytable
>  )
>  select (x).*
>  from r;
> 
> this would preserve the current semantics.

I haven't been able to follow this incredibly long thread, so please
excuse me if way off base, but are we talking about that a CTE would be
silently be rewritten as an inline expression potentially unless it is
decorated with some new syntax?

I would find that very disconcerting myself. For example, would this CTE
potentially get rewritten with multiple evaluation as follows?

DROP SEQUENCE IF EXISTS foo_seq;
CREATE SEQUENCE foo_seq;

WITH a(f1) AS (SELECT nextval('foo_seq'))
SELECT a.f1, a.f1 FROM a;
 f1 | ?column?
+--
  1 |1
(1 row)

ALTER SEQUENCE foo_seq RESTART;
SELECT nextval('foo_seq'), nextval('foo_seq');
 nextval | ?column?
-+--
   1 |2
(1 row)

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] CTE inlining

[Andrew Dunstan]

On 05/04/2017 01:52 PM, Joe Conway wrote:
> On 05/04/2017 10:33 AM, Alvaro Herrera wrote:
>> I'm not sure what your point is.  We know that for some cases the
>> optimization barrier semantics are useful, which is why the proposal is
>> to add a keyword to install one explicitely:
>>
>>  with materialized r as
>>  (
>> select json_populate_record(null::mytype, myjson) as x
>> from mytable
>>  )
>>  select (x).*
>>  from r;
>>
>> this would preserve the current semantics.
> I haven't been able to follow this incredibly long thread, so please
> excuse me if way off base, but are we talking about that a CTE would be
> silently be rewritten as an inline expression potentially unless it is
> decorated with some new syntax?
>
> I would find that very disconcerting myself. For example, would this CTE
> potentially get rewritten with multiple evaluation as follows?
>
> DROP SEQUENCE IF EXISTS foo_seq;
> CREATE SEQUENCE foo_seq;
>
> WITH a(f1) AS (SELECT nextval('foo_seq'))
> SELECT a.f1, a.f1 FROM a;
>  f1 | ?column?
> +--
>   1 |1
> (1 row)
>
> ALTER SEQUENCE foo_seq RESTART;
> SELECT nextval('foo_seq'), nextval('foo_seq');
>  nextval | ?column?
> -+--
>1 |2
> (1 row)
>



I think that would be a change in semantics, which we should definitely
not be getting. Avoiding a change in semantics might be an interesting
exercise, but we have lots of clever coders ...

cheers

andrew

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



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] CTE inlining

[Tomas Vondra]

On 5/4/17 7:56 PM, Andrew Dunstan wrote:



On 05/04/2017 01:52 PM, Joe Conway wrote:

On 05/04/2017 10:33 AM, Alvaro Herrera wrote:

I'm not sure what your point is.  We know that for some cases the
optimization barrier semantics are useful, which is why the proposal is
to add a keyword to install one explicitely:

  with materialized r as
  (
 select json_populate_record(null::mytype, myjson) as x
 from mytable
  )
  select (x).*
  from r;

this would preserve the current semantics.

I haven't been able to follow this incredibly long thread, so please
excuse me if way off base, but are we talking about that a CTE would be
silently be rewritten as an inline expression potentially unless it is
decorated with some new syntax?

I would find that very disconcerting myself. For example, would this CTE
potentially get rewritten with multiple evaluation as follows?

DROP SEQUENCE IF EXISTS foo_seq;
CREATE SEQUENCE foo_seq;

WITH a(f1) AS (SELECT nextval('foo_seq'))
SELECT a.f1, a.f1 FROM a;
  f1 | ?column?
+--
   1 |1
(1 row)

ALTER SEQUENCE foo_seq RESTART;
SELECT nextval('foo_seq'), nextval('foo_seq');
  nextval | ?column?
-+--
1 |2
(1 row)





I think that would be a change in semantics, which we should definitely
not be getting. Avoiding a change in semantics might be an interesting
exercise, but we have lots of clever coders ...



nextval is a volatile function, and in my understanding the plan was not 
to inline CTEs with volatile functions (or CTEs doing writes etc.). That 
is, we'd guarantee the same results as we get now.


regards

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


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] CTE inlining

[Joe Conway]

On 05/04/2017 10:56 AM, Andrew Dunstan wrote:
> 
> 
> On 05/04/2017 01:52 PM, Joe Conway wrote:
>> On 05/04/2017 10:33 AM, Alvaro Herrera wrote:
>>> I'm not sure what your point is.  We know that for some cases the
>>> optimization barrier semantics are useful, which is why the proposal is
>>> to add a keyword to install one explicitely:
>>>
>>>  with materialized r as
>>>  (
>>> select json_populate_record(null::mytype, myjson) as x
>>> from mytable
>>>  )
>>>  select (x).*
>>>  from r;
>>>
>>> this would preserve the current semantics.
>> I haven't been able to follow this incredibly long thread, so please
>> excuse me if way off base, but are we talking about that a CTE would be
>> silently be rewritten as an inline expression potentially unless it is
>> decorated with some new syntax?
>>
>> I would find that very disconcerting myself. For example, would this CTE
>> potentially get rewritten with multiple evaluation as follows?
>>
>> DROP SEQUENCE IF EXISTS foo_seq;
>> CREATE SEQUENCE foo_seq;
>>
>> WITH a(f1) AS (SELECT nextval('foo_seq'))
>> SELECT a.f1, a.f1 FROM a;
>>  f1 | ?column?
>> +--
>>   1 |1
>> (1 row)
>>
>> ALTER SEQUENCE foo_seq RESTART;
>> SELECT nextval('foo_seq'), nextval('foo_seq');
>>  nextval | ?column?
>> -+--
>>1 |2
>> (1 row)
>>
> 
> I think that would be a change in semantics, which we should definitely
> not be getting. Avoiding a change in semantics might be an interesting
> exercise, but we have lots of clever coders ...

Well I think my point is that I always have understood CTEs to be
executed precisely once producing a temporary result set that is then
referenced elsewhere. I don't think that property of CTEs should change.
Somewhere else in the thread someone mentioned predicate push down --
that makes sense and maybe some clever coder can come up with a patch
that does that, but I would not be in favor of CTEs being inlined and
therefore evaluated multiple times.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] json_agg produces nonstandard json

[Tom Lane]

Jordan Deitch  writes:
> A json(b)_agg() will produce the following result when no results are
> passed in:
> "[null]"
> per:
> select jsonb_agg((select 1 where false));

Looks fine to me.

> I believe, generally speaking, '[]' would be the more appropriate output.

Why?  What you gave it was one null value.  An empty array result would
imply that there were zero inputs, which is wrong.

Perhaps you're confused about the way scalar sub-selects work?  The
above is equivalent to "select jsonb_agg(null::integer)"; it's not
the same as

# select jsonb_agg(1) where false;
 jsonb_agg 
---
 
(1 row)

Now you could legitimately argue that this case, where there are zero
input rows, should produce '[]' rather than a SQL null.  But I think we
had that discussion already, and agreed that this behavior is more in
keeping with the behavior of SQL's standard aggregates, notably SUM().
You can use coalesce() to inject '[]' (or whatever result you want) for
the no-rows case:

# select coalesce(jsonb_agg(1), '[]') where false;
 coalesce 
--
 []
(1 row)

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Reducing runtime of stats regression test

[Tom Lane]

Robert Haas  writes:
> On Thu, May 4, 2017 at 10:22 AM, Tom Lane  wrote:
>> Yes, but that would be getting into the realm of new features, not
>> post-feature-freeze test adjustments.  It certainly couldn't be
>> a candidate for back-patching.

> I'm not sure there's some bright line between adding a new
> SQL-callable function to cut down the test time and any other
> tinkering we might do to reduce the regression test time.  I think
> there's a pretty good argument that all of the recent changes you made
> in this area constitute strictly optional tinkering.  I'm haven't been
> objecting because they don't seem likely to destabilize anything, but
> I don't see that they're really helping us get ready for beta either,
> which is presumably what we ought to be focusing on at this point.

Well, to my mind, making the regression tests faster is something that
could be quite helpful during beta, because a lot of people will be
running them.  (Or so we hope, at least.)  If the fact that e.g.
the recovery tests take a lot of time discourages people from running
them, that can't be a good thing for beta purposes.  So I respectfully
reject your opinion about what I should be working on.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] CTE inlining

[Tom Lane]

Alvaro Herrera  writes:
> I'm not sure what your point is.  We know that for some cases the
> optimization barrier semantics are useful, which is why the proposal is
> to add a keyword to install one explicitely:

>  with materialized r as
>  (
> select json_populate_record(null::mytype, myjson) as x
> from mytable
>  )
>  select (x).*
>  from r;

> this would preserve the current semantics.

Uh, no, it wouldn't, until you had changed your query (and made it
non-SQL-compliant, and unable to work at all on pre-v11 PG).
This might be a good design, but surely it does not provide backwards
compatibility.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] CTE inlining

[Tomas Vondra]

On 5/4/17 8:03 PM, Joe Conway wrote:

On 05/04/2017 10:56 AM, Andrew Dunstan wrote:



On 05/04/2017 01:52 PM, Joe Conway wrote:

On 05/04/2017 10:33 AM, Alvaro Herrera wrote:

I'm not sure what your point is.  We know that for some cases the
optimization barrier semantics are useful, which is why the proposal is
to add a keyword to install one explicitely:

  with materialized r as
  (
 select json_populate_record(null::mytype, myjson) as x
 from mytable
  )
  select (x).*
  from r;

this would preserve the current semantics.

I haven't been able to follow this incredibly long thread, so please
excuse me if way off base, but are we talking about that a CTE would be
silently be rewritten as an inline expression potentially unless it is
decorated with some new syntax?

I would find that very disconcerting myself. For example, would this CTE
potentially get rewritten with multiple evaluation as follows?

DROP SEQUENCE IF EXISTS foo_seq;
CREATE SEQUENCE foo_seq;

WITH a(f1) AS (SELECT nextval('foo_seq'))
SELECT a.f1, a.f1 FROM a;
  f1 | ?column?
+--
   1 |1
(1 row)

ALTER SEQUENCE foo_seq RESTART;
SELECT nextval('foo_seq'), nextval('foo_seq');
  nextval | ?column?
-+--
1 |2
(1 row)



I think that would be a change in semantics, which we should definitely
not be getting. Avoiding a change in semantics might be an interesting
exercise, but we have lots of clever coders ...


Well I think my point is that I always have understood CTEs to be
executed precisely once producing a temporary result set that is then
referenced elsewhere. I don't think that property of CTEs should change.
Somewhere else in the thread someone mentioned predicate push down --
that makes sense and maybe some clever coder can come up with a patch
that does that, but I would not be in favor of CTEs being inlined and
therefore evaluated multiple times.



I agree with this, but there's a difference between "executed exactly 
once" and "producing the same result as if executed exactly once".


I may be misunderstanding what other people proposed in this thread, but 
I think the plan was to only inline CTEs where we know it won't change 
the results, etc. So e.g. CTEs with volatile functions would not get 
inlined, which includes nextval() for example.


regards

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


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] Potential issue with alter system

[Joshua D. Drake]

Folks,

So I did this:

postgres=# alter system set archive_command to 'rsynv -av %p 
postgres@52.3.141.224:/data/archive/%f

';

Note the new line. It properly created in postgresql.auto.conf:

archive_command = 'rsynv -av %p postgres@52.3.141.224:/data/archive/%f
'
(note the new line)

I noticed I spelled rsync wrong and now I get this:

postgres=# alter system set archive_command to 'rsync -av %p 
postgres@52.3.141.224:/data/archive/%f'

;
ERROR:  could not parse contents of file "postgresql.auto.conf"

This is 9.5.2 (Yes, I know... I am in the process of setting up 
replication to 9.5.6 for failover).


JD

--
Command Prompt, Inc.  http://the.postgres.company/
+1-503-667-4564
PostgreSQL Centered full stack support, consulting and development.
Everyone appreciates your honesty, until you are honest with them.
Unless otherwise stated, opinions are my own.


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] Fix freeing of dangling IndexScanDesc.xs_hitup in GiST

[Nikita Glukhov]

Hello, hackers!

The last query in the following script crashes Postgres:

create table t (id serial, amount int);
insert into t (amount) select random() * 1000 from generate_series(1, 100);
create extension btree_gist;
create index t_gist_idx on t using gist(id, amount);

select p.id, p.amount, s.nearest
from t as p left join lateral
(
  select p.id, array_agg(l.id) as nearest from (
select id from t order by amount <-> p.amount limit 10
  ) l
) s using(id);

In gistrescan() IndexScanDesc.xs_hitup is not reset after MemoryContextReset() 
of
so->queueCxt in which xs_hitup was allocated, then getNextNearest() tries to 
pfree()
dangling xs_hitup, which results in the reuse of this pointer and the 
subsequent crash.

Attached patches fix this bug introduced in commit
d04c8ed9044eccebce043143a930617e3998c005 "Add support for index-only scans in 
GiST".
The bug is present in v9.5, v9.6, v10.0.

--
Nikita Glukhov
Postgres Professional:http://www.postgrespro.com
The Russian Postgres Company

diff --git a/src/backend/access/gist/gistscan.c b/src/backend/access/gist/gistscan.c
index 2526a39..580b6ac 100644
--- a/src/backend/access/gist/gistscan.c
+++ b/src/backend/access/gist/gistscan.c
@@ -148,6 +148,11 @@ gistrescan(IndexScanDesc scan, ScanKey key, int nkeys,
 		/* third or later time through */
 		MemoryContextReset(so->queueCxt);
 		first_time = false;
+		/*
+		 * scan->xs_itup is allocated in so->queueCxt and now it is invalid,
+		 * so we need to reset it to prevent it from freeing in getNextNearest().
+		 */
+		scan->xs_itup = NULL;
 	}
 
 	/*
diff --git a/src/backend/access/gist/gistscan.c b/src/backend/access/gist/gistscan.c
index 81ff8fc..5e10ada 100644
--- a/src/backend/access/gist/gistscan.c
+++ b/src/backend/access/gist/gistscan.c
@@ -148,6 +148,11 @@ gistrescan(IndexScanDesc scan, ScanKey key, int nkeys,
 		/* third or later time through */
 		MemoryContextReset(so->queueCxt);
 		first_time = false;
+		/*
+		 * scan->xs_hitup is allocated in so->queueCxt and now it is invalid,
+		 * so we need to reset it to prevent it from freeing in getNextNearest().
+		 */
+		scan->xs_hitup = NULL;
 	}
 
 	/*

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WITH clause in CREATE STATISTICS

[Alvaro Herrera]

Here's a patch implementing this idea.  From gram.y's comment, the
support syntax is now:

  /*
   *
   *QUERY :
!  *CREATE STATISTICS stats_name [(stat types)] arguments
! 
!  *where 'arguments' can be one or more of:
!  *{ ON (columns)
!  *  | FROM relations
!  *  | WITH (options)
!  *  | WHERE expression }

Note that I removed the USING keyword in the stat types list, and also
made it mandatory that that list appears immediately after the new stats
name.  This should make it possible to have USING in the relation list
(the FROM clause), if we allow explicit multiple relations with join
syntax there.  The other options can appear in any order.

Also, both WITH and WHERE are accepted by the grammar, but immediately
throw "feature not implemented" error at parse time.

I was on the fence about adding copy/equal/out support for the new
StatisticArgument node; it seems pointless because that node does not
leave gram.y anyway.

Unless there are objections, I'll push this tomorrow.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
*** a/doc/src/sgml/perform.sgml
--- b/doc/src/sgml/perform.sgml
***
*** 1132,1138  WHERE tablename = 'road';
   To inspect functional dependencies on a statistics
   stts, you may do this:
  
! CREATE STATISTICS stts WITH (dependencies)
 ON (zip, city) FROM zipcodes;
  ANALYZE zipcodes;
  SELECT stxname, stxkeys, stxdependencies
--- 1132,1138 
   To inspect functional dependencies on a statistics
   stts, you may do this:
  
! CREATE STATISTICS stts (dependencies)
 ON (zip, city) FROM zipcodes;
  ANALYZE zipcodes;
  SELECT stxname, stxkeys, stxdependencies
***
*** 1219,1225  EXPLAIN (ANALYZE, TIMING OFF) SELECT * FROM t WHERE a = 1 
AND b = 10;
   Continuing the above example, the n-distinct coefficients in a ZIP
   code table may look like the following:
  
! CREATE STATISTICS stts2 WITH (ndistinct)
 ON (zip, state, city) FROM zipcodes;
  ANALYZE zipcodes;
  SELECT stxkeys AS k, stxndistinct AS nd
--- 1219,1225 
   Continuing the above example, the n-distinct coefficients in a ZIP
   code table may look like the following:
  
! CREATE STATISTICS stts2 (ndistinct)
 ON (zip, state, city) FROM zipcodes;
  ANALYZE zipcodes;
  SELECT stxkeys AS k, stxndistinct AS nd
*** a/doc/src/sgml/planstats.sgml
--- b/doc/src/sgml/planstats.sgml
***
*** 526,532  EXPLAIN (ANALYZE, TIMING OFF) SELECT * FROM t WHERE a = 1 AND 
b = 1;
  multivariate statistics on the two columns:
  
  
! CREATE STATISTICS stts WITH (dependencies) ON (a, b) FROM t;
  ANALYZE t;
  EXPLAIN (ANALYZE, TIMING OFF) SELECT * FROM t WHERE a = 1 AND b = 1;
QUERY PLAN  
 
--- 526,532 
  multivariate statistics on the two columns:
  
  
! CREATE STATISTICS stts (dependencies) ON (a, b) FROM t;
  ANALYZE t;
  EXPLAIN (ANALYZE, TIMING OFF) SELECT * FROM t WHERE a = 1 AND b = 1;
QUERY PLAN  
 
***
*** 569,575  EXPLAIN (ANALYZE, TIMING OFF) SELECT COUNT(*) FROM t GROUP BY 
a, b;
  calculation, the estimate is much improved:
  
  DROP STATISTICS stts;
! CREATE STATISTICS stts WITH (dependencies, ndistinct) ON (a, b) FROM t;
  ANALYZE t;
  EXPLAIN (ANALYZE, TIMING OFF) SELECT COUNT(*) FROM t GROUP BY a, b;
 QUERY PLAN 
   
--- 569,575 
  calculation, the estimate is much improved:
  
  DROP STATISTICS stts;
! CREATE STATISTICS stts (dependencies, ndistinct) ON (a, b) FROM t;
  ANALYZE t;
  EXPLAIN (ANALYZE, TIMING OFF) SELECT COUNT(*) FROM t GROUP BY a, b;
 QUERY PLAN 
   
*** a/doc/src/sgml/ref/create_statistics.sgml
--- b/doc/src/sgml/ref/create_statistics.sgml
***
*** 22,28  PostgreSQL documentation
   
  
  CREATE STATISTICS [ IF NOT EXISTS ] statistics_name
! WITH ( option [= 
value] [, ... ] )
  ON ( column_name, 
column_name [, ...])
  FROM table_name
  
--- 22,28 
   
  
  CREATE STATISTICS [ IF NOT EXISTS ] statistics_name
! [ ( statistic_type [, ... ] 
) ]
  ON ( column_name, 
column_name [, ...])
  FROM table_name
  
***
*** 75,80  CREATE STATISTICS [ IF NOT EXISTS ] statistics_na
--- 75,93 
 
  
 
+ statistic_type
+ 
+  
+   A statistic type to be enabled for this statistics.  Currently
+   supported types are ndistinct, which enables
+   n-distinct coefficient tracking,
+   and dependen

Re: [HACKERS] json_agg produces nonstandard json

[Jordan Deitch]

Thank you for responding!

Good points.

However, I don't see consistency between the results of these two
statements:

select jsonb_agg((select 1 where false));
select sum((select 1 where false));

Therefore another option I would like to suggest is returning the same null
value-types for the sum() and json_agg().

So the select jsonb_agg((select 1 where false)); would return null as
opposed to [null]. In this case it would be compatible with coalesce()

---
Thanks
Jordan Deitch

Re: [HACKERS] CTE inlining

[Tom Lane]

Tomas Vondra  writes:
> On 5/4/17 8:03 PM, Joe Conway wrote:
>>> I haven't been able to follow this incredibly long thread, so please
>>> excuse me if way off base, but are we talking about that a CTE would be
>>> silently be rewritten as an inline expression potentially unless it is
>>> decorated with some new syntax?

> I agree with this, but there's a difference between "executed exactly 
> once" and "producing the same result as if executed exactly once".

> I may be misunderstanding what other people proposed in this thread, but 
> I think the plan was to only inline CTEs where we know it won't change 
> the results, etc. So e.g. CTEs with volatile functions would not get 
> inlined, which includes nextval() for example.

I haven't been keeping close tabs either, but surely we still have to have
the optimization fence in (at least) all these cases:

* CTE contains INSERT/UPDATE/DELETE
* CTE contains SELECT FOR UPDATE/SHARE (else the set of rows that get
  locked might change)
* CTE contains volatile functions

I'm willing to write off cases where, eg, a function should have been
marked volatile and was not.  That's user error and there are plenty
of hazards of that kind already.  But if the optimizer has reason
to know that discarding the fence might change any query side-effects,
it mustn't.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] logical replication syntax (was DROP SUBSCRIPTION, query cancellations and slot handling)

[Robert Haas]

On Wed, May 3, 2017 at 12:38 AM, Petr Jelinek
 wrote:
> Ok, Let me be clear, I actually happen to agree with your proposal. The
> reason I am moaning is that I always seem to find myself doing tons of
> mechanical work to rewrite some cosmetic aspect of some patch based on
> which committer is paying attention in a specific week. So while I am
> for doing exactly what you proposed, I'd like to see couple of +1s first
> (Peter?) since I don't want to rewrite it to something different again
> and it's also long past freeze.

So, Tom Lane and Thom Brown and Josh Drake all seemed generally in
favor of cleaning this up.  Perhaps they could opine on this
particular proposal.

> To repeat the proposal:
> - change the WITH (...) clauses in subscription/publication commands to:
> (create_slot true/false, connect true/false, slot_name 'something',
> copy_data true/false, etc)
>
> - change the NOREFRESH to NO REFRESH in ALTER SUBSCRIPTION name SET
> PUBLICATION (btw I originally had SKIP REFRESH there but changed it to
> NOREFRESH for consistency with the other NO* stuff, wonder if SKIP would
> sound more english).
>
> - change the (publish insert/nopublish insert/publish update/nopublish
> update), etc options to (publish 'update,insert').
>
> And one question, if we are not using the definitions (key = value)
> should we keep the WITH keyword in the syntax, would it be confusing?

No opinion on that.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] CTE inlining

[Serge Rielau]

I haven't been keeping close tabs either, but surely we still have to have
the optimization fence in (at least) all these cases:

* CTE contains INSERT/UPDATE/DELETE
* CTE contains SELECT FOR UPDATE/SHARE (else the set of rows that get
locked might change)
* CTE contains volatile functions

I'm willing to write off cases where, eg, a function should have been
marked volatile and was not. That's user error and there are plenty
of hazards of that kind already. But if the optimizer has reason
to know that discarding the fence might change any query side-effects,
it mustn't. Yes! +100
Cheers Serge

Re: [HACKERS] json_agg produces nonstandard json

[Tom Lane]

Jordan Deitch  writes:
> However, I don't see consistency between the results of these two
> statements:

> select jsonb_agg((select 1 where false));
> select sum((select 1 where false));

Well, SUM() is defined to ignore null input values, which is not too
surprising as it couldn't do anything very useful with them.  So it ends
up deciding there are no input rows.  jsonb_agg() is defined to translate
null input values to JSON "null", which seems like a sane behavior to me
although I agree that they aren't exactly the same concept.
If you don't want that, you could suppress the null inputs with a FILTER
clause:

regression=# select jsonb_agg(x) from (values (1),(2),(null),(4)) v(x);
jsonb_agg
-
 [1, 2, null, 4]
(1 row)

regression=# select jsonb_agg(x) filter (where x is not null) from (values 
(1),(2),(null),(4)) v(x);
 jsonb_agg 
---
 [1, 2, 4]
(1 row)

regression=# select jsonb_agg(x) filter (where x is not null) from (values 
(null),(null),(null)) v(x);
 jsonb_agg 
---
 
(1 row)

We could perhaps invent a "jsonb_agg_strict()" variant that skips
nulls for you.  But I'd want to see multiple requests before
concluding that it was worth carrying such a function.  The FILTER
workaround seems good enough if it's an infrequent need.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Fix freeing of dangling IndexScanDesc.xs_hitup in GiST

[Tom Lane]

Nikita Glukhov  writes:
> In gistrescan() IndexScanDesc.xs_hitup is not reset after 
> MemoryContextReset() of
> so->queueCxt in which xs_hitup was allocated, then getNextNearest() tries to 
> pfree()
> dangling xs_hitup, which results in the reuse of this pointer and the 
> subsequent crash.

Right.  I already did something about this, about an hour ago --- a
bit differently from your patch, but same idea.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Potential issue with alter system

[Tom Lane]

"Joshua D. Drake"  writes:
> So I did this:

> postgres=# alter system set archive_command to 'rsynv -av %p 
> postgres@52.3.141.224:/data/archive/%f
> ';

> Note the new line. It properly created in postgresql.auto.conf:

> archive_command = 'rsynv -av %p postgres@52.3.141.224:/data/archive/%f
> '
> (note the new line)

Ugh.  That's broken --- we don't support newlines within values in
postgresql.conf files.

[ pokes around ... ]  HEAD seems to reject this properly:

regression=# alter system set archive_command to 'rsynv -av %p 
postgres@52.3.141.224:/data/archive/%f
regression'# ';
ERROR:  parameter value for ALTER SYSTEM must not contain a newline

> This is 9.5.2 (Yes, I know... I am in the process of setting up 
> replication to 9.5.6 for failover).

Ah.  [ digs further... ]  Yup, we fixed that in 9.5.3:

Author: Tom Lane 
Branch: master Release: REL9_6_BR [99f3b5613] 2016-04-04 18:05:23 -0400
Branch: REL9_5_STABLE Release: REL9_5_3 [f3d17491c] 2016-04-04 18:05:23 -0400
Branch: REL9_4_STABLE Release: REL9_4_8 [28148e258] 2016-04-04 18:05:24 -0400

Disallow newlines in parameter values to be set in ALTER SYSTEM.

As noted by Julian Schauder in bug #14063, the configuration-file parser
doesn't support embedded newlines in string literals.  While there might
someday be a good reason to remove that restriction, there doesn't seem
to be one right now.  However, ALTER SYSTEM SET could accept strings
containing newlines, since many of the variable-specific value-checking
routines would just see a newline as whitespace.  This led to writing a
postgresql.auto.conf file that was broken and had to be removed manually.

Pending a reason to work harder, just throw an error if someone tries this.

In passing, fix several places in the ALTER SYSTEM logic that failed to
provide an errcode() for an ereport(), and thus would falsely log the
failure as an internal XX000 error.

Back-patch to 9.4 where ALTER SYSTEM was introduced.


If you have other entries you want to keep in the postgresql.auto.conf
file, you could get away with manually editing it to remove the newline.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Fix freeing of dangling IndexScanDesc.xs_hitup in GiST

[Nikita Glukhov]

On 04.05.2017 22:16, Tom Lane wrote:


Nikita Glukhov  writes:

In gistrescan() IndexScanDesc.xs_hitup is not reset after MemoryContextReset() 
of
so->queueCxt in which xs_hitup was allocated, then getNextNearest() tries to 
pfree()
dangling xs_hitup, which results in the reuse of this pointer and the 
subsequent crash.

Right.  I already did something about this, about an hour ago --- a
bit differently from your patch, but same idea.

regards, tom lane


Sorry that I'm not monitoring pgsql-bugs.

It might be interesting that I found this bug back in July 2016 when I
was experimenting with my KNN-btree implementation, but I failed to report
it because I could reproduce it only manually by a calling in a loop
gistrescan() and gistgettuple().

--
Nikita Glukhov
Postgres Professional:http://www.postgrespro.com
The Russian Postgres Company



--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] what's up with IDENTIFIER_LOOKUP_EXPR?

[Robert Haas]

plpgsql has an enum called IdentifierLookup which includes a value
IDENTIFIER_LOOKUP_EXPR which is declared like this:

IDENTIFIER_LOOKUP_EXPR  /* In SQL expression --- special case */

It regrettably does not explain what exactly is special about it, and
AFAICT, neither does any other comment.  If I replace every usage of
IDENTIFIER_LOOKUP_EXPR with IDENTIFIER_LOOKUP_NORMAL, the regression
tests pass nonetheless.  It was introduced by
01f7d29902cb27fb698e5078d72cb837398e074c, committed by Tom in 2010:

commit 01f7d29902cb27fb698e5078d72cb837398e074c
Author: Tom Lane 
Date:   Sun Jan 10 17:15:18 2010 +

Improve plpgsql's handling of record field references by forcing
all potential
field references in SQL expressions to have RECFIELD datum-array entries at
parse time.  If it turns out that the reference is actually to a SQL column,
the RECFIELD entry is useless, but it costs little.  This allows
us to get rid
of the previous use of FieldSelect applied to a whole-row Param
for the record
variable; which was not only slower than a direct RECFIELD reference, but
failed for references to system columns of a trigger's NEW or OLD record.
Per report and fix suggestion from Dean Rasheed.

The rule, as far as I can tell from reading the code, is that
IDENTIFIER_LOOKUP_NORMAL looks up words, double-words (e.g. x.y), and
triple-words (e.g x.y.z), IDENTIFIER_LOOKUP_EXPR looks up only
double-words and triple-words, and IDENTIFIER_LOOKUP_DECLARE looks up
nothing.  But it's not clear to me exactly what the motivation for
that is. plpgsql_parse_word says:

/*
 * We should do nothing in DECLARE sections.  In SQL expressions, there's
 * no need to do anything either --- lookup will happen when the
 * expression is compiled.
 */

...but that doesn't really explain why we go out of our way to have
this third state, because the wording implies that it doesn't
particularly matter one way or the other.

Any help appreciated.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Adding support for Default partition in partitioning

[Jeevan Ladhe]

Hi Rahila,

I have started reviewing your latest patch, and here are my initial
comments:

1.
In following block, we can just do with def_index, and we do not need
found_def
flag. We can check if def_index is -1 or not to decide if default partition
is
present.

@@ -166,6 +172,8 @@ RelationBuildPartitionDesc(Relation rel)
  /* List partitioning specific */
  PartitionListValue **all_values = NULL;
  bool found_null = false;
+ bool found_def = false;
+ int def_index = -1;
  int null_index = -1;

2.
In check_new_partition_bound, in case of PARTITION_STRATEGY_LIST, remove
following duplicate declaration of boundinfo, because it is confusing and
after
your changes it is not needed as its not getting overridden in the if block
locally.
if (partdesc->nparts > 0)
{
PartitionBoundInfo boundinfo = partdesc->boundinfo;
ListCell   *cell;


3.
In following function isDefaultPartitionBound, first statement "return
false"
is not needed.

+ * Returns true if the partition bound is default
+ */
+bool
+isDefaultPartitionBound(Node *value)
+{
+ if (IsA(value, DefElem))
+ {
+ DefElem *defvalue = (DefElem *) value;
+ if(!strcmp(defvalue->defname, "DEFAULT"))
+ return true;
+ return false;
+ }
+ return false;
+}

4.
As mentioned in my previous set of comments, following if block inside a
loop
in get_qual_for_default needs a break:

+ foreach(cell1, bspec->listdatums)
+ {
+ Node *value = lfirst(cell1);
+ if (isDefaultPartitionBound(value))
+ {
+ def_elem = true;
+ *defid  = inhrelid;
+ }
+ }

5.
In the grammar the rule default_part_list is not needed:

+default_partition:
+ DEFAULT  { $$ = (Node *)makeDefElem("DEFAULT", NULL, @1); }
+
+default_part_list:
+ default_partition { $$ = list_make1($1); }
+ ;
+

Instead you can simply declare default_partition as a list and write it as:

default_partition:
DEFAULT
{
Node *def = (Node *)makeDefElem("DEFAULT", NULL, @1);
$$ = list_make1(def);
}

6.
You need to change the output of the describe command, which is currently
as below: postgres=# \d+ test; Table "public.test" Column | Type |
Collation | Nullable | Default | Storage | Stats target | Description
+-+---+--+-+-+--+-
a | integer | | | | plain | | b | date | | | | plain | | Partition key:
LIST (a) Partitions: pd FOR VALUES IN (DEFAULT), test_p1 FOR VALUES IN (4,
5) What about changing the Paritions output as below: Partitions: *pd
DEFAULT,* test_p1 FOR VALUES IN (4, 5)

7.
You need to handle tab completion for DEFAULT.
e.g.
If I partially type following command:
CREATE TABLE pd PARTITION OF test DEFA
and then press tab, I get following completion:
CREATE TABLE pd PARTITION OF test FOR VALUES

I did some primary testing and did not find any problem so far.

I will review and test further and let you know my comments.

Regards,
Jeevan Ladhe

On Thu, May 4, 2017 at 6:09 PM, Rajkumar Raghuwanshi <
rajkumar.raghuwan...@enterprisedb.com> wrote:

> On Thu, May 4, 2017 at 5:14 PM, Rahila Syed 
> wrote:
>
>> The syntax implemented in this patch is as follows,
>>
>> CREATE TABLE p11 PARTITION OF p1 DEFAULT;
>>
>> Applied v9 patches, table description still showing old pattern of
> default partition. Is it expected?
>
> create table lpd (a int, b int, c varchar) partition by list(a);
> create table lpd_d partition of lpd DEFAULT;
>
> \d+ lpd
>  Table "public.lpd"
>  Column |   Type| Collation | Nullable | Default | Storage  |
> Stats target | Description
> +---+---+--+
> -+--+--+-
>  a  | integer   |   |  | | plain
> |  |
>  b  | integer   |   |  | | plain
> |  |
>  c  | character varying |   |  | | extended
> |  |
> Partition key: LIST (a)
> Partitions: lpd_d FOR VALUES IN (DEFAULT)
>

Re: [HACKERS] what's up with IDENTIFIER_LOOKUP_EXPR?

[Tom Lane]

Robert Haas  writes:
> plpgsql has an enum called IdentifierLookup which includes a value
> IDENTIFIER_LOOKUP_EXPR which is declared like this:
> IDENTIFIER_LOOKUP_EXPR  /* In SQL expression --- special case 
> */
> It regrettably does not explain what exactly is special about it, and
> AFAICT, neither does any other comment.  If I replace every usage of
> IDENTIFIER_LOOKUP_EXPR with IDENTIFIER_LOOKUP_NORMAL, the regression
> tests pass nonetheless.

AFAICS, the only place where IDENTIFIER_LOOKUP_EXPR acts differently from
IDENTIFIER_LOOKUP_NORMAL is plpgsql_parse_word(), and what it does is to
prevent a lookup in plpgsql's namestack from occurring, so that an
identifier is reported as "not recognized" even if there is a matching
variable.  In the two places that currently select IDENTIFIER_LOOKUP_EXPR,
that seems to be only an optimization, because they will act the same
anyway for all the token types that plpgsql_parse_word could return.
I don't remember if there originally was a bigger reason than that.
But it seems reasonable to be able to signal the lookup machinery that
we're in a SQL expression rather than someplace else; in principle that
could have larger implications than it does right now.

> The rule, as far as I can tell from reading the code, is that
> IDENTIFIER_LOOKUP_NORMAL looks up words, double-words (e.g. x.y), and
> triple-words (e.g x.y.z), IDENTIFIER_LOOKUP_EXPR looks up only
> double-words and triple-words, and IDENTIFIER_LOOKUP_DECLARE looks up
> nothing.  But it's not clear to me exactly what the motivation for
> that is. plpgsql_parse_word says:

The doubleword and tripleword cases are slightly different: lookup can't
be turned off completely, because we need to create matching RECFIELD
datums for use later.  It's still true that we don't especially care what
token is returned, but the side-effect of creating a RECFIELD entry is
important.  Possibly we could shave a few more cycles by making the code
know that explicitly, but it didn't seem worth the complexity at the time.

Feel free to improve the comments if this bugs you ...

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] delta relations in AFTER triggers

[Thomas Munro]

On Fri, May 5, 2017 at 12:39 AM, Neha Sharma
 wrote:
> While testing the feature we encountered one more crash,below is the
> scenario to reproduce.
>
> create table t1 ( a int);
> create table t2 ( a int);
> insert into t1 values (11),(12),(13);
>
> create or replace function my_trig() returns trigger
> language plpgsql as $my_trig$
> begin
> insert into t2(select a from new_table);
> RETURN NEW;
> end;
> $my_trig$;
>
> create trigger my_trigger
> after truncate or update  on t1
> referencing new table as new_table old table as oldtab
> for each statement
> execute procedure my_trig();
>
> truncate t1;
> server closed the connection unexpectedly
> This probably means the server terminated abnormally
> before or while processing the request.
> The connection to the server was lost. Attempting reset: Failed.

Thanks.  Reproduced here.  The stack looks like this:

frame #3: 0x000103e5e8b0
postgres`ExceptionalCondition(conditionName="!((trigdata->tg_event)
& 0x0003) == 0x) || (((trigdata->tg_event) & 0x0003)
== 0x0002) || (((trigdata->tg_event) & 0x0003) == 0x0001))
&& (((trigdata->tg_event) & 0x0018) == 0x) &&
!(trigdata->tg_event & 0x0020) && !(trigdata->tg_event &
0x0040)) || (trigdata->tg_oldtable == ((void*)0) &&
trigdata->tg_newtable == ((void*)0)))", errorType="FailedAssertion",
fileName="trigger.c", lineNumber=2045) + 128 at assert.c:54
frame #4: 0x000103a6f542
postgres`ExecCallTriggerFunc(trigdata=0x7fff5c40bad0, tgindx=0,
finfo=0x7fd8ba0817b8, instr=0x,
per_tuple_context=0x7fd8b906f928) + 258 at trigger.c:2039
frame #5: 0x000103a754ed
postgres`AfterTriggerExecute(event=0x7fd8ba092460,
rel=0x0001043fd9c0, trigdesc=0x7fd8ba068758,
finfo=0x7fd8ba0817b8, instr=0x,
per_tuple_context=0x7fd8b906f928,
trig_tuple_slot1=0x,
trig_tuple_slot2=0x) + 1469 at trigger.c:3860
frame #6: 0x000103a73080
postgres`afterTriggerInvokeEvents(events=0x7fd8ba07fb00,
firing_id=1, estate=0x7fd8ba090440, delete_ok='\x01') + 592 at
trigger.c:4051
frame #7: 0x000103a72b7b
postgres`AfterTriggerEndQuery(estate=0x7fd8ba090440) + 203 at
trigger.c:4227
frame #8: 0x000103a498aa
postgres`ExecuteTruncate(stmt=0x7fd8ba059f40) + 2026 at
tablecmds.c:1485

There's an assertion that it's (one of INSERT, UPDATE, DELETE, an
AFTER trigger, not deferred) *or* there are no transition tables.
Here it's TRUNCATE and there are transition tables, so it fails:

/*
 * Protect against code paths that may fail to initialize transition table
 * info.
 */
Assert(((TRIGGER_FIRED_BY_INSERT(trigdata->tg_event) ||
 TRIGGER_FIRED_BY_UPDATE(trigdata->tg_event) ||
 TRIGGER_FIRED_BY_DELETE(trigdata->tg_event)) &&
TRIGGER_FIRED_AFTER(trigdata->tg_event) &&
!(trigdata->tg_event & AFTER_TRIGGER_DEFERRABLE) &&
!(trigdata->tg_event & AFTER_TRIGGER_INITDEFERRED)) ||
   (trigdata->tg_oldtable == NULL && trigdata->tg_newtable == NULL));


We can't possibly support transition tables on TRUNCATE (the whole
point of TRUNCATE is not to inspect all the rows so we can't collect
them), and we already reject ROW triggers on TRUNCATE, so we should
reject transition tables on STATEMENT triggers for TRUNCATE at
creation time too.  See attached.  Thoughts?

> Log file and core dump attached for reference.

Thanks!  Just by the way, it'd be better to post just an interesting
stack trace fragment rather than a core file, because core files can't
really be used without the exact executable that you built.

-- 
Thomas Munro
http://www.enterprisedb.com


no-transition-tables-for-truncate.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Adding support for Default partition in partitioning

[Jeevan Ladhe]

While reviewing the code I was trying to explore more cases, and I here
comes an
open question to my mind:
should we allow the default partition table to be partitioned further?

If we allow it(as in the current case) then observe following case, where I
have defined a default partitioned which is further partitioned on a
different
column.

postgres=# CREATE TABLE test ( a int, b int, c int) PARTITION BY LIST (a);
CREATE TABLE
postgres=# CREATE TABLE test_p1 PARTITION OF test FOR VALUES IN(4, 5, 6, 7,
8);
CREATE TABLE
postgres=# CREATE TABLE test_pd PARTITION OF test DEFAULT PARTITION BY
LIST(b);
CREATE TABLE
postgres=# INSERT INTO test VALUES (20, 24, 12);
ERROR:  no partition of relation "test_pd" found for row
DETAIL:  Partition key of the failing row contains (b) = (24).

Note, that it does not allow inserting the tuple(20, 24, 12) because though
a=20
would fall in default partition i.e. test_pd, table test_pd itself is
further
partitioned and does not have any partition satisfying b=24.
Further if I define a default partition for table test_pd, the the tuple
gets inserted.

Doesn't this sound like the whole purpose of having DEFAULT partition on
test
table is defeated?

Any views?

Regards,
Jeevan Ladhe

Re: [HACKERS] transition table behavior with inheritance appears broken (was: Declarative partitioning - another take)

[Thomas Munro]

On Fri, May 5, 2017 at 2:40 AM, Robert Haas  wrote:
> On Thu, May 4, 2017 at 4:46 AM, Thomas Munro
>  wrote:
>> On Thu, May 4, 2017 at 4:02 AM, Alvaro Herrera  
>> wrote:
>>> Robert Haas wrote:
 I suspect that most users would find it more useful to capture all of
 the rows that the statement actually touched, regardless of whether
 they hit the named table or an inheritance child.
>>>
>>> Yes, agreed.  For the plain inheritance cases each row would need to
>>> have an indicator of which relation it comes from (tableoid); I'm not
>>> sure if such a thing would be useful in the partitioning case.
>>
>> On Thu, May 4, 2017 at 4:26 AM, David Fetter  wrote:
>>> +1 on the not-duct-tape view of partitioned tables.
>>
>> Hmm.  Ok.  Are we talking about PG10 or PG11 here?  Does this approach
>> makes sense?
>
> I was thinking PG10 if it can be done straightforwardly.

Ok, I will draft a patch to do it the way I described and see what people think.

>> To avoid the whiff of duct tape, we'd probably also want to make ROW
>> triggers created on the partitioned table(s) containing partition to
>> fire too, with appropriate TypeConversionMap treatment.  Not sure what
>> exactly is involved there.
>>
>> On the other hand, doing that with inheritance hierarchies would be an
>> incompatible behavioural change, which I guess people don't want -- am
>> I right?
>
> Incompatible with what?  Transition tables haven't been released yet.
> If we're going to fix the definition of what they do, now's the time.

The last two paragraphs of my email were about ROW triggers created on
partitioned tables and tables with inheritance children, not the new
transition table stuff.  I will forget that for now and look only at
making the transition tables duct-tape-free.

-- 
Thomas Munro
http://www.enterprisedb.com


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Adding support for Default partition in partitioning

[Sven R. Kunze]

Hi Rahila,

still thinking about the syntax (sorry):


On 04.05.2017 13:44, Rahila Syed wrote:

[...] The syntax implemented in this patch is as follows,

CREATE TABLE p11 PARTITION OF p1 DEFAULT;


Rewriting the following:

On Thu, May 4, 2017 at 4:02 PM, amul sul > wrote:


[...] CREATE TABLE p1 PARTITION OF test FOR VALUES IN  (DEFAULT)
PARTITION BY LIST(b); [...]



It yields

CREATE TABLE p1 PARTITION OF test DEFAULT PARTITION BY LIST(b);

This reads to me like "DEFAULT PARTITION".


I can imagine a lot of confusion when those queries are encountered in 
the wild. I know this thread is about creating a default partition but I 
were to propose a minor change in the following direction, I think 
confusion would be greatly avoided:


CREATE TABLE p1 PARTITION OF test*AS *DEFAULT PARTITION*ED* BY LIST(b);

I know it's a bit longer but I think those 4 characters might serve 
readability in the long term. It was especially confusing to see 
PARTITION in two positions serving two different functions.


Sven

Re: [HACKERS] PG 10 release notes

[Alvaro Herrera]

Claudio Freire wrote:
> On Tue, Apr 25, 2017 at 2:45 PM, Bruce Momjian  wrote:

> > However, given your explanation, I have added the item:
> >
> >Improve speed of VACUUM's removal of trailing empty
> >heap pages (Alvaro Herrera)
> 
> That's enough for me, thanks.

Thanks!  I amended the entry by cons'ing Claudio's name as author.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] PG 10 release notes

[Alvaro Herrera]

Bruce Momjian wrote:
> On Tue, Apr 25, 2017 at 04:03:53PM +1200, David Rowley wrote:
> >  ..On 25 April 2017 at 13:31, Bruce Momjian  wrote:
> > > The only unusual thing is that this release has ~180 items while most
> > > recent release have had ~220.  The pattern I see that there are more
> > > large features in this release than previous ones.
> > 
> > Thanks for drafting this up.
> > 
> > I understand that it may have been filtered out, but I'd say that
> > 7e534adcdc70 is worth a mention.
> > 
> > Users creating BRIN indexes previously would have had to know
> > beforehand that the table would be sufficiently correlated on the
> > indexed columns for the index to be worthwhile, whereas now there's a
> > lesser need for the user to know this beforehand.
> 
> I can't see how this can be added to an existing BRIN entry, so it would
> have to be new.  The text would be:
> 
>   Improve accuracy in determining if a BRIN index scan is beneficial
> 
> though this not something I would normally mention becuause most users
> don't understand the optimizer choices and just assume it works.

The problem is that previously it was possible for a BRIN index to be
created, and cause some queries to choose it which were faster using
some other index (a btree).  The point is that with this change it
becomes more credible to create BRIN indexes without fear that such
queries are going to slow down.

Your proposed text sounds good to me.  Authors would be David Rowley and
Emre Hasegeli.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] what's up with IDENTIFIER_LOOKUP_EXPR?

[Robert Haas]

On Thu, May 4, 2017 at 4:21 PM, Tom Lane  wrote:
> Robert Haas  writes:
>> plpgsql has an enum called IdentifierLookup which includes a value
>> IDENTIFIER_LOOKUP_EXPR which is declared like this:
>> IDENTIFIER_LOOKUP_EXPR  /* In SQL expression --- special 
>> case */
>> It regrettably does not explain what exactly is special about it, and
>> AFAICT, neither does any other comment.  If I replace every usage of
>> IDENTIFIER_LOOKUP_EXPR with IDENTIFIER_LOOKUP_NORMAL, the regression
>> tests pass nonetheless.
>
> AFAICS, the only place where IDENTIFIER_LOOKUP_EXPR acts differently from
> IDENTIFIER_LOOKUP_NORMAL is plpgsql_parse_word(), and what it does is to
> prevent a lookup in plpgsql's namestack from occurring, so that an
> identifier is reported as "not recognized" even if there is a matching
> variable.  In the two places that currently select IDENTIFIER_LOOKUP_EXPR,
> that seems to be only an optimization, because they will act the same
> anyway for all the token types that plpgsql_parse_word could return.
> I don't remember if there originally was a bigger reason than that.
> But it seems reasonable to be able to signal the lookup machinery that
> we're in a SQL expression rather than someplace else; in principle that
> could have larger implications than it does right now.

Thanks for the explanation.

>> The rule, as far as I can tell from reading the code, is that
>> IDENTIFIER_LOOKUP_NORMAL looks up words, double-words (e.g. x.y), and
>> triple-words (e.g x.y.z), IDENTIFIER_LOOKUP_EXPR looks up only
>> double-words and triple-words, and IDENTIFIER_LOOKUP_DECLARE looks up
>> nothing.  But it's not clear to me exactly what the motivation for
>> that is. plpgsql_parse_word says:
>
> The doubleword and tripleword cases are slightly different: lookup can't
> be turned off completely, because we need to create matching RECFIELD
> datums for use later.  It's still true that we don't especially care what
> token is returned, but the side-effect of creating a RECFIELD entry is
> important.  Possibly we could shave a few more cycles by making the code
> know that explicitly, but it didn't seem worth the complexity at the time.

The PLPGSQL_DTYPE_* constants are another thing that's not really
documented.  You've mentioned that we should get rid of
PLPGSQL_DTYPE_ROW in favor of, uh, whatever's better than that, but
it's not clear to me what that really means, why one way is better
than the other way, or what is involved.  I'm not really clear on what
a PLpgSQL_datum_type is in general, or what any of the specific types
actually mean.  I'll guess that PLPGSQL_DTYPE_VAR is a variable, but
beyond that...

> Feel free to improve the comments if this bugs you ...

I might try to do that at some point, but I don't think my
understanding of all this is clear enough to attempt it at this point.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WITH clause in CREATE STATISTICS

[Tom Lane]

Alvaro Herrera  writes:
> Here's a patch implementing this idea.  From gram.y's comment, the
> support syntax is now:

>   
> /*
>*
>*QUERY :
> !  *CREATE STATISTICS stats_name [(stat types)] arguments
> ! 
> !  *where 'arguments' can be one or more of:
> !  *{ ON (columns)
> !  *  | FROM relations
> !  *  | WITH (options)
> !  *  | WHERE expression }

> Note that I removed the USING keyword in the stat types list, and also
> made it mandatory that that list appears immediately after the new stats
> name.  This should make it possible to have USING in the relation list
> (the FROM clause), if we allow explicit multiple relations with join
> syntax there.  The other options can appear in any order.

Hmm ... I'm not sure that I buy that particular argument.  If you're
concerned that the grammar could not handle "FROM x JOIN y USING (z)",
wouldn't it also have a problem with "FROM x JOIN y ON (z)"?

It might work anyway, since the grammar should know whether ON or USING
is needed to complete the JOIN clause.  But I think you'd better check
whether the complete join syntax works there, even if we're not going
to support it now.

I'm not against what you've done here, because I had no love for USING
in this context anyway; it conveys approximately nothing to the mind
about what is in the list it's introducing.  But I'm concerned whether
we're boxing ourselves in by using ON.

Actually, "ON" doesn't seem all that mnemonic either.  Maybe "FOR"
would be a good substitute, if it turns out that "ON" has a problem?

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Missing feature in Phrase Search?

[Sven R. Kunze]

Hi everybody,

On 21.04.2017 20:47, Josh Berkus wrote:

Oleg, Teodor, folks:

I was demo'ing phrase search for a meetup yesterday, and the user
feedback I got showed that there's a missing feature with phrase search.
  Let me explain by example:


'fix <-> error' will match 'fixed error', 'fixing error'
but not 'fixed language error' or 'fixed a small error'

'fix <2> error' will match 'fixed language error',
but not 'fixing error' or 'fixed a small error'

'fix <3> error' will match 'fixed a small error',
but not any of the other strings.


This is because the # in <#> is an exact match.

Seems like we could really use a way for users to indicate that they
want a range of word gaps.  Like, in the example above, users could
search on:

'fix <1:3> error'

... which would search for any phrase where "error" followed "fix" by
between 1 and 3 words.

Not wedded to any particular syntax for that, of course.


That could be useful. I would like to add another idea here about 
leaving out one side of the range.


'fix <:3> error'
'fix <2:> error'

To either indicate 1 (left) or unbounded (right).

Sven


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] CTE inlining

[Gavin Flower]

On 05/05/17 06:39, Tomas Vondra wrote:



On 5/4/17 8:03 PM, Joe Conway wrote:

On 05/04/2017 10:56 AM, Andrew Dunstan wrote:



On 05/04/2017 01:52 PM, Joe Conway wrote:

On 05/04/2017 10:33 AM, Alvaro Herrera wrote:

I'm not sure what your point is.  We know that for some cases the
optimization barrier semantics are useful, which is why the 
proposal is

to add a keyword to install one explicitely:

  with materialized r as
  (
 select json_populate_record(null::mytype, myjson) as x
 from mytable
  )
  select (x).*
  from r;

this would preserve the current semantics.

I haven't been able to follow this incredibly long thread, so please
excuse me if way off base, but are we talking about that a CTE 
would be

silently be rewritten as an inline expression potentially unless it is
decorated with some new syntax?

I would find that very disconcerting myself. For example, would 
this CTE

potentially get rewritten with multiple evaluation as follows?

DROP SEQUENCE IF EXISTS foo_seq;
CREATE SEQUENCE foo_seq;

WITH a(f1) AS (SELECT nextval('foo_seq'))
SELECT a.f1, a.f1 FROM a;
  f1 | ?column?
+--
   1 |1
(1 row)

ALTER SEQUENCE foo_seq RESTART;
SELECT nextval('foo_seq'), nextval('foo_seq');
  nextval | ?column?
-+--
1 |2
(1 row)



I think that would be a change in semantics, which we should definitely
not be getting. Avoiding a change in semantics might be an interesting
exercise, but we have lots of clever coders ...


Well I think my point is that I always have understood CTEs to be
executed precisely once producing a temporary result set that is then
referenced elsewhere. I don't think that property of CTEs should change.
Somewhere else in the thread someone mentioned predicate push down --
that makes sense and maybe some clever coder can come up with a patch
that does that, but I would not be in favor of CTEs being inlined and
therefore evaluated multiple times.



I agree with this, but there's a difference between "executed exactly 
once" and "producing the same result as if executed exactly once".


I may be misunderstanding what other people proposed in this thread, 
but I think the plan was to only inline CTEs where we know it won't 
change the results, etc. So e.g. CTEs with volatile functions would 
not get inlined, which includes nextval() for example.


regards

It was the behaviour of "producing the same result as if executed 
exactly once" that I was thinking of - I think this is still valid for 
triggers & volatile functions, but such behaviour should be clearly 
documented.  This what I implicitly thought about CTE's when I first 
came across them - to me it is the intuitively obvious behaviour.  
However, limiting the rows based on the body of the SELECT would often 
be a very useful optimisation



Cheers,
Gavin



--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] logical replication syntax (was DROP SUBSCRIPTION, query cancellations and slot handling)

[Tom Lane]

Robert Haas  writes:
> On Wed, May 3, 2017 at 12:38 AM, Petr Jelinek
>  wrote:
>> Ok, Let me be clear, I actually happen to agree with your proposal. The
>> reason I am moaning is that I always seem to find myself doing tons of
>> mechanical work to rewrite some cosmetic aspect of some patch based on
>> which committer is paying attention in a specific week. So while I am
>> for doing exactly what you proposed, I'd like to see couple of +1s first
>> (Peter?) since I don't want to rewrite it to something different again
>> and it's also long past freeze.

> So, Tom Lane and Thom Brown and Josh Drake all seemed generally in
> favor of cleaning this up.  Perhaps they could opine on this
> particular proposal.

It seems like there's some remaining indecision between "make it look
like the options in EXPLAIN, VACUUM, etc" and "make it look like the
WITH options found in some other statements".  I do not have a strong
opinion which one to do, but I'd definitely say that you should use WITH
in the latter case but not in the former.  I think this mostly boils down
to whether to use "=" or not; you've got "not" in the proposal, which
means you are following the EXPLAIN precedent and should not use WITH.

I'm okay with the other specifics mentioned.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pg_dump emits ALTER TABLE ONLY partitioned_table

[Stephen Frost]

Amit,

* Amit Langote (amitlangot...@gmail.com) wrote:
> On Wed, May 3, 2017 at 12:05 PM, Stephen Frost  wrote:
> > Assuming this looks good to you, I'll push it tomorrow, possibly with
> > other minor adjustments and perhaps a few more tests.
> 
> Your latest patch looks good to me.

Found a few more issues (like pg_dump not working against older versions
of PG, because the queries for older versions hadn't been adjusted) and
did a bit more tidying.

I'll push this in a couple of hours.

Thanks!

Stephen
From ec25e5ecf44d461a93187999d3a491eae49b587f Mon Sep 17 00:00:00 2001
From: Stephen Frost 
Date: Thu, 4 May 2017 17:14:51 -0400
Subject: [PATCH] Change the way pg_dump retrieves partitioning info

This gets rid of the code that issued separate queries to retrieve the
partitioning parent-child relationship, parent partition key, and child
partition bound information.  With this patch, the information is
retrieved instead using the queries issued from getTables() and
getInherits(), which is both more efficient than the previous approach
and doesn't require any new code.

Since the partitioning parent-child relationship is now retrieved with
the same old code that handles inheritance, partition attributes receive
a proper flagInhAttrs() treatment (that it didn't receive before), which
is needed so that the inherited NOT NULL constraints are not emitted if
we already emitted it for the parent.

Also, fix a bug in pg_dump's --binary-upgrade code, which caused pg_dump
to emit invalid command to attach a partition to its parent.

Author: Amit Langote, with some additional changes by me.
---
 src/bin/pg_dump/common.c |  90 -
 src/bin/pg_dump/pg_dump.c| 264 +--
 src/bin/pg_dump/pg_dump.h|  15 +--
 src/bin/pg_dump/t/002_pg_dump.pl |  36 +-
 4 files changed, 153 insertions(+), 252 deletions(-)

diff --git a/src/bin/pg_dump/common.c b/src/bin/pg_dump/common.c
index e2bc357..47191be 100644
--- a/src/bin/pg_dump/common.c
+++ b/src/bin/pg_dump/common.c
@@ -68,8 +68,6 @@ static int	numextmembers;
 
 static void flagInhTables(TableInfo *tbinfo, int numTables,
 			  InhInfo *inhinfo, int numInherits);
-static void flagPartitions(TableInfo *tblinfo, int numTables,
-			  PartInfo *partinfo, int numPartitions);
 static void flagInhAttrs(DumpOptions *dopt, TableInfo *tblinfo, int numTables);
 static DumpableObject **buildIndexArray(void *objArray, int numObjs,
 Size objSize);
@@ -77,8 +75,6 @@ static int	DOCatalogIdCompare(const void *p1, const void *p2);
 static int	ExtensionMemberIdCompare(const void *p1, const void *p2);
 static void findParentsByOid(TableInfo *self,
  InhInfo *inhinfo, int numInherits);
-static void findPartitionParentByOid(TableInfo *self, PartInfo *partinfo,
- int numPartitions);
 static int	strInArray(const char *pattern, char **arr, int arr_size);
 
 
@@ -97,10 +93,8 @@ getSchemaData(Archive *fout, int *numTablesPtr)
 	NamespaceInfo *nspinfo;
 	ExtensionInfo *extinfo;
 	InhInfo*inhinfo;
-	PartInfo*partinfo;
 	int			numAggregates;
 	int			numInherits;
-	int			numPartitions;
 	int			numRules;
 	int			numProcLangs;
 	int			numCasts;
@@ -238,10 +232,6 @@ getSchemaData(Archive *fout, int *numTablesPtr)
 	inhinfo = getInherits(fout, &numInherits);
 
 	if (g_verbose)
-		write_msg(NULL, "reading partition information\n");
-	partinfo = getPartitions(fout, &numPartitions);
-
-	if (g_verbose)
 		write_msg(NULL, "reading event triggers\n");
 	getEventTriggers(fout, &numEventTriggers);
 
@@ -255,11 +245,6 @@ getSchemaData(Archive *fout, int *numTablesPtr)
 		write_msg(NULL, "finding inheritance relationships\n");
 	flagInhTables(tblinfo, numTables, inhinfo, numInherits);
 
-	/* Link tables to partition parents, mark parents as interesting */
-	if (g_verbose)
-		write_msg(NULL, "finding partition relationships\n");
-	flagPartitions(tblinfo, numTables, partinfo, numPartitions);
-
 	if (g_verbose)
 		write_msg(NULL, "reading column info for interesting tables\n");
 	getTableAttrs(fout, tblinfo, numTables);
@@ -293,10 +278,6 @@ getSchemaData(Archive *fout, int *numTablesPtr)
 	getPolicies(fout, tblinfo, numTables);
 
 	if (g_verbose)
-		write_msg(NULL, "reading partition key information for interesting tables\n");
-	getTablePartitionKeyInfo(fout, tblinfo, numTables);
-
-	if (g_verbose)
 		write_msg(NULL, "reading publications\n");
 	getPublications(fout);
 
@@ -354,43 +335,6 @@ flagInhTables(TableInfo *tblinfo, int numTables,
 	}
 }
 
-/* flagPartitions -
- *	 Fill in parent link fields of every target table that is partition,
- *	 and mark parents of partitions as interesting
- *
- * modifies tblinfo
- */
-static void
-flagPartitions(TableInfo *tblinfo, int numTables,
-			  PartInfo *partinfo, int numPartitions)
-{
-	int		i;
-
-	for (i = 0; i < numTables; i++)
-	{
-		/* Some kinds are never partitions */
-		if (tblinfo[i].relkind == RELKIND_SEQUENCE ||
-			tblinfo[i].relkind == RELKIND_VIEW ||
-			tblinfo[i].relkind == REL

Re: [HACKERS] Row Level Security UPDATE Confusion

[Stephen Frost]

Robert, all,

* Robert Haas (robertmh...@gmail.com) wrote:
> On Fri, Apr 14, 2017 at 9:16 AM, Stephen Frost  wrote:
> > I agreed already up-thread that there's an issue there and will be
> > looking to fix it.  That comment was simply replying to Rod's point that
> > the documentation could also be improved.
> 
> OK, thanks.  The wrap for the next set of minor releases is, according
> to my understanding, scheduled for Monday, so you'd better jump on
> this soon if you're hoping to get a fix out this time around.

I've worked out what's happening here and it's because the ALL policy
has both USING and WITH CHECK that it's not acting the same as the
SELECT policy (which can only have USING).  add_with_check_quals() is
what determines if the WITH CHECK policy or the USING policy should be
used (through a bit of a grotty #define, if you ask me..).

I've been considering how best to fix it.  The two main options are to
use a different WCOKind and then track that through, which might be nice
as we might be able to provide a more useful error message in that case,
or to just add an additional flag to add_with_check_quals() to say
"always add the USING clause when this flag is true."

Either way, I expect to wrap this up either later tonight or tomorrow.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] PG 10 release notes

[Alvaro Herrera]

Thanks for doing this, looks great.  A few notes:

  
   
  
   Add the ability to compute a correlation ratio and the number of
   distinct values on several columns (Tomas Vondra, David Rowley)
  

I think this should be worded in terms of "extended data statistics" or
such.  I think your proposed wording is a bit obscure.  How about
something like "Add extended statistics to improve query planning".
Also, I'd add myself as co-author, with Tomas' permission.

  
   
   
Cause BRIN index summarization to happen more
aggressively (Álvaro Herrera)
   

   
Specifically, summarize the previous page range when a new page
range is created.
   
  

I think it should be pointed out that this is optional and defaults to
off.  Maybe start with "add option to ..."  (I wonder if it's worth
creating a linkable area in the docs that explain this feature, so that
we could have a link in the relnotes entry).


  
   
   
Add function brin_desummarize_range() to remove
BRIN summarization of a specified range (Álvaro
Herrera)
   

   
This allows future BRIN index summarization to be
more compact.  CLARIFY
   
  

The point of this change is that if data has changed (delete, update) in
a way that could use tighter min/max limits, it would be worthwhile to
remove the previous BRIN tuple so that a new one is created so that
future scans can skip scanning the range.  Maybe word it something like
"This is useful if UPDATE and DELETE have changed the data to tighter
limits; a future BRIN index entry will be more accurate"?


 
  
  
   Add support for converting XML-formatted data into a row
   set (Pavel Stehule, Álvaro Herrera)
  
XMLTABLE is a sql-standard-specified construct, so we should mention it
by name in the leading paragraph more prominently ("add support for the
XMLTABLE standard feature" or something like that); also, I think it
should be in the Queries section, not Functions.


   
Add GUC 
to limit the number of worker processes that can be used for
parallelism (Julien Rouhaud)
   

   
This can be set lower than  to reserve worker processes
for non-parallel purposes.
   

Strange last phrase.  I suggest "...to reserve worker processes for
purposes other than parallel queries".  Also in the leading para, "for
parallelism" should probably be "for query parallelism".


I think commit e306df7f9 ("Full Text Search support for JSON
and JSONB") does definitely not belong to section Indexes; I
think Data Types is a better fit.


I think commits 67dc4ccbb and 1753b1b02 (pg_sequence and pg_sequences)
should be listed in the opposite order.  Maybe merge them into one?


   
This proves better security than the existing md5
negotiation and storage method.
   
You probably mean "provides" not "proves".


   
Allow SSL configuration to be updated at
SIGHUP (Andreas Karlsson, Tom Lane)
   
SIGHUP seems much too technical.  How about "... to be updated during
configurating reload"


I think the entry for commits a734fd5d1 and dafa0848d does not belong in
the reliability section.  In fact I wonder if it's necessary to list
this one at all.



 Increase the maximum configurable WAL size to 1
 gigabyte (Beena Emerson)

"WAL segment size" perhaps, and note that this is useful to reduce
frequency of archive_command?


   
Also add -nosync option to disable fsync.
   
Misspelled --no-sync.


   
Add log options for pg_ctl wait (--wait)
and no-wait (--no-wait) (Vik Fearing)
   
Mispelled "long options".


-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP Patch: Precalculate stable functions, infrastructure v1

[Alexander Korotkov]

On Thu, May 4, 2017 at 7:51 PM, Marina Polyakova  wrote:

> and here I send infrastructure patch which includes <...>
>>
>
> Next 2 patches:
>
> Patch 'planning and execution', which includes:
> - replacement nonvolatile functions and operators by appropriate cached
> expressions;
> - planning and execution cached expressions;
> - regression tests.
>
> Patch 'costs', which includes cost changes for cached expressions
> (according to their behaviour).


Great, thank you for your work.
It's good and widely used practice to prepend number to the patch name
while dealing with patch set.

--
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: [HACKERS] what's up with IDENTIFIER_LOOKUP_EXPR?

[Tom Lane]

Robert Haas  writes:
> The PLPGSQL_DTYPE_* constants are another thing that's not really
> documented.

Yeah :-(.  Complain to Jan sometime.

> You've mentioned that we should get rid of
> PLPGSQL_DTYPE_ROW in favor of, uh, whatever's better than that, but
> it's not clear to me what that really means, why one way is better
> than the other way, or what is involved.  I'm not really clear on what
> a PLpgSQL_datum_type is in general, or what any of the specific types
> actually mean.  I'll guess that PLPGSQL_DTYPE_VAR is a variable, but
> beyond that...

Well, from memory ...

PLpgSQL_datum is really a symbol table entry.  The conflict against what
we mean by "Datum" elsewhere is pretty unfortunate.

VAR - variable of scalar type (well, any non-composite type ---
arrays and ranges are this kind of PLpgSQL_datum too, for instance).

REC - variable of composite type, stored as a HeapTuple.

RECFIELD - reference to a field of a REC variable.  The datum includes
the field name and a link to the datum for the parent REC variable.
Notice this implies a runtime lookup of the field name whenever we're
accessing the datum; which sucks for performance but it makes life a
lot easier when you think about the possibility of the composite type
changing underneath you.

ARRAYELEM - reference to an element of an array.  The datum includes
a subscript expression and a link to the datum for the parent array
variable (which can be a VAR, and I think a RECFIELD too).

ROW - this is where it gets fun.  A ROW is effectively a variable
of a possibly-anonymous composite type, and it is defined by a list
(in its own datum) of links to PLpgSQL_datums representing the
individual columns.  Typically the member datums would be VARs
but that's not the only possibility.

As I mentioned earlier, the case that ROW is actually well adapted
for is multiple targets in INTO and similar constructs.  For example,
if you have

SELECT ...blah blah... INTO a,b,c

then the target of the PLpgSQL_stmt_execsql is represented as a single
ROW datum whose members are the datums for a, b, and c.  That's totally
determined by the text of the function and can't change under us.

However ... somebody thought it'd be cute to use the ROW infrastructure
for variables of named composite types, too.  So if you have

DECLARE foo some_composite_type;

then the name "foo" refers to a ROW datum, and the plpgsql compiler
generates additional anonymous VAR datums, one for each declared column
in some_composite_type, which become the members of the ROW datum.
The runtime representation is actually that each field value is stored
separately in its datum, as though it were an independent VAR.  Field
references "foo.col1" are not compiled into RECFIELD datums; we just look
up the appropriate member datum during compile and make the expression
tree point to that datum directly.

So, this representation is great for speed of access and modification
of individual fields of the composite variable.  It sucks when you
want to assign to the composite as a whole or retrieve its value as
a whole, because you have to deconstruct or reconstruct a tuple to
do that.  (The REC/RECFIELD approach has approximately the opposite
strengths and weaknesses.)  Also, dealing with changes in the named
composite type is a complete fail, because we've built its structure
into the function's symbol table at parse time.

I forget why there's a dtype for EXPR.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] CTE inlining

[Andreas Karlsson]

On 05/04/2017 06:22 PM, Andrew Dunstan wrote:

I wrote this query:

select (json_populate_record(null::mytype, myjson)).*
from mytable;


It turned out that this was an order of magnitude faster:

with r as
(
   select json_populate_record(null::mytype, myjson) as x
   from mytable
)
select (x).*
from r;


I do not know the planner that well, but I imagined that when we remove 
the optimization fence that one would be evaluated similar to if it had 
been a lateral join, i.e. there would be no extra function calls in this 
case after removing the fence.


Andreas


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WITH clause in CREATE STATISTICS

[Sven R. Kunze]

On 04.05.2017 23:13, Tom Lane wrote:

I'm not against what you've done here, because I had no love for USING
in this context anyway; it conveys approximately nothing to the mind
about what is in the list it's introducing.  But I'm concerned whether
we're boxing ourselves in by using ON.

Actually, "ON" doesn't seem all that mnemonic either.  Maybe "FOR"
would be a good substitute, if it turns out that "ON" has a problem?


The whole syntax reminds me of a regular SELECT clause. So, SELECT?


Also considering the most generic form of statistic support mentioned in 
[1], one could even thing about allowing aggregates, windowing functions 
etc, aka the full SELECT clause in the future.



Sven


[1] 
https://www.postgresql.org/message-id/CAEZATCUtGR+U5+QTwjHhe9rLG2nguEysHQ5NaqcK=vbj78v...@mail.gmail.com 




--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] PG 10 release notes

[Merlin Moncure]

On Mon, May 1, 2017 at 7:02 AM, Robert Haas  wrote:
> On Tue, Apr 25, 2017 at 11:01 AM, Bruce Momjian  wrote:
>> I didn't think logical decoding was really more than a proof-of-concept
>> until now.
>
> /me searches for jaw on floor.
>
> I would not in any way refer to logical decoding as being only a proof
> of concept, even before logical replication.

That's fair, but I think I understand what Bruce was going for here.
Data point: github third party modules are generally not approved for
deployment in my organization so logical decoding from a production
perspective does not exist (for me) until 10.0.  Point being, an
important threshold has been crossed.

merlin


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] PG 10 release notes

[Andres Freund]

On 2017-04-25 15:29:01 -0400, Bruce Momjian wrote:
> Uh, the only logical decoding code that I know we ship pre-PG 10 is
> contrib/test_decoding/.

That's completely wrong.  src/backend/replication/logical/ is a a bit
bigger than that...

- Andres


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] PG 10 release notes

[Andres Freund]

On 2017-05-04 17:33:13 -0500, Merlin Moncure wrote:
> On Mon, May 1, 2017 at 7:02 AM, Robert Haas  wrote:
> > On Tue, Apr 25, 2017 at 11:01 AM, Bruce Momjian  wrote:
> >> I didn't think logical decoding was really more than a proof-of-concept
> >> until now.
> >
> > /me searches for jaw on floor.

Yea, this kind of argument is getting really old.  People, including
Robert and I in this thread, have spent tremendous amounts of work on
it.  Not fun to just get that discounted.


> > I would not in any way refer to logical decoding as being only a proof
> > of concept, even before logical replication.
> 
> That's fair, but I think I understand what Bruce was going for here.
> Data point: github third party modules are generally not approved for
> deployment in my organization so logical decoding from a production
> perspective does not exist (for me) until 10.0.  Point being, an
> important threshold has been crossed.

By that argument the extension APIs, which after all are what external
FDWs, external index types, postgis, and other extensions use, aren't a
feature of postgres.  Some sites not being able to use external
extensions is *one* argument for building some things into core, but
that doesn't mean the extension APIs don't exist or aren't features.

- Andres


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] logical replication syntax (was DROP SUBSCRIPTION, query cancellations and slot handling)

[Petr Jelinek]

On 04/05/17 23:29, Tom Lane wrote:
> Robert Haas  writes:
>> On Wed, May 3, 2017 at 12:38 AM, Petr Jelinek
>>  wrote:
>>> Ok, Let me be clear, I actually happen to agree with your proposal. The
>>> reason I am moaning is that I always seem to find myself doing tons of
>>> mechanical work to rewrite some cosmetic aspect of some patch based on
>>> which committer is paying attention in a specific week. So while I am
>>> for doing exactly what you proposed, I'd like to see couple of +1s first
>>> (Peter?) since I don't want to rewrite it to something different again
>>> and it's also long past freeze.
> 
>> So, Tom Lane and Thom Brown and Josh Drake all seemed generally in
>> favor of cleaning this up.  Perhaps they could opine on this
>> particular proposal.
> 
> It seems like there's some remaining indecision between "make it look
> like the options in EXPLAIN, VACUUM, etc" and "make it look like the
> WITH options found in some other statements".  I do not have a strong
> opinion which one to do, but I'd definitely say that you should use WITH
> in the latter case but not in the former.  I think this mostly boils down
> to whether to use "=" or not; you've got "not" in the proposal, which
> means you are following the EXPLAIN precedent and should not use WITH.
> 

Okay, here is my initial attempt on changing this. I opted for WITH and
"=" as I like it slightly better (also the generic_options expect values
to be quoted which I don't like and then I would have to again invent
something like generic_options but not quite the same).

Most of the changes go to doc and tests, not that much code has changed
as I already used the definiton (which is the parser's name for these
WITH things). Except that I removed the NO shorthands and changed
publish_insert,etc to publish = 'insert,...'. I also changed the
NOREFRESH to SKIP REFRESH.

I didn't touch the DROP SUBSCRIPTION slot handling so far, that needs to
be separate patch as there is behavior change there, while this is
purely cosmetic and IMHO it's better to not mix those. (I plan to send
patch for that which changes the behavior heavily soonish as well)

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


Rework-the-options-for-logical-replication.patch
Description: binary/octet-stream

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] PG 10 release notes

[Bruce Momjian]

On Thu, Apr 27, 2017 at 10:21:44AM +0500, Andrew Borodin wrote:
> Hi, Bruce!
> 
> 2017-04-25 6:31 GMT+05:00 Bruce Momjian :
> > The only unusual thing is that this release has ~180 items while most
> > recent release have had ~220.  The pattern I see that there are more
> > large features in this release than previous ones.
> 
> I'm not sure, but, probably, it worth mentioning this [1,2] in the
> E.1.3.1.2. Indexes section.
> Better tuple management on GiST page allows faster inserts and
> updates. (In numbers: 15% for randomized data,3x for ordered data in
> specific cases)
> 
> [1] https://commitfest.postgresql.org/10/661/
> [2] 
> https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=b1328d78f88cdf4f7504004159e530b776f0de16

OK, got it.  Here is the new item:

   Author: Tom Lane 
   2016-09-09 [b1328d78f] Invent PageIndexTupleOverwrite, and
   teach BRIN and GiST

   Allow faster GiST inserts and updates by reusing
   index space more efficiently (Andrey Borodin)

-- 
  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 +


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] PG 10 release notes

[Bruce Momjian]

On Thu, Apr 27, 2017 at 10:43:34AM +0200, Fabien COELHO wrote:
> 
> Hello Bruce,
> 
> >I have committed the first draft of the Postgres 10 release notes.  They
> >are current as of two days ago, and I will keep them current.  Please
> >give me any feedback you have.
> 
> About:
> 
> """
>   Fix psql \p to always print what would be executed by \g or \w (Daniel
>   Vérité)
> 
>   Previously \p didn't properly print the reverted-to command after a
>   buffer contents reset. CLARIFY?
> """
> 
> The fix is linked to the change introduced by Tom when refactoring/cleaning
> up in e984ef586 (\if) which change psql's \p behavior.
> 
> This is not a user visible change version-to-version, it is more like a bug
> fix for a bug/behavioral change introduced within pg10 developement process
> itself.
> 
> I'm not sure how this should appear in the release notes. Maybe not at all,
> associated to the feature in which the behavioral change was introduced...

Agreed, I removed the item and moved those commits to the \if item.

-- 
  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 +


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] PG 10 release notes

[Bruce Momjian]

On Thu, Apr 27, 2017 at 03:04:57PM +0200, Daniel Verite wrote:
>   Fabien COELHO wrote:
> 
> >Fix psql \p to always print what would be executed by \g or \w (Daniel
> >Vérité)
> > 
> >Previously \p didn't properly print the reverted-to command after a
> >buffer contents reset. CLARIFY?
> > 
> > The fix is linked to the change introduced by Tom when 
> > refactoring/cleaning up in e984ef586 (\if) which change psql's \p 
> > behavior.
> 
> That's my recollection as well. The "Previously" does not refer to 9.6,
> but to that commit.

Yes, adjusted.

> > I'm not sure how this should appear in the release notes. Maybe not at 
> > all, associated to the feature in which the behavioral change was 
> > introduced...
> 
> There is small change of behavior coming as a by-product of the
> introduction of  /if.../endif blocks.
> 
> When doing in 9.x:
> 
> select '1st buffer' \g
> followed by \e
> and editing with select '2nd buffer' (without ending the query)
> and then back in psql doing '\r' and '\p', the result is
> select '2nd buffer'
> 
> The same with v10 leads instead to
> select '1st buffer'
> 
> I'm not sure whether it's above the level of detail worth being mentioned
> in the release notes.

Wow, I am not sure how to even explain that.  ;-)  Let's see if we get
any user feedback on the change.

-- 
  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 +


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] PG 10 release notes

[Bruce Momjian]

On Thu, Apr 27, 2017 at 04:05:09PM +0100, Dagfinn Ilmari Mannsåker wrote:
> Bruce Momjian  writes:
> 
> > I have committed the first draft of the Postgres 10 release notes.  They
> > are current as of two days ago, and I will keep them current.  Please
> > give me any feedback you have.
> 
> I noticed a few niggles with the links in "my" entires, patch attached.

Thanks, applied.

-- 
  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 +


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] PG 10 release notes

[Bruce Momjian]

On Fri, Apr 28, 2017 at 01:12:34PM +0900, Masahiko Sawada wrote:
> On Tue, Apr 25, 2017 at 10:31 AM, Bruce Momjian  wrote:
> > I have committed the first draft of the Postgres 10 release notes.  They
> > are current as of two days ago, and I will keep them current.  Please
> > give me any feedback you have.
> >
> 
> Related to a following item in release note, oldestxmin is also
> reported by VACUUM VERBOSE and autovacuum , which is introduced by
> commit 9eb344faf54a849898d9be012ddfa8204cfeb57c (author is Simon).
> 
> * Have VACUUM VERBOSE report the number of skipped frozen pages
> (Masahiko Sawada)
> 
> Could we add this item to the release note?

OK, adjusted text below and commit added above this item:

   Have VACUUM VERBOSE report
   the number of skipped frozen pages and oldest xmin (Masahiko Sawada)

-- 
  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 +


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Potential issue with alter system

[Joshua D. Drake]

On 05/04/2017 12:49 PM, Tom Lane wrote:

"Joshua D. Drake"  writes:

So I did this:




If you have other entries you want to keep in the postgresql.auto.conf
file, you could get away with manually editing it to remove the newline.


Got it. Thanks for digging in. This is actually a very real and easy way 
to explain to people why they need to keep up to date on their dot releases.


Thanks,

JD



regards, tom lane




--
Command Prompt, Inc.  http://the.postgres.company/
+1-503-667-4564
PostgreSQL Centered full stack support, consulting and development.
Everyone appreciates your honesty, until you are honest with them.


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] PG 10 release notes

[Bruce Momjian]

On Mon, May  1, 2017 at 08:02:46AM -0400, Robert Haas wrote:
> On Tue, Apr 25, 2017 at 11:01 AM, Bruce Momjian  wrote:
> >> Or the ability of logical decoding to follow timeline switches.
> >
> > I didn't think logical decoding was really more than a proof-of-concept
> > until now.
> 
> /me searches for jaw on floor.
> 
> It sounds like you don't understand how logical decoding works.  There
> are plugins -- fairly widely used, I think -- like
> https://github.com/confluentinc/bottledwater-pg and
> https://github.com/eulerto/wal2json which use the in-core
> infrastructure to do very nifty things, much like there are foreign
> data wrappers other than postgres_fdw.  Even test_decoding is (perhaps
> regrettably) being used to build production solutions.  The point is
> that most of the logic is in core; test_decoding or bottlewater or
> wal2json are just small plugins that tap into that infrastructure.
> 
> I would not in any way refer to logical decoding as being only a proof
> of concept, even before logical replication.

The community ships a reliable logical _encoding_, and a test logical
_decoding_.

This came up from discussion related to this item:

the ability of logical decoding to follow timeline switches

My point was that based on the text it is test_decoding that can do
timeline switches, and is that significant enough to mention in the
release notes?  Now, if it is that logical "encoding" now allows
external logical decoding modules to handle timeline switches, that is
different, but no one has said that yet.

You can have all the emotional reactions you want.

-- 
  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 +


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] snapbuild woes

[Andres Freund]

Hi,

On 2017-05-02 08:55:53 +0200, Petr Jelinek wrote:
> Aah, now I understand we talked about slightly different things, I
> considered the running thing to be first step towards tracking aborted
> txes everywhere.
> I think
> we'll have to revisit tracking of aborted transactions in PG11 then
> though because of the 'snapshot too large' issue when exporting, at
> least I don't see any other way to fix that.

FWIW, that seems unnecessary - we can just check for that using the
clog.  Should be very simple to check for aborted xacts when exporting
the snapshot (like 2 lines + comments).  That should address your
concern, right?


> If you think that adding the SNAPBUILD_BUILD_INITIAL_SNAPSHOT would be
> less invasive/smaller patch I am okay with doing that for PG10.

Attached is a prototype patch for that.

What I decided is that essentially tracking the running xacts is too
unrealiable due to the race, so I decided that just relying on
oldestRunningXid and nextXid - which are solely in the procArray and
thus racefree - is better.

It's not perfect yet, primarily because we'd need to take a bit more
care about being ABI compatible for older releases, and because we'd
probably have to trigger LogStandbySnapshot() a bit more frequently
(presumably while waiting for WAL).  The change means we'll have to wait
a bit longer for slot creation, but it's considerably simpler / more
robust.

Could you have a look?

Regards,

Andres


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] CTE inlining

[Craig Ringer]

On 5 May 2017 02:52, "Tom Lane"  wrote:

Tomas Vondra  writes:
> On 5/4/17 8:03 PM, Joe Conway wrote:
>>> I haven't been able to follow this incredibly long thread, so please
>>> excuse me if way off base, but are we talking about that a CTE would be
>>> silently be rewritten as an inline expression potentially unless it is
>>> decorated with some new syntax?

> I agree with this, but there's a difference between "executed exactly
> once" and "producing the same result as if executed exactly once".

> I may be misunderstanding what other people proposed in this thread, but
> I think the plan was to only inline CTEs where we know it won't change
> the results, etc. So e.g. CTEs with volatile functions would not get
> inlined, which includes nextval() for example.

I haven't been keeping close tabs either, but surely we still have to have
the optimization fence in (at least) all these cases:

* CTE contains INSERT/UPDATE/DELETE
* CTE contains SELECT FOR UPDATE/SHARE (else the set of rows that get
  locked might change)
* CTE contains volatile functions

I'm willing to write off cases where, eg, a function should have been
marked volatile and was not.  That's user error and there are plenty
of hazards of that kind already.  But if the optimizer has reason
to know that discarding the fence might change any query side-effects,
it mustn't.


I think everyone is in total agreement there.

Re: [HACKERS] PG 10 release notes

[Bruce Momjian]

On Mon, May  1, 2017 at 10:20:38AM -0400, Robert Haas wrote:
> I'm pretty sure this is not the first year in which your policy of
> excluding certain performance-related items has met with opposition.
> I agree that there are some improvements that are sufficiently small
> and boring that they do not merit a mention, but (1) that's also true
> of non-performance items and (2) the fact that people keep complaining
> about performance items you excluded constitutes a discussion of
> changing your filter.
> 
> My own opinion is that the filter should not be particularly different
> for performance items vs. non-performance.  The question ought to be
> whether the change is significant enough that users are likely to
> care.  If you've got several people saying "hey, you forgot NN in
> the release notes!" it is probably a good bet that the change is
> significant enough that users will care about it.

Yes, the "do people care" filter is what I usually use.  When new
functionality is added, we usually mention it because someone usually
care, but for performance, the threshold is usually whether workloads,
even rare ones, would have a visible performance change.  It is
difficult to determine this from the release notes, which is why I
always need feedback on these items.

-- 
  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 +


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] PG 10 release notes

[Andres Freund]

On 2017-05-04 19:56:21 -0400, Bruce Momjian wrote:
> On Mon, May  1, 2017 at 08:02:46AM -0400, Robert Haas wrote:
> > On Tue, Apr 25, 2017 at 11:01 AM, Bruce Momjian  wrote:
> > >> Or the ability of logical decoding to follow timeline switches.
> > >
> > > I didn't think logical decoding was really more than a proof-of-concept
> > > until now.
> > 
> > /me searches for jaw on floor.
> > 
> > It sounds like you don't understand how logical decoding works.  There
> > are plugins -- fairly widely used, I think -- like
> > https://github.com/confluentinc/bottledwater-pg and
> > https://github.com/eulerto/wal2json which use the in-core
> > infrastructure to do very nifty things, much like there are foreign
> > data wrappers other than postgres_fdw.  Even test_decoding is (perhaps
> > regrettably) being used to build production solutions.  The point is
> > that most of the logic is in core; test_decoding or bottlewater or
> > wal2json are just small plugins that tap into that infrastructure.
> > 
> > I would not in any way refer to logical decoding as being only a proof
> > of concept, even before logical replication.
> 
> The community ships a reliable logical _encoding_, and a test logical
> _decoding_.

Yes, so what?  What you said is "I didn't think logical decoding was
really more than a proof-of-concept until now", which is plainly wrong,
given I know a significant number of users using it in production.  Some
of them are well known & large enterprises, and it's used to enable
critical things.


On Mon, May  1, 2017 at 08:02:46AM -0400, Robert Haas wrote:
> Even test_decoding is (perhaps regrettably) being used to build production 
> solutions.

E.g. to power amazon's data migration service (yes, that scares me).


> My point was that based on the text it is test_decoding that can do
> timeline switches, and is that significant enough to mention in the
> release notes?  Now, if it is that logical "encoding" now allows
> external logical decoding modules to handle timeline switches, that is
> different, but no one has said that yet.

The change has nothing to do with test_decoding.


Petr: The timeline change itself does, for the moment, not seem
particularly noteworthy to me - it's not really useful atm on its own?
To me it's more of infrastructure to add "logical decoding on standby"
next release?


> You can have all the emotional reactions you want.

Nice one.


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] PG 10 release notes

[Bruce Momjian]

On Thu, May  4, 2017 at 06:02:58PM -0300, Alvaro Herrera wrote:
> > I can't see how this can be added to an existing BRIN entry, so it would
> > have to be new.  The text would be:
> > 
> > Improve accuracy in determining if a BRIN index scan is beneficial
> > 
> > though this not something I would normally mention becuause most users
> > don't understand the optimizer choices and just assume it works.
> 
> The problem is that previously it was possible for a BRIN index to be
> created, and cause some queries to choose it which were faster using
> some other index (a btree).  The point is that with this change it
> becomes more credible to create BRIN indexes without fear that such
> queries are going to slow down.
> 
> Your proposed text sounds good to me.  Authors would be David Rowley and
> Emre Hasegeli.

OK, added, thanks:


 
+
+ Improve accuracy in determining if a BRIN index scan
+ is beneficial (David Rowley, Emre Hasegeli)
+
+   

-- 
  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 +


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers