Re: Typo in origin.c

[Masahiko Sawada]

On Tue, Feb 13, 2018 at 12:40 PM, Peter Eisentraut
 wrote:
> On 2/12/18 21:58, Masahiko Sawada wrote:
>> Attached a patch for fixing $subject.
>>
>> s/funcion/function/
>
> fixed
>

Thank you.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

Re: [HACKERS] [PATCH] Vacuum: Update FSM more frequently

[Masahiko Sawada]

On Fri, Feb 9, 2018 at 11:48 PM, Claudio Freire  wrote:
> On Fri, Feb 9, 2018 at 1:36 AM, Masahiko Sawada  wrote:
>> On Fri, Feb 9, 2018 at 12:45 AM, Claudio Freire  
>> wrote:
>>> On Thu, Feb 8, 2018 at 1:36 AM, Masahiko Sawada  
>>> wrote:
 On Tue, Feb 6, 2018 at 9:51 PM, Claudio Freire  
 wrote:
> I can look into doing 3, that *might* get rid of the need to do that
> initial FSM vacuum, but all other intermediate vacuums are still
> needed.

 Understood. So how about that this patch focuses only make FSM vacuum
 more frequently and leaves the initial FSM vacuum and the handling
 cancellation cases? The rest can be a separate patch.
>>>
>>> Ok.
>>>
>>> Attached split patches. All but the initial FSM pass is in 1, the
>>> initial FSM pass as in the original patch is in 2.
>>>
>>> I will post an alternative patch for 2 when/if I have one. In the
>>> meanwhile, we can talk about 1.
>>
>> Thank you for updating the patch!
>>
>> +/* Tree pruning for partial vacuums */
>> +if (threshold)
>> +{
>> +child_avail = fsm_get_avail(page, slot);
>> +if (child_avail >= threshold)
>> +continue;
>> +}
>>
>> I think slots in a non-leaf page might not have a correct value
>> because fsm_set_avail doesn't propagate up beyond pages. So do we need
>> to check the root of child page rather than a slot in the non-lead
>> page? I might be missing something though.
>
> I'm not sure I understand what you're asking.
>
> Yes, a partial FSM vacuum won't correct those inconsistencies. It
> doesn't matter though, because as long as the root node (or whichever
> inner node we're looking at the time) reports free space, the lookup
> for free space will recurse and find the lower nodes, even if they
> report more space than the inner node reported. The goal of making
> free space discoverable will have been achieved nonetheless.

For example, if a slot of internal node of fsm tree has a value
greater than the threshold while the root of leaf node of fsm tree has
a value less than threshold, we want to update the internal node of
fsm tree but we will skip it with your patch. I wonder if this can
happen.

> The final FSM vacuum pass isn't partial, to finally correct all those
> small inconsistencies.

Yes, but the purpose of this patch is to prevent table bloating from
concurrent modifications?

Here is other review comments.

+/* Tree pruning for partial vacuums */
+if (threshold)
+{
+child_avail = fsm_get_avail(page, slot);
+if (child_avail >= threshold)
+continue;
+}

'child_avail' is a category value while 'threshold' is an actual size
of freespace.

The logic of finding the max_freespace seems not work fine if table
with indices because max_freespace is not updated based on
post-compaction freespace. I think we need to get the max freespace
size during vacuuming heap (i.g. at lazy_vacuum_heap) and use it as
the threshold.

+   vacuum_fsm_every_pages = nblocks / 8;
+   vacuum_fsm_every_pages = Max(vacuum_fsm_every_pages, 1048576);

I think a comment for 1048576 is needed. And perhaps we can define it
as a macro.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

Re: [HACKERS] Partition-wise aggregation/grouping

[Rafia Sabih]

On Tue, Feb 13, 2018 at 6:21 PM, Jeevan Chalke <
jeevan.cha...@enterprisedb.com> wrote:

>
> I see that partition-wise aggregate plan too uses parallel index, am I
> missing something?
>
>
You're right, I missed that, oops.

>
>> Q18 takes some 390 secs with patch and some 147 secs without it.
>>
>
> This looks strange. This patch set does not touch parallel or seq scan as
> such. I am not sure why this is happening. All these three queries explain
> plan shows much higher execution time for parallel/seq scan.
>
> Yeah strange it is.

> However, do you see similar behaviour with patches applied,
> "enable_partition_wise_agg = on" and "enable_partition_wise_agg = off" ?
>

I tried that for query 18, with patch and  enable_partition_wise_agg = off,
query completes in some 270 secs. You may find the explain analyse output
for it in the attached file. I noticed that on head the query plan had
parallel hash join however with patch and no partition-wise agg it is using
nested loop joins. This might be the issue.

>
> Also, does rest of the queries perform better with partition-wise
> aggregates?
>
>
As far as this setting goes, there wasn't any other query using
partition-wise-agg, so, no.

BTW, just an FYI, this experiment is on scale factor 20.

-- 
Regards,
Rafia Sabih
EnterpriseDB: http://www.enterprisedb.com/


18_pwa_off.out
Description: Binary data

Re: proposal: alternative psql commands quit and exit

[Ivan E. Panchenko]

14.02.2018 09:17, Bruce Momjian пишет:

On Wed, Feb 14, 2018 at 03:07:46PM +0900, Michael Paquier wrote:

On Wed, Feb 14, 2018 at 12:56:48AM -0500, Bruce Momjian wrote:

On Mon, Feb 12, 2018 at 01:28:53AM -0500, Bruce Momjian wrote:

Should we give the same "Use control-D to quit." hint here for '\q'?
I think it is logical that we should.

I have applied the attached patch giving a ^D/^C hint where \q is
ignored, and add C comment documenting why exit/quit is not documented.

I consider this issue closed, or quited, or exited.  :-)

FYI, Ivan Panchenko got applause at PGConf.Russia 2018 in Moscow when he
mentioned we added quit/exit to psql.  :-)  I also think the ^D/^C hint
for \q when you are in quotes will help too.

:)
Will there be a video or a reference to the talk slides?  I'd love to
hear the audience reaction.

Uh, I think it was this talk, "What to expect in Pg 11 ?"

https://pgconf.ru/en/2018/110916

I know that Ivan Panchenko was not mentioned as a presenter for that,
but he was a presenter.  I also know there was a video camera but I
don't see the recordings online.  The applause did surprise me.
This talk was technically divided into two parts, one (Postgres as the 
Universal DBMS) by Oleg Bartunov, who was late due to the air traffic 
jam, so it had to be presented by me.
Other part (Pg11) was presented by Teodor Sigaev and Alexander Korotkov. 
The video recordings of the conference will be available on the website 
in a couple of weeks.

Re: proposal: alternative psql commands quit and exit

[Bruce Momjian]

On Wed, Feb 14, 2018 at 03:07:46PM +0900, Michael Paquier wrote:
> On Wed, Feb 14, 2018 at 12:56:48AM -0500, Bruce Momjian wrote:
> > On Mon, Feb 12, 2018 at 01:28:53AM -0500, Bruce Momjian wrote:
> > > > Should we give the same "Use control-D to quit." hint here for '\q'?
> > > > I think it is logical that we should.
> > > 
> > > I have applied the attached patch giving a ^D/^C hint where \q is
> > > ignored, and add C comment documenting why exit/quit is not documented.
> > > 
> > > I consider this issue closed, or quited, or exited.  :-)
> > 
> > FYI, Ivan Panchenko got applause at PGConf.Russia 2018 in Moscow when he
> > mentioned we added quit/exit to psql.  :-)  I also think the ^D/^C hint
> > for \q when you are in quotes will help too.
> 
> :)
> Will there be a video or a reference to the talk slides?  I'd love to
> hear the audience reaction.

Uh, I think it was this talk, "What to expect in Pg 11 ?"

https://pgconf.ru/en/2018/110916

I know that Ivan Panchenko was not mentioned as a presenter for that,
but he was a presenter.  I also know there was a video camera but I
don't see the recordings online.  The applause did surprise me.

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

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

Re: proposal: alternative psql commands quit and exit

[Michael Paquier]

On Wed, Feb 14, 2018 at 12:56:48AM -0500, Bruce Momjian wrote:
> On Mon, Feb 12, 2018 at 01:28:53AM -0500, Bruce Momjian wrote:
> > > Should we give the same "Use control-D to quit." hint here for '\q'?
> > > I think it is logical that we should.
> > 
> > I have applied the attached patch giving a ^D/^C hint where \q is
> > ignored, and add C comment documenting why exit/quit is not documented.
> > 
> > I consider this issue closed, or quited, or exited.  :-)
> 
> FYI, Ivan Panchenko got applause at PGConf.Russia 2018 in Moscow when he
> mentioned we added quit/exit to psql.  :-)  I also think the ^D/^C hint
> for \q when you are in quotes will help too.

:)
Will there be a video or a reference to the talk slides?  I'd love to
hear the audience reaction.
--
Michael


signature.asc
Description: PGP signature

Re: [HACKERS] MERGE SQL Statement for PG11

[Pavan Deolasee]

Hi Stephen,

On Tue, Feb 6, 2018 at 3:37 PM, Stephen Frost  wrote:

>
>
> Coming out of that, my understanding is that Simon is planning to have a
> patch which implements RLS and partitioning (though the query plans for
> partitioning may be sub-par and not ideal) as part of MERGE and I've
> agreed to review at least the RLS bits (though my intention is to at
> least go through the rest of the patch as well, though likely in less
> detail).  Of course, I encourage others to review it as well.
>
>
Thanks for volunteering to review the RLS bits. I am planning to send a
revised version soon. As I work through it, I am faced with some semantic
questions again. Would appreciate if you or anyone have answers to those.

While executing MERGE, for existing tuples in the target table, we may end
up doing an UPDATE or DELETE, depending on the WHEN MATCHED AND conditions.
So it seems unlikely that we would be able to push down USING security
quals down to the scan. For example, if the target row is set for deletion,
it seems wrong to exclude the row from the join based on UPDATE policy's
USING quals. So I am thinking that we should apply the respective USING
quals *after* the decision to either update, delete or do nothing for the
given target tuple is made.

The question I have is, if the USING qual evaluates to false or NULL,
should we silently ignore the tuple (like regular UPDATE does) or throw an
error (like INSERT ON CONFLICT DO UPDATE)? ISTM that we might have decided
to throw an error in case of INSERT ON CONFLICT to avoid any confusion
where the tuple is neither inserted nor updated. Similar situation may
arise with MERGE because for a source row, we may neither do UPDATE
(because of RLS) nor INSERT because a matching tuple already exists. But
someone may argue that we should stay closer to regular UPDATE/DELETE.
Apart from that, are there any security angles that we need to be mindful
of and would those impact the choice?

SELECT policies will be applied to the target table during the scan and
rows which do not pass SELECT quals will not be processed at all. If there
are NOT MATCHED actions, we might end up inserting duplicate rows in that
case or throw errors, but I don't see anything wrong with that. Similar
things would have happened if someone tried to insert rows into the table
using regular INSERT.

Similarly, INSERT policies will be applied when MERGE attempts to INSERT a
row into the table and error will be thrown if the row does not satisfy
INSERT policies.

Thanks,
Pavan

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

Re: tapeblocks is uninitialized in logtape.c

[Tom Lane]

I wrote:
> Jaime Casanova  writes:
>> My compiler gives me this message
>> logtape.c: In function 'ltsConcatWorkerTapes':
>> logtape.c:462:48: warning: 'tapeblocks' may be used uninitialized in
>> this function [-Wmaybe-uninitialized]
>> lts->nBlocksAllocated = lt->offsetBlockNumber + tapeblocks;

> FWIW, I'm not seeing that.  What compiler are you using exactly?
> (There are one or two other places where I see "may be used uninitialized"
> complaints from certain older gcc versions.  Not sure how excited we
> should be about removing such warnings.)

Enlarging on that a bit ... I scraped the buildfarm logs to find all the
"may be used uninitialized" warnings currently being emitted by various
critters on HEAD.  The full list is attachment 1.

Now, a large majority of those warnings are from coypu, locust, and
prairiedog, which are the three buildfarm members that use the broken
fallback implementations of the integer-ops-with-overflow-detection
functions in int.h, and I *will* continue to hound Andres until he accepts
that we need to fix those.  There is a difference between "implementation
defined" and "undefined" behaviors, and those functions are on the wrong
side of that line.

However, if we exclude those three critters from the report, we get
attachment 2, which shows that there are exactly 8 places that are
being whined about by multiple buildfarm animals.  Maybe it's worth
cleaning up those 8?

regards, tom lane

 locust| 2018-02-14 01:13:35 | data.c:299: warning: 'dres' may be used 
uninitialized in this function
 prairiedog| 2018-02-13 23:55:10 | data.c:299: warning: 'dres' may be used 
uninitialized in this function
 locust| 2018-02-14 01:13:35 | dependencies.c:970: warning: 'attnum' 
may be used uninitialized in this function
 prairiedog| 2018-02-13 23:55:10 | dependencies.c:970: warning: 'attnum' 
may be used uninitialized in this function
 locust| 2018-02-14 01:13:35 | float.c:3528: warning: 'result' may be 
used uninitialized in this function
 prairiedog| 2018-02-13 23:55:10 | float.c:3528: warning: 'result' may be 
used uninitialized in this function
 coypu | 2018-02-14 00:20:30 | int.c:1030: warning: 'result' may be 
used uninitialized in this function
 locust| 2018-02-14 01:13:35 | int.c:1030: warning: 'result' may be 
used uninitialized in this function
 prairiedog| 2018-02-13 23:55:10 | int.c:1030: warning: 'result' may be 
used uninitialized in this function
 coypu | 2018-02-14 00:20:30 | int.c:1044: warning: 'result' may be 
used uninitialized in this function
 locust| 2018-02-14 01:13:35 | int.c:1044: warning: 'result' may be 
used uninitialized in this function
 prairiedog| 2018-02-13 23:55:10 | int.c:1044: warning: 'result' may be 
used uninitialized in this function
 coypu | 2018-02-14 00:20:30 | int.c:1058: warning: 'result' may be 
used uninitialized in this function
 locust| 2018-02-14 01:13:35 | int.c:1058: warning: 'result' may be 
used uninitialized in this function
 prairiedog| 2018-02-13 23:55:10 | int.c:1058: warning: 'result' may be 
used uninitialized in this function
 coypu | 2018-02-14 00:20:30 | int.c:607: warning: 'sum' may be used 
uninitialized in this function
 locust| 2018-02-14 01:13:35 | int.c:607: warning: 'sum' may be used 
uninitialized in this function
 prairiedog| 2018-02-13 23:55:10 | int.c:607: warning: 'sum' may be used 
uninitialized in this function
 coypu | 2018-02-14 00:20:30 | int.c:654: warning: 'sum' may be used 
uninitialized in this function
 locust| 2018-02-14 01:13:35 | int.c:654: warning: 'sum' may be used 
uninitialized in this function
 prairiedog| 2018-02-13 23:55:10 | int.c:654: warning: 'sum' may be used 
uninitialized in this function
 coypu | 2018-02-14 00:20:30 | int.c:689: warning: 'sum' may be used 
uninitialized in this function
 locust| 2018-02-14 01:13:35 | int.c:689: warning: 'sum' may be used 
uninitialized in this function
 prairiedog| 2018-02-13 23:55:10 | int.c:689: warning: 'sum' may be used 
uninitialized in this function
 coypu | 2018-02-14 00:20:30 | int.c:772: warning: 'result' may be used 
uninitialized in this function
 locust| 2018-02-14 01:13:35 | int.c:772: warning: 'result' may be used 
uninitialized in this function
 prairiedog| 2018-02-13 23:55:10 | int.c:772: warning: 'result' may be used 
uninitialized in this function
 coypu | 2018-02-14 00:20:30 | int.c:786: warning: 'result' may be used 
uninitialized in this function
 locust| 2018-02-14 01:13:35 | int.c:786: warning: 'result' may be used 
uninitialized in this function
 prairiedog| 2018-02-13 23:55:10 | int.c:786: warning: 'result' may be used 
uninitialized in this function
 coypu | 2018-02-14 00:20:30 | int.c:800: warning: 'result' may be used 
uninitialized in this function
 locust| 

Re: CALL stmt, ERROR: unrecognized node type: 113 bug

[Michael Paquier]

On Tue, Feb 13, 2018 at 03:26:20PM -0500, Peter Eisentraut wrote:
> and committed

Thanks, those changes are fine for me.

Perhaps you want to have print_function_rettype() drop a elog(ERROR) if
called with an invalid prorettype?  I tend to be allergic to Asserts
in ruleutils.c since 0daeba0e...
--
Michael


signature.asc
Description: PGP signature

[bug fix] Cascaded standby cannot start after a clean shutdown

[Tsunakawa, Takayuki]

Hello,

Our customer encountered a rare bug of PostgreSQL which prevents a cascaded 
standby from starting up.  The attached patch is a fix for it.  I hope this 
will be back-patched.  I'll add this to the next CF.


PROBLEM
==

The PostgreSQL version is 9.5.  The cluster consists of a master, a cascading 
standby (SB1), and a cascaded standby (SB2).  The WAL flows like this: master 
-> SB1 -> SB2.

The user shut down SB2 and tried to restart it, but failed with the following 
message:

FATAL:  invalid memory alloc request size 3075129344

The master and SB1 continued to run.


CAUSE
==

total_len in the code below was about 3GB, so palloc() rejected the memory 
allocation request.

[xlogreader.c]
record = (XLogRecord *) (state->readBuf + RecPtr % XLOG_BLCKSZ);
total_len = record->xl_tot_len;
...
/*
 * Enlarge readRecordBuf as needed.
 */
if (total_len > state->readRecordBufSize &&
!allocate_recordbuf(state, total_len))
{

Why was XLogRecord->xl_tot_len so big?  That's because the XLOG reader read the 
garbage portion in a WAL file, which is immediately after the end of valid WAL 
records.

Why was there the garbage?  Because the cascading standby sends just the valid 
WAL records, not the whole WAL blocks, to the cascaded standby.  When the 
cascaded standby receives those WAL records and write them in a recycled WAL 
file, the last WAL block in the file contains the mix of valid WAL records and 
the garbage left over.

Why did the XLOG reader try to allocate memory for a garbage?  Doesn't it stop 
reading the WAL?  As the following code shows, when the WAL record header 
crosses a WAL block boundary, the XLOG reader first allocates memory for the 
whole record, and then checks the validity of the record header as soon as it 
reads the entire header.

[xlogreader.c]
/*
 * If the whole record header is on this page, validate it immediately.
 * Otherwise do just a basic sanity check on xl_tot_len, and validate 
the
 * rest of the header after reading it from the next page.  The 
xl_tot_len
 * check is necessary here to ensure that we enter the "Need to 
reassemble
 * record" code path below; otherwise we might fail to apply
 * ValidXLogRecordHeader at all.
 */
if (targetRecOff <= XLOG_BLCKSZ - SizeOfXLogRecord)
{
if (!ValidXLogRecordHeader(state, RecPtr, state->ReadRecPtr, 
record,
   randAccess))
goto err;
gotheader = true;
}
else
{
/* XXX: more validation should be done here */
if (total_len < SizeOfXLogRecord)
{
report_invalid_record(state,
  "invalid 
record length at %X/%X: wanted %u, got %u",
  (uint32) 
(RecPtr >> 32), (uint32) RecPtr,
  (uint32) 
SizeOfXLogRecord, total_len);
goto err;
}
gotheader = false;
}


FIX
==

One idea is to defer the memory allocation until the entire record header is 
read and ValidXLogRecordHeader() is called.  However, ValidXLogRecordHeader() 
might misjudge the validity if the garbage contains xl_prev seeming correct.

So, I modified walreceiver to zero-fill the last partial WAL block.  This is 
the guard that the master does in AdvanceXLInsertBuffer() as below:

/*
 * Be sure to re-zero the buffer so that bytes beyond what we've
 * written will look like zeroes and not valid XLOG records...
 */
MemSet((char *) NewPage, 0, XLOG_BLCKSZ);

FYI, the following unsolved problem may be solved, too.

https://www.postgresql.org/message-id/CAE2gYzzVZNsGn%3D-E6grO4sVQs04J02zNKQofQEO8gu8%3DqCFR%2BQ%40mail.gmail.com


Regards
Takayuki Tsunakawa



zerofill_walblock_on_standby.patch
Description: zerofill_walblock_on_standby.patch

Re: TODO item: WAL replay of CREATE TABLESPACE with differing directory structure

[Michael Paquier]

On Tue, Feb 13, 2018 at 01:44:34PM -0800, Patrick Krecker wrote:
> I am searching for a way to make a contribution to Postgres and I came
> across this TODO item (I realize there has been some controversy
> around the TODO list [1], and I hope that my use of it doesn't spark
> another discussion about removing it altogether):

Well, it will point out again that TODO items are hard, complicated and
mostly impossible projects.

> "Allow WAL replay of CREATE TABLESPACE to work when the directory
> structure on the recovery computer is different from the original"
> 
> Currently it looks like tablespaces have to live inside the data
> directory on the replica, notwithstanding administrator intervention
> by manipulating the tablespace directory with symlinks after (or even
> before?) it has been created via replay.

Let's be clear here. There is no hard restriction with tablespace paths
within the data directory, though you should not do that, and you get a
nice warning when trying to do so with CREATE TABLESPACE (see 33cb8ff6).
This also causes pg_basebackup to fail.  It is also bad design to create
tablespaces within the data directory as those are aimed at making hot
paths work on different partitions with different I/O properties.

> Is the idea behind this task to allow the master to instruct the
> replica where to put the tablespace on its filesystem, so as to allow
> it to live outside of the data directory without direct manipulation
> of the filesystem?

WAL records associated to CREATE TABLESPACE (xl_tblspc_create_rec)
register the location where a tablespace is located.  The location of a
tablespace is not saved in the system catalogs, which offers flexibility
in the way the symlink from pg_tblspc can be handled.  This is where the
tablespace path remapping of pg_basebackup becomes handy, because you
can repurpose paths easily when taking a base backup, but this forces
you to create tablespaces first, and then create standbys.  We have also
a set of existing problems:
1) If a primary and its standby are on the same server and you issue a
CREATE TABLESPACE, then they would try to write to the same paths.
2) How do we design at DDL level a command which allows for specifying
different paths depending on the node where the recovery happens.

You would need in both cases a sort of ability to define a node name, so
as for 1) you append the node name to the path and both primary and
standby can use the same tablespace path, but with different sub-paths.
And for 2), you can enforce a patch name by defining as well a path
associated to a node name so as when xl_tblspc_create_rec records are
replayed at recovery, you know which path to create.  Just designing
that the right way as its own set of complications.

> If this task is a worthwhile endeavor, I would be happy to take it on.
> If not, I am open to other ideas :)

This is part of the difficult, perhaps-not-worth doing impossible
problems.  As a first contribution, you may want something easier.
--
Michael


signature.asc
Description: PGP signature

Re: unique indexes on partitioned tables

[Peter Eisentraut]

Here is a mini-patch on top of yours to fix a few cosmetic things.

I don't understand the variable name "third".  I don't see a "first" or
"second" nearby.

I find some of the columns in pg_constraint confusing.  For a primary
key on a partitioned table, for the PK on the partition I get

conislocal = false, coninhcount = 1, connoinherit = true

The last part is confusing to me.

I don't know if that's really on this patch.  But perhaps it could be
documented better.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>From 69fbe467e554f6f70195d0f40898eba62ddb9641 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Tue, 13 Feb 2018 22:53:55 -0500
Subject: [PATCH] fixup! allow indexes on partitioned tables to be unique

---
 doc/src/sgml/ref/alter_table.sgml  | 5 ++---
 doc/src/sgml/ref/create_table.sgml | 3 +--
 src/backend/catalog/index.c| 2 +-
 3 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/doc/src/sgml/ref/alter_table.sgml 
b/doc/src/sgml/ref/alter_table.sgml
index 2d3b6d3960..5be56d4b28 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -839,9 +839,8 @@ Description
  
   This form attaches an existing table (which might itself be partitioned)
   as a partition of the target table. The table can be attached
-  as a partition for specific values using FOR VALUES
-   or as a default partition by using
-  DEFAULT.
+  as a partition for specific values using FOR VALUES
+  or as a default partition by using DEFAULT.
   For each index in the target table, a corresponding
   one will be created in the attached table; or, if an equivalent
   index already exists, will be attached to the target table's index,
diff --git a/doc/src/sgml/ref/create_table.sgml 
b/doc/src/sgml/ref/create_table.sgml
index 83d3472da2..b7bf7bc1c8 100644
--- a/doc/src/sgml/ref/create_table.sgml
+++ b/doc/src/sgml/ref/create_table.sgml
@@ -546,8 +546,7 @@ Parameters
  
 
  
-  Partitioned tables do not support
-  EXCLUDE, or
+  Partitioned tables do not support EXCLUDE or
   FOREIGN KEY constraints; however, you can define
   these constraints on individual partitions.
  
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index a5fa3540a7..5fb50a3a0e 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -1325,7 +1325,7 @@ index_constraint_create(Relation heapRelation,
 */
if (OidIsValid(parentConstraintId))
{
-   ObjectAddress   third;
+   ObjectAddress   third; // ???
 
ObjectAddressSet(third, ConstraintRelationId, 
parentConstraintId);
recordDependencyOn(, , 
DEPENDENCY_INTERNAL_AUTO);
-- 
2.16.1

Re: Parallel bt build crashes when DSM_NONE

[Michael Paquier]

On Wed, Feb 14, 2018 at 10:38:58AM +0900, Kyotaro HORIGUCHI wrote:
> I'll post a patch that eliminate DSM_IMPL_NONE after this. I'd like it
> to be shipped on 11. 

Feel free to.  Be just careful that the connection attempts in
test_config_settings() should try to use the DSM implementation defined
by choose_dsm_implementation().
--
Michael


signature.asc
Description: PGP signature

Re: Parameter status message not sent?

[Tatsuo Ishii]

> It'll only get sent to the client the next time the server processes a
> query. We can't just at arbitrary points reload the config file or send
> messages out. The SIGHUP handler just sets ConfigReloadPending which
> PostgresMain() then processes:
> 
>   /*
>* (6) check for any other interesting events that happened 
> while we
>* slept.
>*/
>   if (ConfigReloadPending)
>   {
>   ConfigReloadPending = false;
>   ProcessConfigFile(PGC_SIGHUP);
>   }
> 
> which'll then, in turn, send out ParameterStatus messages for changed
> GUC_REPORT GUCs.

Thanks. I confirmed what you said by using strace attached to
backend.

One thing I noticed was, some GUC variables were sent as well even if
they were not changed.

sendto(10, "S\0\0\0\27DateStyle\0ISO, 
MDY\0S\0\0\0\23TimeZone\0Japan\0S\0\0\0#standard_conforming_strings\0on\0T\0\0\0!\0\1?column?\0\0\0\0\0\0\0\0\0\0\27\0\4\377\377\377\377\0\0D\0\0\0\v\0\1\0\0\0\0011C\0\0\0\rSELECT
 1\0Z\0\0\0\5I", 146, 0, NULL, 0) = 146

So not only standard_conforming_strings, but Datestyle and TimeZone
were sent to frontend (I only changed standard_conforming_strings in
postgresql.conf).

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp

Re: Parameter status message not sent?

[Andres Freund]

Hi,

On 2018-02-14 10:42:12 +0800, Craig Ringer wrote:
> I was wondering a while ago - can't we just set our own proc's latch here,
> so we wake up and send it earlier if we're in the idle main loop?

The problem is that the client doesn't really expect messages from the
server when it's idle... It's also not clear to me what the gain would
be?

Greetings,

Andres Freund

Re: Python 3.7 support

[Michael Paquier]

On Tue, Feb 13, 2018 at 04:17:13PM -0500, Peter Eisentraut wrote:
> A small patch to tweak the tests to support output differences with
> Python 3.7 (currently in beta).

Wouldn't it be better to wait for the version to be released before
pushing anything in the tree?  If there are again changes when this
comes out as GA then an extra patch would be needed.
--
Michael


signature.asc
Description: PGP signature

Re: Parameter status message not sent?

[Craig Ringer]

On 14 February 2018 at 10:26, Andres Freund  wrote:

> On 2018-02-14 11:12:30 +0900, Tatsuo Ishii wrote:
> > According to the manual, backend sends a parameter status message when
> > certain configuration variable has been changed and SIGHUP signal is
> sent.
> > https://www.postgresql.org/docs/10/static/protocol-flow.
> html#PROTOCOL-ASYNC
> >
> >   ParameterStatus messages will be generated whenever the active value
> >   changes for any of the parameters the backend believes the frontend
> >   should know about. Most commonly this occurs in response to a SET
> >   SQL command executed by the frontend, and this case is effectively
> >   synchronous ― but it is also possible for parameter status changes
> >   to occur because the administrator changed a configuration file and
> >   then sent the SIGHUP signal to the server.
> >
> > So I connected to PostgreSQL using psql and attached strace to psql.
> > Then I changed standard_conforming_strings and executed pg_ctl
> > reload. The PostgreSQL log shows:
> >
> > 12073 2018-02-14 11:05:22 JST LOG:  received SIGHUP, reloading
> configuration files
> > 12073 2018-02-14 11:05:22 JST LOG:  parameter
> "standard_conforming_strings" changed to "off"
> > 12073 2018-02-14 11:05:22 JST DEBUG:  sending signal 1 to process 12158
> >
> > But as far as strace tells, nothing was sent to psql. Is this expected?
>
> It'll only get sent to the client the next time the server processes a
> query. We can't just at arbitrary points reload the config file or send
> messages out. The SIGHUP handler just sets ConfigReloadPending which
> PostgresMain() then processes:
>
> /*
>  * (6) check for any other interesting events that
> happened while we
>  * slept.
>  */
> if (ConfigReloadPending)
> {
> ConfigReloadPending = false;
> ProcessConfigFile(PGC_SIGHUP);
> }
>
> which'll then, in turn, send out ParameterStatus messages for changed
> GUC_REPORT GUCs.


 I was wondering a while ago - can't we just set our own proc's latch here,
so we wake up and send it earlier if we're in the idle main loop?

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

Re: Parameter status message not sent?

[Andres Freund]

On 2018-02-14 11:12:30 +0900, Tatsuo Ishii wrote:
> According to the manual, backend sends a parameter status message when
> certain configuration variable has been changed and SIGHUP signal is sent.
> https://www.postgresql.org/docs/10/static/protocol-flow.html#PROTOCOL-ASYNC
> 
>   ParameterStatus messages will be generated whenever the active value
>   changes for any of the parameters the backend believes the frontend
>   should know about. Most commonly this occurs in response to a SET
>   SQL command executed by the frontend, and this case is effectively
>   synchronous ― but it is also possible for parameter status changes
>   to occur because the administrator changed a configuration file and
>   then sent the SIGHUP signal to the server.
> 
> So I connected to PostgreSQL using psql and attached strace to psql.
> Then I changed standard_conforming_strings and executed pg_ctl
> reload. The PostgreSQL log shows:
> 
> 12073 2018-02-14 11:05:22 JST LOG:  received SIGHUP, reloading configuration 
> files
> 12073 2018-02-14 11:05:22 JST LOG:  parameter "standard_conforming_strings" 
> changed to "off"
> 12073 2018-02-14 11:05:22 JST DEBUG:  sending signal 1 to process 12158
> 
> But as far as strace tells, nothing was sent to psql. Is this expected?

It'll only get sent to the client the next time the server processes a
query. We can't just at arbitrary points reload the config file or send
messages out. The SIGHUP handler just sets ConfigReloadPending which
PostgresMain() then processes:

/*
 * (6) check for any other interesting events that happened 
while we
 * slept.
 */
if (ConfigReloadPending)
{
ConfigReloadPending = false;
ProcessConfigFile(PGC_SIGHUP);
}

which'll then, in turn, send out ParameterStatus messages for changed
GUC_REPORT GUCs.

Greetings,

Andres Freund

Parameter status message not sent?

[Tatsuo Ishii]

According to the manual, backend sends a parameter status message when
certain configuration variable has been changed and SIGHUP signal is sent.
https://www.postgresql.org/docs/10/static/protocol-flow.html#PROTOCOL-ASYNC

  ParameterStatus messages will be generated whenever the active value
  changes for any of the parameters the backend believes the frontend
  should know about. Most commonly this occurs in response to a SET
  SQL command executed by the frontend, and this case is effectively
  synchronous ― but it is also possible for parameter status changes
  to occur because the administrator changed a configuration file and
  then sent the SIGHUP signal to the server.

So I connected to PostgreSQL using psql and attached strace to psql.
Then I changed standard_conforming_strings and executed pg_ctl
reload. The PostgreSQL log shows:

12073 2018-02-14 11:05:22 JST LOG:  received SIGHUP, reloading configuration 
files
12073 2018-02-14 11:05:22 JST LOG:  parameter "standard_conforming_strings" 
changed to "off"
12073 2018-02-14 11:05:22 JST DEBUG:  sending signal 1 to process 12158

But as far as strace tells, nothing was sent to psql. Is this expected?

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp

Re: reorganizing partitioning code (was: Re: [HACKERS] path toward faster partition pruning)

[Amit Langote]

On 2018/02/13 23:08, Ashutosh Bapat wrote:
> On Tue, Feb 13, 2018 at 2:17 PM, Amit Langote
>  wrote:
>>
>> Agree with the proposed reorganizing and adding a partcache.c, which I
>> tried to do in the attached patch.
>>
>> * The new src/backend/utils/cache/partcache.c contains functions that
>> initialize relcache's partitioning related fields.  Various partition
>> bound comparison and search functions (and then some) that work off of the
>> cached information are moved.
>
> Are you moving partition bound comparison functions to partcache.c?
> They will also used by optimizer, so may be leave them out of
> partcache.c?

Yes, I moved the partition bound comparison and search functions, because
I thought that they should live with the other code that manages the
cached information.  So, I moved not only the code that reads the catalogs
and builds the partition key, partition (bound) descriptor, and partition
qual (for individual partitions), but also the code that uses those
structures.

So, with the new arrangement, optimizer will include utils/partcache.h,
instead of catalog/partition.h.

Thanks,
Amit

tapeblocks is uninitialized in logtape.c

[Jaime Casanova]

On 13 February 2018 at 21:07, Jaime Casanova
 wrote:
> Hi,
>
> Compiling with CFLAGS="-ggdb -Og -g3 -fno-omit-frame-pointer" as
> recommended in https://wiki.postgresql.org/wiki/Developer_FAQ#Compile-time
>
> My compiler gives me this message
>
> """
> logtape.c: In function ‘ltsConcatWorkerTapes’:
> logtape.c:462:48: warning: ‘tapeblocks’ may be used uninitialized in
> this function [-Wmaybe-uninitialized]
>   lts->nBlocksAllocated = lt->offsetBlockNumber + tapeblocks;
>   ~~^~~~
> """
>
> Can we please, initialize tapeblocks variable in
> ltsConcatWorkerTapes() function?
>

Sorry, i sent it without a subject...


-- 
Jaime Casanova  www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

[no subject]

[Jaime Casanova]

Hi,

Compiling with CFLAGS="-ggdb -Og -g3 -fno-omit-frame-pointer" as
recommended in https://wiki.postgresql.org/wiki/Developer_FAQ#Compile-time

My compiler gives me this message

"""
logtape.c: In function ‘ltsConcatWorkerTapes’:
logtape.c:462:48: warning: ‘tapeblocks’ may be used uninitialized in
this function [-Wmaybe-uninitialized]
  lts->nBlocksAllocated = lt->offsetBlockNumber + tapeblocks;
  ~~^~~~
"""

Can we please, initialize tapeblocks variable in
ltsConcatWorkerTapes() function?

-- 
Jaime Casanova  www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Missing comment edit

[Kyotaro HORIGUCHI]

Hello.

I happend to find that the comment on formdesc is missing
pg_subscription. Please find the attached patch (I'm sure:) to
fix that .

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index d5cc246..022a48c 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -1848,8 +1848,8 @@ LookupOpclassInfo(Oid operatorClassOid,
  *		catalogs.
  *
  * formrdesc is currently used for: pg_database, pg_authid, pg_auth_members,
- * pg_shseclabel, pg_class, pg_attribute, pg_proc, and pg_type
- * (see RelationCacheInitializePhase2/3).
+ * pg_shseclabel, pg_subscription, pg_class, pg_attribute, pg_proc, and
+ * pg_type (see RelationCacheInitializePhase2/3).
  *
  * Note that these catalogs can't have constraints (except attnotnull),
  * default values, rules, or triggers, since we don't cope with any of that.

Re: Parallel bt build crashes when DSM_NONE

[Kyotaro HORIGUCHI]

At Mon, 12 Feb 2018 22:05:36 +0900, Michael Paquier  wrote 
in <20180212130536.ga18...@paquier.xyz>
> On Fri, Feb 09, 2018 at 05:06:35PM +0900, Kyotaro HORIGUCHI wrote:
> 4 regression tests fail when using dynamic_shared_memory_type=none:
> join, aggregates, select_parallel and write_parallel.  test_shm_mq of
> course blows up.  Could that justify getting rid of DSM_IMPL_NONE?  As

A query runs into the fallback route in the case, which even
though the regtest doesn't care about. So it alone doesn't
justify that.

> far as I can see there is an alternative on Windows, and we fallback to
> sysv in the worst case.  So I am wondering what's actually the use case
> for "none".  And it is good to keep alternate outputs at a minimum,
> those tend to rot easily.

Agreed. As Tom mentioned, no bf animal haven't complained with
that since 9.4 and I belive they won't. initdb doesn't create a
database with DSM_IMPL_NONE. Although it can be manually
deactivated, the fact that we haven't have a complain about that
can justify to remove it to some extent. I'll post a patch that
eliminate DSM_IMPL_NONE after this. I'd like it to be shipped on
11.

> Except for those mind errands your patch looks good to me.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: Server won't start with fallback setting by initdb.

[Kyotaro HORIGUCHI]

Hello,

At Mon, 12 Feb 2018 22:28:15 +0900, Michael Paquier  wrote 
in <20180212132815.gb18...@paquier.xyz>
> On Fri, Feb 09, 2018 at 05:08:23PM +0900, Kyotaro HORIGUCHI wrote:
> > > postgres: max_wal_senders must be less than max_connections
> > 
> > I think that we can safely increase the fallback value to 20 with
> > which regtests are known not to fail. I believe that is
> > preferable than explicitly reducing max_wal_senders in the
> > generated config file. I confirmed that tegtest won't fail with
> > the value. (Except with permanent failure of dynamic shared
> > memory)
> 
> I would vote for just removing the minimum check at 10 connections as
> you do.  It is not worth the code complication in initdb to decrease
> max_wal_senders if max_connections is set to 10, which does not happen

Definitely. The another rationale for the value is that regtest
fails with the numbers less than 20. So it's not 11 but
20. Currently regtest should succeed with that number of
connections as written in parallel_schedule and I've read (in
Robert's mail, but I haven't confirmed) that tests for parallel
scans are designed to use up to 20 connections.

> even on definitely-not-decent hardware of those days like a RPI
> (memories from my hamster, RIP, which set max_connections to 100). 

I believe it is the minimal box where anyone casulally try to run
PostgreSQL as is.

regareds,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: reorganizing partitioning code

[Amit Langote]

On 2018/02/13 22:23, Alvaro Herrera wrote:
> Thanks for working on this.  May I suggest to open a completely new
> thread?

Done.

Thanks,
Amit

Re: Parallel bt build crashes when DSM_NONE

[Kyotaro HORIGUCHI]

At Mon, 12 Feb 2018 15:00:54 -0500, Robert Haas  wrote 
in 
> On Mon, Feb 12, 2018 at 12:26 PM, Peter Geoghegan  wrote:
> > I think that your patch does the right thing.
> > plan_create_index_workers() is supposed to take care of parallel
> > safety, and this is a parallel safety issue.
> 
> Committed the patch.

Thank you Robert for Committing, and Peter and Michael for the
comments and supplement.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: master plpython check fails on Solaris 10

[Peter Eisentraut]

On 2/13/18 05:40, Marina Polyakova wrote:
> Binary search has shown that this failure begins with commit 
> 8561e4840c81f7e345be2df170839846814fa004 (Transaction control in PL 
> procedures.). On the previous commit 
> (b9ff79b8f17697f3df492017d454caa9920a7183) there's no 
> plpython_transaction test and plpython check passes.

OK, can you get some kind of stack trace or other debugging information?

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

Re: Add more information_schema columns

[Tom Lane]

Andres Freund  writes:
> Do we have a policy about catversion bumps for information schema
> changes? A cluster from before this commit fails the regression tests
> after the change, but still mostly works...

I think historically we've not bumped catversion, on the grounds that
there's no incompatibility with the backend as such.  However, it is
kind of annoying that not updating means the regression tests fail.
Informally, I'm sure most developers take "catversion bump" to mean
"you need to initdb".  So I'd support saying that an information_schema
change should include a catversion bump if it involves any changes in
regression test results.

regards, tom lane

Re: Add more information_schema columns

[Andres Freund]

On 2018-02-07 10:50:12 -0500, Peter Eisentraut wrote:
> On 2/7/18 00:14, Michael Paquier wrote:
> > On Tue, Feb 06, 2018 at 10:45:52PM -0500, Peter Eisentraut wrote:
> >> I think what I had meant to write was something like
> >>
> >> (t.tgtype & (1 | 66))
> >>
> >> but maybe it's clearer to write it all out as you did.
> > 
> > If you prefer that, that's fine for me as well.  I tend to prefer the
> > formulation where both expressions are separated to make clearer that
> > ordering needs to be split for all three characteristics.
> 
> Committed with the separate entries.

Do we have a policy about catversion bumps for information schema
changes? A cluster from before this commit fails the regression tests
after the change, but still mostly works...

Greetings,

Andres Freund

Re: [HACKERS] Client Connection redirection support for PostgreSQL

[Tom Lane]

Robert Haas  writes:
> -- might need some defense against the redirected-to server getting
> the same password as was sent to the original server.  Is that a
> security risk?  Does HTTP have a rule about this?

Without having read any of the previous discussion ... I'd say that if the
redirect info is placed in pg_hba.conf then I would expect a redirect to
happen before any authentication exchange, so that this is not an issue.
Perhaps it would be a good security measure for clients to refuse a
redirect once they've sent any auth-related messages.

But ... pg_hba.conf?  Really?  Surely that is a completely random and
inappropriate place to control redirection?

regards, tom lane

TODO item: WAL replay of CREATE TABLESPACE with differing directory structure

[Patrick Krecker]

Hi Hackers --

I am searching for a way to make a contribution to Postgres and I came
across this TODO item (I realize there has been some controversy
around the TODO list [1], and I hope that my use of it doesn't spark
another discussion about removing it altogether):

"Allow WAL replay of CREATE TABLESPACE to work when the directory
structure on the recovery computer is different from the original"

Currently it looks like tablespaces have to live inside the data
directory on the replica, notwithstanding administrator intervention
by manipulating the tablespace directory with symlinks after (or even
before?) it has been created via replay.

Is the idea behind this task to allow the master to instruct the
replica where to put the tablespace on its filesystem, so as to allow
it to live outside of the data directory without direct manipulation
of the filesystem?

If this task is a worthwhile endeavor, I would be happy to take it on.
If not, I am open to other ideas :)

Thanks,
Patrick

[1] 
https://www.postgresql.org/message-id/CA+TgmoZC3CyzZDY1fWChRNOY-5SBjUkB1w=c6y6jmqhtavu...@mail.gmail.com

Python 3.7 support

[Peter Eisentraut]

A small patch to tweak the tests to support output differences with
Python 3.7 (currently in beta).

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From 543840cbe29653c18cce8d1ab4af4daac5c4899b Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Tue, 13 Feb 2018 16:13:20 -0500
Subject: [PATCH] Tweak tests to support Python 3.7

Python 3.7 removes the trailing comma in the repr() of
BaseException (see ), leading to
test output differences.  Work around that by composing the equivalent
test output in a more manual way.
---
 src/pl/plpython/expected/plpython_subtransaction.out | 6 +++---
 src/pl/plpython/sql/plpython_subtransaction.sql  | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/pl/plpython/expected/plpython_subtransaction.out 
b/src/pl/plpython/expected/plpython_subtransaction.out
index da3b312a06..b38cde8d2d 100644
--- a/src/pl/plpython/expected/plpython_subtransaction.out
+++ b/src/pl/plpython/expected/plpython_subtransaction.out
@@ -134,7 +134,7 @@ with plpy.subtransaction():
 except plpy.SPIError, e:
 if not swallow:
 raise
-plpy.notice("Swallowed %r" % e)
+plpy.notice("Swallowed %s(%r)" % (e.__class__.__name__, e.args[0]))
 return "ok"
 $$ LANGUAGE plpythonu;
 SELECT subtransaction_nested_test();
@@ -153,7 +153,7 @@ SELECT * FROM subtransaction_tbl;
 
 TRUNCATE subtransaction_tbl;
 SELECT subtransaction_nested_test('t');
-NOTICE:  Swallowed SyntaxError('syntax error at or near "error"',)
+NOTICE:  Swallowed SyntaxError('syntax error at or near "error"')
  subtransaction_nested_test 
 
  ok
@@ -178,7 +178,7 @@ with plpy.subtransaction():
 return "ok"
 $$ LANGUAGE plpythonu;
 SELECT subtransaction_deeply_nested_test();
-NOTICE:  Swallowed SyntaxError('syntax error at or near "error"',)
+NOTICE:  Swallowed SyntaxError('syntax error at or near "error"')
  subtransaction_deeply_nested_test 
 ---
  ok
diff --git a/src/pl/plpython/sql/plpython_subtransaction.sql 
b/src/pl/plpython/sql/plpython_subtransaction.sql
index 3c188e3dd2..398c65720c 100644
--- a/src/pl/plpython/sql/plpython_subtransaction.sql
+++ b/src/pl/plpython/sql/plpython_subtransaction.sql
@@ -80,7 +80,7 @@ CREATE FUNCTION subtransaction_nested_test(swallow boolean = 
'f') RETURNS text
 except plpy.SPIError, e:
 if not swallow:
 raise
-plpy.notice("Swallowed %r" % e)
+plpy.notice("Swallowed %s(%r)" % (e.__class__.__name__, e.args[0]))
 return "ok"
 $$ LANGUAGE plpythonu;
 
-- 
2.16.1

Re: spelling of enable_partition_wise_join

[Gavin Flower]

On 14/02/18 09:27, Peter Eisentraut wrote:

I wonder how others feel about this, but the spelling of
enable_partition_wise_join feels funny to me every time I look at it.  I
would write it enable_partitionwise_join.

Thoughts?

As 'wise' is a suffix, not a word in this situation - so it be combined 
with 'partition'.


Hence 'enable_partitionwise_join' is more correct.


Cheers,
Gavin

Re: spelling of enable_partition_wise_join

[Alvaro Herrera]

Peter Eisentraut wrote:
> I wonder how others feel about this, but the spelling of
> enable_partition_wise_join feels funny to me every time I look at it.  I
> would write it enable_partitionwise_join.

+1

See also: https://postgr.es/m/20171005134847.shzldz2ublrb3ny2@alvherre.pgsql

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

Re: CALL stmt, ERROR: unrecognized node type: 113 bug

[Robert Haas]

On Mon, Feb 12, 2018 at 3:19 PM, Tom Lane  wrote:
> modulo that I still don't like prorettype == 0 ;-).

+1.

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

spelling of enable_partition_wise_join

[Peter Eisentraut]

I wonder how others feel about this, but the spelling of
enable_partition_wise_join feels funny to me every time I look at it.  I
would write it enable_partitionwise_join.

Thoughts?

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

Re: CALL stmt, ERROR: unrecognized node type: 113 bug

[Peter Eisentraut]

On 2/13/18 03:57, Michael Paquier wrote:
> On Mon, Feb 12, 2018 at 03:19:27PM -0500, Tom Lane wrote:
>> I've not read in detail, but it looks reasonable offhand, modulo
>> that I still don't like prorettype == 0 ;-).
>>
>> I did notice a tiny typo:
>>
>> - * with.  Hence prefer "$function$", but extend if needed.
>> + * with.  Hence prefer "$function$"/"$procedure", but extend if needed.
>>
>> I think you want
>>
>> + * with.  Hence prefer "$function$"/"$procedure$", but extend if needed.

done

> 0001 and 0002 are welcome.  I have a small comment on top of Tom's for 0003.
> 
> +   appendStringInfoString(, ")\n");
> +   if (proc->prorettype)
> +   {
> +   appendStringInfoString(, " RETURNS ");
> +   print_function_rettype(, proctup);
> +   appendStringInfoChar(, '\n');
> +   }
> Could you use a separate boolean variable which is set as
> OidIsValid(prorettype), say called isfunction?

done

> Should the documentation of pg_function_is_visible also mention
> procedures?

done

and committed

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

Re: [HACKERS] Client Connection redirection support for PostgreSQL

[Robert Haas]

On Mon, Feb 12, 2018 at 1:56 PM, Satyanarayana Narlapuram
 wrote:
> I simplified the patch and for now just allowed one server. Please find the 
> attached patches, and the commit message.

This patch --

-- doesn't include nearly sufficient documentation updates.  For
example, the new message type is not documented in the list of message
types.  The documentation of which messages are legal in which
contexts is not updated to mention this new message.  The new
ConnStatusType is not documented (and is it really needed?).  The
documentation for the new pg_hba.conf parameter does not explain how
to specify the alternate server to which you wish to connect.

-- includes no tests.

-- does include irrelevant whitespace differences.

-- doesn't include any provision for clients to fall back to 3.0 if
3.1 is not supported.

-- doesn't seem to have proper provisions for the server to handle
older clients.  The code looks like just skips over hba.conf redirect
lines if the client is older, which seems like not what we want.  The
proposed commit message claims we just go ahead and send redirects to
older clients that aren't expecting them, which is pretty much missing
the entire point of having minor protocol versions.  I think the right
way to handle this case is to throw FATAL with the error text
suggesting the host/port to which the user should try connecting.

-- probably needs defenses against infinite redirect loops.  Most
likely we should see how this is normally handled by HTTP clients and
do something similar here.

-- probably needs some way for clients to express unwillingness to
follow redirects.  Possibly that could be combined with the previous
item by having a new connection parameter indicating the number of
redirects the client is willing to follow, with the default being,
say, 10 (browsers apparently have a limit of 10 or 20 for HTTP, but 20
seems overly generous for a database connection) and 0 disabling.

-- might need some defense against the redirected-to server getting
the same password as was sent to the original server.  Is that a
security risk?  Does HTTP have a rule about this?

-- might need some way for clients to discover whether they got
redirected and, if so, the server to which they were redirected.  For
example, if I connect with psql, get redirected, and then type
\conninfo, do I get the information on the server to which I think I
connected, or the server to which I got redirected?  Maybe we should
display both?  If the connection gets retried, do we retry the
original server or the server to which we were redirected?  I'd argue
for the former, but maybe other people think differently.

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

Re: [WIP PATCH] Index scan offset optimisation using visibility map

[Tom Lane]

Michail Nikolaev  writes:
> Still not sure about comment formatting. Have you seen any style guid about
> it except "strict ANSI C comment formatting"? Anyway still need to work on
> comments.

The short answer is "make the new code look like the code around it".
But there is actually documentation on the point:

https://www.postgresql.org/docs/current/static/source-format.html

The rest of that chapter is recommendable reading as well.

regards, tom lane

Re: [GSoC Idea Discussion] "Thrift datatype support" Project

[Bear Giles]

On Tue, Feb 13, 2018 at 10:24 AM, Vladimir Sitnikov <
sitnikov.vladi...@gmail.com> wrote:

> Technically speaking, Thrift is "language-independent"
> serialization-deserialization format with clean approach to backward
> compatibility.
>
> I think Thrift (or something like that) can be useful, as it can generate
> serializers/deserializers for lots of languages.
> PostgreSQL's "binary" format is tied to the PostgreSQL and it has to be
> reimplemented for each and every client language.
>
> "text" format is non-trivial as well (e.g. it is hard to get quoting right
> for structs, and text is server-locale-dependent)
>
> Vladimir
>

​​Thanks. I've been helping a coworker with an older parquet writer and
​the URL for the hive metastore uses a thrift:// scheme. That is why I
thought it is a communications protocol.​

Re: Converting plpgsql to use DTYPE_REC for named composite types

[Tom Lane]

Pavel Stehule  writes:
> 2017-12-30 0:16 GMT+01:00 Tom Lane :
>>> I'll stick this into the January commitfest, but I'd like to get it
>>> reviewed and committed pretty soon, because there are follow-on patches
>>> that need to get done in time for v11 --- in particular, we need to close
>>> out the lack of plpgsql support for domains-over-composite.

> all test passed on my comp too.

Is anyone planning on doing further review of this patch than Pavel
already did?  If not, I'd like to go ahead and push it, since there's
still followup stuff that I'd like to address for v11.  As noted upthread,
there are places where plpgsql fails to enforce DOMAIN NOT NULL
constraints, and I think I had some other squishy things in my notes.

regards, tom lane

Re: [GSoC Idea Discussion] "Thrift datatype support" Project

[Vladimir Sitnikov]

Technically speaking, Thrift is "language-independent"
serialization-deserialization format with clean approach to backward
compatibility.

I think Thrift (or something like that) can be useful, as it can generate
serializers/deserializers for lots of languages.
PostgreSQL's "binary" format is tied to the PostgreSQL and it has to be
reimplemented for each and every client language.

"text" format is non-trivial as well (e.g. it is hard to get quoting right
for structs, and text is server-locale-dependent)

Vladimir

Re: Cached/global query plans, autopreparation

[Shay Rojansky]

Hi all,

Was wondering if anyone has a reaction to my email below about statement
preparation, was it too long? :)

(and sorry for top-posting)

On Tue, Feb 6, 2018 at 9:27 PM, Shay Rojansky  wrote:

> Hi all.
>
> Various versions of having PostgreSQL caching and/or autopreparing
> statement plans have been discussed (https://www.postgresql.org/
> message-id/op.t9ggb3wacigqcu%40apollo13.peufeu.com, https:/
> /www.postgresql.org/message-id/8e76d8fc-8b8c-14bd-d4d1-
> e9cf193a74f5%40postgrespro.ru), without clear conclusions or even an
> agreement on what might be worthwhile to implement. I wanted to bring this
> up again from a PostgreSQL driver maintainer's perspective (I'm the owner
> of Npgsql, the open source .NET driver), apologies in advance if I'm
> repeating things or I've missed crucial information. Below I'll describe
> three relevant issues and what I've done to deal with them.
>
> When the same statement is rerun, preparing it has a very significant
> performance boost. However, in short-lived connection scenarios it's
> frequently not possible to benefit from this - think of a typical webapp
> which allocates a connection from a pool, run a query and then return the
> connection. To make sure prepared statements are used, Npgsql's connection
> pool doesn't send DISCARD ALL when a connection is returned (to avoid
> wiping out the connections), and maintains an internal table mapping SQL
> (and parameter types) to a PostgreSQL statement name. The next time the
> application attempts to prepare the same SQL, the prepared statement is
> found in the table and no preparation needs to occur. This means that
> prepared statements persist across pooled connection open/close, and are
> never discarded unless the user uses a specific API. While this works, the
> disadvantages are that:
> 1. This kind of mechanism needs to be implemented again and again, in each
> driver:
> 2. It relies on Npgsql's internal pooling, which can track persistent
> prepared statements on physical connections. If an external pool is used
> (e.g. pgpool), this isn't really possible.
> 1. It complicates resetting the session state (instead of DISCARD ALL, a
> combination of all other reset commands except DEALLOCATE ALL needs be
> sent). This is minor.
>
> The second issue is that many applications don't work directly against the
> database API (ADO.NET in .NET, JDBC in Java). If any sort of O/RM or
> additional layer is used, there's a good chance that that layer doesn't
> prepare in any way, and indeed hide your access to the database API's
> preparation method. Two examples from the .NET world is dapper (a very
> popular micro-O/RM) and Entity Framework. In order to provide the best
> possible performance in these scenarios, Npgsql has an opt-in feature
> whereby it tracks how many times a given statement was executed, and once
> it passes a certain threshold automatically prepares it. An LRU cache is
> then used to determine which prepared statements to discard, to avoid
> explosion. In effect, statement auto-preparation is implemented in the
> driver. I know that the JDBC driver also implements such a mechanism (it
> was actually the inspiration for the Npgsql feature). The issues with this
> are:
>
> 1. As above, this has to be implemented by every driver (and is quite
> complex to do well)
> 2. There's a possible missed opportunity in having a single plan on the
> server, as each connection has its own (the "global plan" option). Many
> apps out there send the same statements across many connections so this
> seems relevant - but I don't know if the gains outweigh the contention
> impact in PostgreSQL.
>
> Finally, since quite a few (most?) other databases include autopreparation
> (SQL Server, Oracle...), users porting their applications - which don't
> explicitly prepare - experience a big performance drop. It can rightly be
> said that porting an application across databases isn't a trivial task and
> that adjustments need to be made, but from experience I can say that
> PostgreSQL is losing quite a few users to this.
>
> The above issues could be helped by having PostgreSQL cache on its side
> (especially the second issue, which is the most important). Ideally, any
> adopted solution would be transparent and not require any modification to
> applications. It would also not impact explicitly-prepared statements in
> any way.
>
> Note that I'm not arguing for any specific implementation on the
> PostgreSQL side (e.g. global or not), but just describing a need and hoping
> to restart a conversation that will lead somewhere.
>
> (and thanks for reading this overly long message!)
>
> Shay
>

Re: [GSoC Idea Discussion] "Thrift datatype support" Project

[Bear Giles]

Isn't thrift the communications protocol?

Do we have foreign server support for parquet and ORC files?

On Tue, Feb 13, 2018 at 8:40 AM, Udit Juneja  wrote:

> Hi,
>
> I am Udit Juneja, a Computer Science undergraduate student at Thapar
> Institute of Engineering and Technology, India. I am interested in
> contributing to PostgreSQL.
>
> A brief introduction about me:
> I am familiar with programming languages (C, C++, Python, SQL).
>
> I am interested in "Thrift datatype support"  project. I have already
> started exploring PostgreSQL, and the libraries involved in this project.
>
> I would like to discuss the idea further and maybe submit a proposal in
> GSoC later.
>
> Regards,
> Udit Juneja
>

Re: GSoC 2018 Project Ideas & Mentors - Last Call

[Andrey Borodin]

Hi!

> 18 янв. 2018 г., в 21:59, Stephen Frost  написал(а):
> 
> I'll be submitting PostgreSQL as an organization for GSoC 2018 soon and
> look forward to having another great PostgreSQL GSoC!

I've just checked GSoC website. PostgreSQL is accepted as an organization this 
year.
https://summerofcode.withgoogle.com/organizations/6357583019900928/ 


Time to attract some great students :)

Best regards, Andrey Borodin.

[GSoC Idea Discussion] "Thrift datatype support" Project

[Udit Juneja]

Hi,

I am Udit Juneja, a Computer Science undergraduate student at Thapar
Institute of Engineering and Technology, India. I am interested in
contributing to PostgreSQL.

A brief introduction about me:
I am familiar with programming languages (C, C++, Python, SQL).

I am interested in "Thrift datatype support"  project. I have already
started exploring PostgreSQL, and the libraries involved in this project.

I would like to discuss the idea further and maybe submit a proposal in
GSoC later.

Regards,
Udit Juneja

Re: Using scalar function as set-returning: bug or feature?

[Tom Lane]

"Tels"  writes:
> On Mon, February 12, 2018 5:03 pm, Tom Lane wrote:
>> ... However, my pending patch at
>> https://commitfest.postgresql.org/17/1439/
>> gets rid of the use of DTYPE_ROW for composite types, and once that
>> is in it might well be reasonable to just throw a flat-out error for
>> wrong number of source values for a DTYPE_ROW target.  I can't
>> immediately think of any good reason why you'd want to allow for
>> the number of INTO items not matching what the query produces.

> Perl lets you set a fixed number of multiple variables from an array and
> discard the rest like so:
>   my ($a, $b) = (1,2,3);
> I'm not sure if you mean exactly the scenario as in the attached test
> case, but this works in plpgsql, too, and would be a shame to lose.

Well, that's exactly the issue.  Whether that's a handy feature or
a foot-gun that hides bugs depends entirely on your point of view.
Personally, this is not the kind of behavior I really want from a
programming language ;-).  And I'm sure that if plpgsql were still
enforcing the old rules, and someone came along with a proposal to
relax that to be more like Perl, it'd be laughed down.

Still, backwards compatibility is worth something.  I don't really
have a strong opinion about whether to change it or not.

regards, tom lane

Re: [Sender Address Forgery]Re: [Sender Address Forgery]Re: [Sender Address Forgery]Re: [HACKERS] path toward faster partition pruning

[Alvaro Herrera]

David Rowley wrote:
> On 19 January 2018 at 16:00, Kyotaro HORIGUCHI
>  wrote:
> > And I'd like to ask David to check out his mail environment so
> > that SPF record is available for his message.
> 
> Will investigate

This should be fixed now.  Please let us know if you still see problems.

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

Re: [HACKERS] Partition-wise aggregation/grouping

[Jeevan Chalke]

On Tue, Feb 13, 2018 at 12:37 PM, Rafia Sabih 
wrote:

>
> I was testing this patch for TPC-H benchmarking and came across following
> results,
>

Thanks Rafia for testing this with TPC-H benchmarking.


>
> Q1 completes in 229 secs with patch and in 66 secs without it. It looks
> like with this patch the time of parallel seq scan itself is elevated for
> some of the partitions. Notice for partitions, lineitem_3, lineitem_7,
> lineitem_10, and linietem_5 it is some 13 secs which was somewhere around 5
> secs on head.
>

> Q6 completes in some 7 secs with patch and it takes 4 secs without it.
> This is mainly caused because with the new parallel append, the parallel
> operator below it (parallel index scan in this case) is not used, however,
> on head it was the append of all the parallel index scans, which was saving
> quite some time.
>

I see that partition-wise aggregate plan too uses parallel index, am I
missing something?


>
> Q18 takes some 390 secs with patch and some 147 secs without it.
>

This looks strange. This patch set does not touch parallel or seq scan as
such. I am not sure why this is happening. All these three queries explain
plan shows much higher execution time for parallel/seq scan.

However, do you see similar behaviour with patches applied,
"enable_partition_wise_agg = on" and "enable_partition_wise_agg = off" ?

Also, does rest of the queries perform better with partition-wise
aggregates?


>
> The experimental setup for these tests is as follows,
> work_mem = 500MB
> shared_buffers = 10GB
> effective_cache_size = 4GB
> seq_page_cost = random+page_cost = 0.01
> enable_partition_wise_join = off
>
> Partitioning info:
> Total 10 partitions on tables - lineitem and orders each with partitioning
> key being l_orderkey and o_orderkey respectively.
>
> Please find the attached file for explain analyse outputs of each of the
> reported query.
> --
> Regards,
> Rafia Sabih
> EnterpriseDB: http://www.enterprisedb.com/
>



-- 
Jeevan Chalke
Technical Architect, Product Development
EnterpriseDB Corporation
The Enterprise PostgreSQL Company

Re: Expression errors with "FOR UPDATE" and postgres_fdw with partition wise join enabled.

[Ashutosh Bapat]

On Mon, Feb 12, 2018 at 6:00 PM, Rajkumar Raghuwanshi
 wrote:
> Hi,
>
> I am getting "ERROR:  unexpected expression in subquery output" and "ERROR:
> variable not found in subplan target lists" errors, for "FOR UPDATE" with
> postgres_fdw. (when set enable_partition_wise_join to true);
>
> Attached patch have queries which are throwing mentioned error on running
> make check in contrib/postgres_fdw folder.
>

Thanks a lot for the patch. I can apply it and reproduce the error.

Here's my analysis of the bug.

The node for which this error comes is a ConvertRowtypeExpr node with
Var::varattno = 0 under it. Whole row reference of the parent is converted to
ConvertRowtypeExpr with whole row of child as an argument. When partition-wise
join is used, targetlist of child-joins contain such ConvertRowtypeExpr when
the parent-join's targetlist has whole-row references of joining
partitioned tables.

When we deparse the targetlist of join pushed down by postgres FDW,
build_tlist_to_deparse() pulls only Var nodes nodes from the join's targetlist.
So it pulls Var reprensenting a whole-row reference of a child from a
ConvertRowtypeExpr, when building targetlist to be deparsed for a child-join
with whole-row references. This targetlist is then saved as fdw_scan_tlist in
ForeignScanPlan.

This causes two problems shown by the two queries below


>
> EXPLAIN (VERBOSE, COSTS OFF)
> SELECT t1.c1, ss.a, ss.b FROM (SELECT c1 FROM pt1  WHERE c1 = 50) t1 INNER
> JOIN (SELECT t2.c1, t3.c1 FROM (SELECT c1 FROM pt1  WHERE c1 between 50 and
> 60) t2 FULL JOIN (SELECT c1 FROM pt2 WHERE c1 between 50 and 60) t3 ON
> (t2.c1 = t3.c1) WHERE t2.c1 IS NULL OR t2.c1 IS NOT NULL) ss(a, b) ON (TRUE)
> ORDER BY t1.c1, ss.a, ss.b FOR UPDATE OF t1;
> ERROR:  unexpected expression in subquery output

get_relation_column_alias_ids() uses foreignrel's targetlist
(foreignrel->reltarget->exprs) as it is to locate given node to be deparsed.
If the joining relation corresponding to ConvertRowtypeExpr is deparsed as a
subquery, this function is called with whole-row reference node (Var node with
varattno = 0).  But the relation's targetlist doesn't contain its whole-row
reference directly but has it embedded in ConvertRowtypeExpr. So, the function
doesn't find the given node and throws error.


>
> EXPLAIN (VERBOSE, COSTS OFF)
> SELECT t1.c1, ss.a, ss.b FROM (SELECT c1 FROM pt1) t1 INNER JOIN (SELECT
> t2.c1, t3.c1 FROM (SELECT c1 FROM pt1) t2 FULL JOIN (SELECT c1 FROM pt1) t3
> ON (t2.c1 = t3.c1)) ss(a, b) ON (TRUE) ORDER BY t1.c1, ss.a, ss.b FOR UPDATE
> OF t1;
> ERROR:  variable not found in subplan target lists

When there is possibility of EvalPlanQual being called, we construct local
join plan matching the pushed down foreign join. In postgresGetForeignPlan()
after we have built the local join plan, the topmost plan node's targetlist is
changed to fdw_scan_tlist to match the output of the ForeignScan node. As
explained above, this targetlist contains a bare reference to whole-row
reference of a child relation if the child-join's targetlist contains a
ConvertRowtypeExpr. When changing the topmost plan node's targetlist, we do not
modify the targetlists of its left and right tree nodes. The left/right plan
involving corresponding child relation will have ConvertRowtypeExpr expression
in its targetlist, but not whole-row reference directly. When the topmost local
join plan node's targetlist is processed by set_plan_ref(), it throws error
"variable not found in subplan target lists" since it doesn't find bare
whole-row reference of the child relation in subplan's targetlists.

The problem can be solved in two ways:

1. Push down ConvertRowtypeExpr and include it in the pushed down targetlist.
This would solve both the problems described above. Both set_plan_ref() and
get_relation_column_alias_ids() will find ConvertRowtypeExpr, they are looking
for and won't throw an error.

This requires two parts
a. In build_tlist_to_deparse(), instead of pulling
Var node from ConvertRowtypeExpr, we pull whole ConvertRowtypeExpr and include
it in the targetlist being deparsed which is also used as to set
fdw_scan_tlist. In order to pull any ConvertRowtypeExpr's in the local quals,
which may be hidden in the expression tree, we will add two more options to
flags viz. PVC_INCLUDE_CONVERTROWTYPEEXPR and PVC_RECURSE_CONVERTROWTYPEEXPR.
Unlike the other PVC_* options, which do not default to a value, we may want to
default to PVC_RECURSE_CONVERTROWTYPEEXPR if nothing is specified. That will
avoid, possibly updating every pull_var_clause call with
PVC_RECURSE_CONVERTROWTYPEEXPR.
b. deparse ConvertRowtypeExpr
For this we need to get the conversion map between the parent and child. We
then deparse ConvertRowtypeExpr as a ROW() with the attributes of child
rearranged per the conversion map. A multi-level partitioned table will have
nested ConvertRowtypeExpr. To deparse such expressions, we need to find the
conversion map between the 

Re: postgres_fdw: perform UPDATE/DELETE .. RETURNING on a join directly

[Etsuro Fujita]

(2018/02/11 6:24), Tom Lane wrote:

However, jaguarundi still shows a problem:

https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=jaguarundi=2018-02-10%2008%3A41%3A32

(previous run similar, so it's semi-reproducible even after this patch).
jaguarundi uses -DCLOBBER_CACHE_ALWAYS, so you might try a few repetitions
with that.


I ran the postgres_fdw regression test with no sleep two times in a 
CLOBBER_CACHE_ALWAYS-enabled build, and then the regression test with 
the sleep (60 seconds) two times, but I couldn't reproduce that in both 
cases.  I suspect the changes in the order of the RETURNING output there 
was still caused by autovacuum kicking in partway through the run.  So 
to make the regression test more stable against autovacuum, I'd propose 
to modify the regression test to disable autovacuum for the remote table 
(ie "S 1"."T 1") (and perform VACUUM ANALYZE to that table manually 
instead) in hopes of getting that fixed.  Attached is a patch for that. 
 I think changes added by the previous patch wouldn't make sense 
anymore, so I removed those changes.


Best regards,
Etsuro Fujita
*** a/contrib/postgres_fdw/expected/postgres_fdw.out
--- b/contrib/postgres_fdw/expected/postgres_fdw.out
***
*** 35,40  CREATE TABLE "S 1"."T 1" (
--- 35,43 
  	c8 user_enum,
  	CONSTRAINT t1_pkey PRIMARY KEY ("C 1")
  );
+ -- "S 1"."T 1" will be heavily updated below, so disable autovacuum for
+ -- the table to avoid unexpected effects of that
+ ALTER TABLE "S 1"."T 1" SET (autovacuum_enabled = 'false');
  CREATE TABLE "S 1"."T 2" (
  	c1 int NOT NULL,
  	c2 text,
***
*** 4244,4253  explain (verbose, costs off) select * from ft3 f, loct3 l
  -- ===
  -- test writable foreign table stuff
  -- ===
- -- Autovacuum on the remote side might affect remote estimates,
- -- so use local stats on ft2 as well
- ALTER FOREIGN TABLE ft2 OPTIONS (SET use_remote_estimate 'false');
- ANALYZE ft2;
  EXPLAIN (verbose, costs off)
  INSERT INTO ft2 (c1,c2,c3) SELECT c1+1000,c2+100, c3 || c3 FROM ft2 LIMIT 20;
  QUERY PLAN
--- 4247,4252 
***
*** 5524,  UPDATE ft2 SET c3 = 'baz'
FROM ft4 INNER JOIN ft5 ON (ft4.c1 = ft5.c1)
WHERE ft2.c1 > 2000 AND ft2.c2 === ft4.c1
RETURNING ft2.*, ft4.*, ft5.*;-- can't be pushed down
! QUERY PLAN 
! ---
   Update on public.ft2
 Output: ft2.c1, ft2.c2, ft2.c3, ft2.c4, ft2.c5, ft2.c6, ft2.c7, ft2.c8, ft4.c1, ft4.c2, ft4.c3, ft5.c1, ft5.c2, ft5.c3
 Remote SQL: UPDATE "S 1"."T 1" SET c3 = $2 WHERE ctid = $1 RETURNING "C 1", c2, c3, c4, c5, c6, c7, c8
!->  Hash Join
   Output: ft2.c1, ft2.c2, NULL::integer, 'baz'::text, ft2.c4, ft2.c5, ft2.c6, ft2.c7, ft2.c8, ft2.ctid, ft4.*, ft5.*, ft4.c1, ft4.c2, ft4.c3, ft5.c1, ft5.c2, ft5.c3
!  Hash Cond: (ft4.c1 = ft5.c1)
   ->  Foreign Scan
!Output: ft2.c1, ft2.c2, ft2.c4, ft2.c5, ft2.c6, ft2.c7, ft2.c8, ft2.ctid, ft4.*, ft4.c1, ft4.c2, ft4.c3
!Filter: (ft2.c2 === ft4.c1)
!Relations: (public.ft2) INNER JOIN (public.ft4)
!Remote SQL: SELECT r1."C 1", r1.c2, r1.c4, r1.c5, r1.c6, r1.c7, r1.c8, r1.ctid, CASE WHEN (r2.*)::text IS NOT NULL THEN ROW(r2.c1, r2.c2, r2.c3) END, r2.c1, r2.c2, r2.c3 FROM ("S 1"."T 1" r1 INNER JOIN "S 1"."T 3" r2 ON (((r1."C 1" > 2000 FOR UPDATE OF r1
!->  Nested Loop
!  Output: ft2.c1, ft2.c2, ft2.c4, ft2.c5, ft2.c6, ft2.c7, ft2.c8, ft2.ctid, ft4.*, ft4.c1, ft4.c2, ft4.c3
!  ->  Foreign Scan on public.ft2
!Output: ft2.c1, ft2.c2, ft2.c4, ft2.c5, ft2.c6, ft2.c7, ft2.c8, ft2.ctid
!Remote SQL: SELECT "C 1", c2, c4, c5, c6, c7, c8, ctid FROM "S 1"."T 1" WHERE (("C 1" > 2000)) FOR UPDATE
   ->  Foreign Scan on public.ft4
 Output: ft4.*, ft4.c1, ft4.c2, ft4.c3
 Remote SQL: SELECT c1, c2, c3 FROM "S 1"."T 3"
!  ->  Hash
!Output: ft5.*, 

Re: Using scalar function as set-returning: bug or feature?

[Marko Tiikkaja]

On Tue, Feb 13, 2018 at 12:30 PM, Tels 
wrote:

> I'm not sure if you mean exactly the scenario as in the attached test
> case, but this works in plpgsql, too, and would be a shame to lose.
>
> OTOH, one could also write:
>
>   SELECT INTO ba, bb  a,b FROM foo(1);
>
> and it would still work, or wouldn't it?
>

Yes.  The code in test.psql shouldn't pass code review.


.m

Re: Using scalar function as set-returning: bug or feature?

[Tels]

Moin Tom,

On Mon, February 12, 2018 5:03 pm, Tom Lane wrote:
> Pavel Stehule  writes:
>> 2018-02-09 12:02 GMT+01:00 Marko Tiikkaja :
>>> This is quite short-sighted.  The better way to do this is to complain
>>> if
>>> the number of expressions is different from the number of target
>>> variables
>>> (and the target variable is not a record-ish type).  There's been at
>>> least
>>> two patches for this earlier (one my me, and one by, I think Pavel
>>> Stehule).  I urge you to dig around in the archives to avoid wasting
>>> your
>>> time.
[snip a bit]

> As things stand today, we would have a hard time tightening that up
> without producing unwanted complaints about the cases mentioned in
> this comment, because the DTYPE_ROW logic is used for both "INTO a,b,c"
> and composite-type variables.  However, my pending patch at
> https://commitfest.postgresql.org/17/1439/
> gets rid of the use of DTYPE_ROW for composite types, and once that
> is in it might well be reasonable to just throw a flat-out error for
> wrong number of source values for a DTYPE_ROW target.  I can't
> immediately think of any good reason why you'd want to allow for
> the number of INTO items not matching what the query produces.

Perl lets you set a fixed number of multiple variables from an array and
discard the rest like so:

  my ($a, $b) = (1,2,3);

and the right hand side can also be a function:

  my ($c, $d) = somefunc( 123 );

Where somefunc() returns more than 2 params.

This is quite handy if you sometimes need more ore less return values, but
don't want to create almost-identical functions for each case.

I'm not sure if you mean exactly the scenario as in the attached test
case, but this works in plpgsql, too, and would be a shame to lose.

OTOH, one could also write:

  SELECT INTO ba, bb  a,b FROM foo(1);

and it would still work, or wouldn't it?

Best regards,

Tels

test.psql
Description: Binary data


test.pl
Description: Perl program

Re: [HACKERS] Transactions involving multiple postgres foreign servers

[Masahiko Sawada]

On Sat, Feb 10, 2018 at 4:08 AM, Robert Haas  wrote:
> On Thu, Feb 8, 2018 at 3:58 AM, Masahiko Sawada  wrote:
>>> Overall, what's the status of this patch?  Are we hung up on this
>>> issue only, or are there other things?
>>
>> AFAIK there is no more technical issue in this patch so far other than
>> this issue. The patch has tests and docs, and includes all stuff to
>> support atomic commit to distributed transactions: the introducing
>> both the atomic commit ability to distributed transactions and some
>> corresponding FDW APIs, and having postgres_fdw support 2pc. I think
>> this patch needs to be reviewed, especially the functionality of
>> foreign transaction resolution which is re-designed before.
>
> OK.  I'm going to give 0002 a read-through now, but I think it would
> be a good thing if others also contributed to the review effort.
> There is a lot of code here, and there are a lot of other patches
> competing for attention.  That said, here we go:

I appreciate your reviewing. I'll thoroughly update the whole patch
based on your comments and suggestions. Here is answer, question and
my understanding.

>
> The fdw-transactions section of the documentation seems to imply that
> henceforth every FDW must call FdwXactRegisterForeignServer, which I
> think is an unacceptable API break.
>
> It doesn't seem advisable to make this behavior depend on
> max_prepared_foreign_transactions.  I think that it should be an
> server-level option: use 2PC for this server, or not?  FDWs that don't
> have 2PC default to "no"; but others can be set to "yes" if the user
> wishes.  But we shouldn't force that decision to be on a cluster-wide
> basis.

Since I've added a new option two_phase_commit to postgres_fdw we need
to ask FDW whether the foreign server is 2PC-capable server or not in
order to register the foreign server information. That's why the patch
asked FDW to call  FdwXactRegisterForeignServer. However we can
register a foreign server information automatically by executor (e.g.
at  BeginDirectModify and at BeginForeignModify) if a foreign server
itself has that information. We can add two_phase_commit_enabled
column to pg_foreign_server system catalog and that column is set to
true if the foriegn server is 2PC-capable (i.g. has enough functions)
and user want to use it.

>
> But that brings up another issue: why is MyFdwConnections named that
> way and why does it have those semantics?  That is, why do we really
> need a list of every FDW connection? I think we really only need the
> ones that are 2PC-capable writes.  If a non-2PC-capable foreign server
> is involved in the transaction, then we don't really to keep it in a
> list.  We just need to flag the transaction as not locally prepareable
> i.e. clear TwoPhaseReady.  I think that this could all be figured out
> in a much less onerous way: if we ever perform a modification of a
> foreign table, have nodeModifyTable.c either mark the transaction
> non-preparable by setting XACT_FLAGS_FDWNOPREPARE if the foreign
> server is not 2PC capable, or otherwise add the appropriate
> information to MyFdwConnections, which can then be renamed to indicate
> that it contains only information about preparable stuff.  Then you
> don't need each individual FDW to be responsible about calling some
> new function; the core code just does the right thing automatically.

I could not get this comment. Did you mean that the foreign
transaction on not 2PC-capable foreign server should be end in the
same way as before (i.g. by XactCallback)?

Currently, because there is not FDW API to end foreign transaction,
almost FDWs use XactCallbacks to end the transaction. But after
introduced new FDW APIs, I think it's better to use FDW APIs to end
transactions rather than using XactCallbacks. Otherwise we end up with
having FDW APIs for 2PC (prepare and resolve) and XactCallback for
ending the transaction, which would be hard to understand. So I've
changed the foreign transaction management so that core code
explicitly asks FDW to end/prepare a foreign transaction instead of
ending it by individual FDWs. After introduced new FDW APIs, core code
can have the information of all foreign servers involved with the
transaction and call each APIs at appropriate timing.

>
> +if (!fdw_conn->end_foreign_xact(fdw_conn->serverid, fdw_conn->userid,
> +fdw_conn->umid, true))
> +elog(WARNING, "could not commit transaction on server %s",
> + fdw_conn->servername);
>
> First, as I noted above, this assumes that the local transaction can't
> fail after this point, which is certainly false -- if nothing else,
> think about a plug pull.  Second, it assumes that the right thing to
> do would be to throw a WARNING, which I think is false: if this is
> running in the pre-commit phase, it's not too late to switch to the
> abort path, and certainly if we make changes on only 1 server 

master plpython check fails on Solaris 10

[Marina Polyakova]

Hello, hackers! I got a permanent failure of master (commit 
ebdb42a0d6a61b93a5bb9f4204408edf5959332c) plpython check on Solaris 10. 
Regression output and diffs are attached.


I used the following commands:
./configure CC="ccache gcc" CFLAGS="-m64 -I/opt/csw/include" 
PKG_CONFIG_PATH="/opt/csw/lib/pkgconfig:/usr/local/lib/pkgconfig" 
LDFLAGS="-L/opt/csw/lib/sparcv9 -L/usr/local/lib/64" --enable-cassert 
--enable-debug --enable-nls --enable-tap-tests --with-perl --with-tcl 
--with-python --with-gssapi --with-openssl --with-ldap --with-libxml 
--with-libxslt --with-icu

gmake > make_results.txt
gmake -C src/pl/plpython check

Binary search has shown that this failure begins with commit 
8561e4840c81f7e345be2df170839846814fa004 (Transaction control in PL 
procedures.). On the previous commit 
(b9ff79b8f17697f3df492017d454caa9920a7183) there's no 
plpython_transaction test and plpython check passes.


About the system: SunOS, Release 5.10, KernelID Generic_141444-09.

P.S. It seems that there's a similar failure on Windows, and I'm trying 
to reproduce it..


--
Marina Polyakova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company*** /home/buildfarm/mpolyakova/postgrespro_core_353_stable_func/src/pl/plpython/expected/plpython_transaction.out	Tue Feb 13 13:00:33 2018
--- /home/buildfarm/mpolyakova/postgrespro_core_353_stable_func/src/pl/plpython/results/plpython_transaction.out	Tue Feb 13 13:34:39 2018
***
*** 88,136 
  return 1
  $$;
  SELECT transaction_test4();
! ERROR:  spiexceptions.InvalidTransactionTermination: invalid transaction termination
! CONTEXT:  Traceback (most recent call last):
!   PL/Python function "transaction_test4", line 2, in 
! plpy.execute("DO LANGUAGE plpythonu $x$ plpy.commit() $x$")
! PL/Python function "transaction_test4"
! -- commit inside subtransaction (prohibited)
! DO LANGUAGE plpythonu $$
! s = plpy.subtransaction()
! s.enter()
! plpy.commit()
! $$;
! WARNING:  forcibly aborting a subtransaction that has not been exited
! ERROR:  cannot commit while a subtransaction is active
! CONTEXT:  PL/Python anonymous code block
! -- commit inside cursor loop
! CREATE TABLE test2 (x int);
! INSERT INTO test2 VALUES (0), (1), (2), (3), (4);
! TRUNCATE test1;
! DO LANGUAGE plpythonu $$
! for row in plpy.cursor("SELECT * FROM test2 ORDER BY x"):
! plpy.execute("INSERT INTO test1 (a) VALUES (%s)" % row['x'])
! plpy.commit()
! $$;
! ERROR:  cannot commit transaction while a cursor is open
! CONTEXT:  PL/Python anonymous code block
! SELECT * FROM test1;
!  a | b 
! ---+---
! (0 rows)
! 
! -- rollback inside cursor loop
! TRUNCATE test1;
! DO LANGUAGE plpythonu $$
! for row in plpy.cursor("SELECT * FROM test2 ORDER BY x"):
! plpy.execute("INSERT INTO test1 (a) VALUES (%s)" % row['x'])
! plpy.rollback()
! $$;
! ERROR:  cannot abort transaction while a cursor is open
! CONTEXT:  PL/Python anonymous code block
! SELECT * FROM test1;
!  a | b 
! ---+---
! (0 rows)
! 
! DROP TABLE test1;
! DROP TABLE test2;
--- 88,94 
  return 1
  $$;
  SELECT transaction_test4();
! server closed the connection unexpectedly
! 	This probably means the server terminated abnormally
! 	before or while processing the request.
! connection to server was lost

==

*** /home/buildfarm/mpolyakova/postgrespro_core_353_stable_func/src/pl/plpython/expected/plpython_drop.out	Wed Feb  7 17:27:50 2018
--- /home/buildfarm/mpolyakova/postgrespro_core_353_stable_func/src/pl/plpython/results/plpython_drop.out	Tue Feb 13 13:34:39 2018
***
*** 1,6 
! --
! -- For paranoia's sake, don't leave an untrusted language sitting around
! --
! SET client_min_messages = WARNING;
! DROP EXTENSION plpythonu CASCADE;
! DROP EXTENSION IF EXISTS plpython2u CASCADE;
--- 1 
! psql: FATAL:  the database system is in recovery mode

==

test plpython_schema  ... ok
test plpython_populate... ok
test plpython_test... ok
test plpython_do  ... ok
test plpython_global  ... ok
test plpython_import  ... ok
test plpython_spi ... ok
test plpython_newline ... ok
test plpython_void... ok
test plpython_call... ok
test plpython_params  ... ok
test plpython_setof   ... ok
test plpython_record  ... ok
test plpython_trigger ... ok
test plpython_types   ... ok
test plpython_error   ... ok
test plpython_ereport ... ok
test plpython_unicode ... ok
test plpython_quote   ... ok
test plpython_composite   ... ok
test plpython_subtransaction  ... ok
test plpython_transaction ... FAILED (test process exited with exit code 2)
test plpython_drop... FAILED (test process exited with exit code 2)

Re: CALL stmt, ERROR: unrecognized node type: 113 bug

[Michael Paquier]

On Mon, Feb 12, 2018 at 03:19:27PM -0500, Tom Lane wrote:
> I've not read in detail, but it looks reasonable offhand, modulo
> that I still don't like prorettype == 0 ;-).
> 
> I did notice a tiny typo:
> 
> -  * with.  Hence prefer "$function$", but extend if needed.
> +  * with.  Hence prefer "$function$"/"$procedure", but extend if needed.
> 
> I think you want
> 
> +  * with.  Hence prefer "$function$"/"$procedure$", but extend if needed.

0001 and 0002 are welcome.  I have a small comment on top of Tom's for 0003.

+   appendStringInfoString(, ")\n");
+   if (proc->prorettype)
+   {
+   appendStringInfoString(, " RETURNS ");
+   print_function_rettype(, proctup);
+   appendStringInfoChar(, '\n');
+   }
Could you use a separate boolean variable which is set as
OidIsValid(prorettype), say called isfunction?  The goal is to avoid the
check on prorettype in more than one place.  If pg_proc's shape is
changed depending on the discussion, the current patch is a recipy to
forget updating all those places.  A comment in pg_get_function_result
to mention that prorettype = InvalidOid is here to track that the call
involves a procedure would be nice.

Should the documentation of pg_function_is_visible also mention
procedures?
--
Michael


signature.asc
Description: PGP signature