Re: [HACKERS] Small patch for pg_basebackup argument parsing

[Pierre Ducroquet]

On Friday, April 14, 2017 8:44:37 AM CEST Michael Paquier wrote:
> On Fri, Apr 14, 2017 at 6:32 AM, Pierre Ducroquet  
wrote:
> > Yesterday while doing a few pg_basebackup, I realized that the integer
> > parameters were not properly checked against invalid input.
> > It is not a critical issue, but this could be misleading for an user who
> > writes -z max or -s 0.5…
> > I've attached the patch to this mail. Should I add it to the next commit
> > fest or is it not needed for such small patches ?
> 
> A call to atoi is actually equivalent to strtol with the rounding:
> (int)strtol(str, (char **)NULL, 10);
> So I don't think this is worth caring.

The problem with atoi is that it simply ignores any invalid input and returns 
0 instead.
That's why I did this patch, because I did a typo when calling pg_basebackup 
and was not warned for an invalid input.
But it doesn't matter that much, so if you don't think that's interesting, no 
problem.

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

Re: [HACKERS] Interval for launching the table sync worker

[Kyotaro HORIGUCHI]

At Thu, 13 Apr 2017 20:02:33 +0200, Petr Jelinek  
wrote in 
> >> Although this is not a problem of this patch, this is a problem
> >> generally.
> 
> Huh? We explicitly switch to CacheMemoryContext before pallocing
> anything that should survive long term.

Ah.. yes, sorry for the noise.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center



-- 
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] Rewriting the test of pg_upgrade as a TAP test

[Michael Paquier]

On Thu, Apr 6, 2017 at 7:48 AM, Michael Paquier
 wrote:
> On Wed, Apr 5, 2017 at 10:24 PM, Stephen Frost  wrote:
>> * Michael Paquier (michael.paqu...@gmail.com) wrote:
>>> 1) Initialize the old cluster and start it.
>>> 2) create a bunch of databases with full range of ascii characters.
>>> 3) Run regression tests.
>>> 4) Take dump on old cluster.
>>> 4) Stop the old cluster.
>>> 5) Initialize the new cluster.
>>> 6) Run pg_upgrade.
>>> 7) Start new cluster.
>>> 8) Take dump from it.
>>> 9) Run deletion script (Oops forgot this part!)
>>
>> Presumably the check to match the old dump against the new one is also
>> performed?
>
> Yes. That's run with command_ok() at the end.

Attached is an updated patch to use --no-sync with pg_dumpall calls.
-- 
Michael


pgupgrade-tap-test-v3.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] identity columns

[Noah Misch]

On Thu, Apr 06, 2017 at 10:26:16PM -0700, Vitaly Burovoy wrote:
> On 4/6/17, Peter Eisentraut  wrote:
> > On 4/4/17 22:53, Vitaly Burovoy wrote:
> >> The next nitpickings to the last patch. I try to get places with
> >> lacking of variables' initialization.
> >> All other things seem good for me now. I'll continue to review the
> >> patch while you're fixing the current notes.
> >
> > Committed with your changes (see below) as well as executor fix.
> 
> Thank you very much!
> 
> 
> > As I tried to mention earlier, it is very difficult to implement the IF
> > NOT EXISTS behavior here, because we need to run the commands the create
> > the sequence before we know whether we will need it.
> 
> In fact with the function "get_attidentity" it is not so hard. Please,
> find the patch attached.
> I've implement SET GENERATED ... IF NOT EXISTS. It must be placed
> before other SET options but fortunately it conforms with the
> standard.
> Since that form always changes the sequence behind the column, I
> decided to explicitly write "[NO] CACHE" in pg_dump.
> 
> As a plus now it is possible to rename the sequence behind the column
> by specifying SEQUENCE NAME in SET GENERATED.
> 
> I hope it is still possible to get rid of the "ADD GENERATED" syntax.

[Action required within three days.  This is a generic notification.]

The above-described topic is currently a PostgreSQL 10 open item.  Peter,
since you committed the patch believed to have created it, you own this open
item.  If some other commit is more relevant or if this does not belong as a
v10 open item, please let us know.  Otherwise, please observe the policy on
open item ownership[1] and send a status update within three calendar days of
this message.  Include a date for your subsequent status update.  Testers may
discover new open items at any time, and I want to plan to get them all fixed
well in advance of shipping v10.  Consequently, I will appreciate your efforts
toward speedy resolution.  Thanks.

[1] 
https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.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] Allowing extended stats on foreign and partitioned tables

[Noah Misch]

On Mon, Apr 10, 2017 at 09:39:22PM +1200, David Rowley wrote:
> While reviewing extended stats I noticed that it was possible to
> create extended stats on many object types, including sequences. I
> mentioned that this should be disallowed. Statistics were then changed
> to be only allowed on plain tables and materialized views.
> 
> This should be relaxed again to allow anything ANALYZE is allowed on.
> 
> The attached does this.

[Action required within three days.  This is a generic notification.]

The above-described topic is currently a PostgreSQL 10 open item.  Alvaro,
since you committed the patch believed to have created it, you own this open
item.  If some other commit is more relevant or if this does not belong as a
v10 open item, please let us know.  Otherwise, please observe the policy on
open item ownership[1] and send a status update within three calendar days of
this message.  Include a date for your subsequent status update.  Testers may
discover new open items at any time, and I want to plan to get them all fixed
well in advance of shipping v10.  Consequently, I will appreciate your efforts
toward speedy resolution.  Thanks.

[1] 
https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.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] Tuplesort merge pre-reading

[Peter Geoghegan]

On Thu, Apr 13, 2017 at 10:19 PM, Peter Geoghegan  wrote:
> I actually think Heikki's work here would particularly help on
> spinning rust, especially when less memory is available. He
> specifically justified it on the basis of it resulting in a more
> sequential read pattern, particularly when multiple passes are
> required.

BTW, what you might have missed is that Heikki did end up using a
significant amount of memory in the committed version. It just ended
up being managed by logtape.c, which now does the prereading instead
of tuplesort.c, but at a lower level. There is only one tuple in the
merge heap, but there is still up to 1GB of memory per tape,
containing raw preread tuples mixed with integers that demarcate tape
contents.

-- 
Peter Geoghegan

VMware vCenter Server
https://www.vmware.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] [pgsql-www] Small issue in online devel documentation build

[Noah Misch]

On Wed, Apr 05, 2017 at 01:43:42PM -0400, Peter Eisentraut wrote:
> On 4/5/17 02:56, Noah Misch wrote:
> > On Thu, Mar 23, 2017 at 11:21:39PM -0400, Peter Eisentraut wrote:
> >> I think the fix belongs into the web site CSS, so there is nothing to
> >> commit into PostgreSQL here.  I will close the commit fest entry, but I
> >> have added a section to the open items list so we keep track of it.
> >> (https://wiki.postgresql.org/wiki/PostgreSQL_10_Open_Items#Documentation_tool_chain)
> > 
> > [Action required within three days.  This is a generic notification.]
> 
> I will work on this next week.  I believe I will be able to provide a
> patch for the web site CSS by April 12, but ultimate success will likely
> depend on the collaboration of the web team.

This PostgreSQL 10 open item is past due for your status update.  Kindly send
a status update within 24 hours, and include a date for your subsequent status
update.  Refer to the policy on open item ownership:
https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.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] Tuplesort merge pre-reading

[Peter Geoghegan]

On Thu, Apr 13, 2017 at 9:51 PM, Tom Lane  wrote:
> I'm fairly sure that the point was exactly what it said, ie improve
> locality of access within the temp file by sequentially reading as many
> tuples in a row as we could, rather than grabbing one here and one there.
>
> It may be that the work you and Peter G. have been doing have rendered
> that question moot.  But I'm a bit worried that the reason you're not
> seeing any effect is that you're only testing situations with zero seek
> penalty (ie your laptop's disk is an SSD).  Back then I would certainly
> have been testing with temp files on spinning rust, and I fear that this
> may still be an issue in that sort of environment.

I actually think Heikki's work here would particularly help on
spinning rust, especially when less memory is available. He
specifically justified it on the basis of it resulting in a more
sequential read pattern, particularly when multiple passes are
required.

> The larger picture to be drawn from that thread is that we were seeing
> very different performance characteristics on different platforms.
> The specific issue that Tatsuo-san reported seemed like it might be
> down to weird read-ahead behavior in a 90s-vintage Linux kernel ...
> but the point that this stuff can be environment-dependent is still
> something to take to heart.

BTW, I'm skeptical of the idea of Heikki's around killing polyphase
merge itself at this point. I think that keeping most tapes active per
pass is useful now that our memory accounting involves handing over an
even share to each maybe-active tape for every merge pass, something
established by Heikki's work on external sorting.

Interestingly enough, I think that Knuth was pretty much spot on with
his "sweet spot" of 7 tapes, even if you have modern hardware. Commit
df700e6 (where the sweet spot of merge order 7 was no longer always
used) was effective because it masked certain overheads that we
experience when doing multiple passes, overheads that Heikki and I
mostly removed. This was confirmed by Robert's testing of my merge
order cap work for commit fc19c18, where he found that using 7 tapes
was only slightly worse than using many hundreds of tapes. If we could
somehow be completely effective in making access to logical tapes
perfectly sequential, then 7 tapes would probably be noticeably
*faster*, due to CPU caching effects.

Knuth was completely correct to say that it basically made no
difference once more than 7 tapes are used to merge, because he didn't
have logtape.c fragmentation to worry about.

-- 
Peter Geoghegan

VMware vCenter Server
https://www.vmware.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_dump emits ALTER TABLE ONLY partitioned_table

[Amit Langote]

Hi Stephen,

On 2017/04/14 0:05, Stephen Frost wrote:
> Robert,
> 
> * Robert Haas (robertmh...@gmail.com) wrote:
>> So I think I was indeed confused before, and I think you're basically
>> right here, but on one point I think you are not right -- ALTER TABLE
>> ONLY .. CHECK () doesn't work on a table with inheritance children
>> regardless of whether the children already have the matching
>> constraint:
> 
> To be clear- I wasn't thinking about what's possible today with
> inheiritance or partitioning or in PG at all, but rather focusing on
> what a user is likely to expect and understand from the messaging.
> 
>> rhaas=# create table foo (a int, b text);
>> CREATE TABLE
>> rhaas=# create table bar () inherits (foo);
>> CREATE TABLE
>> rhaas=# alter table only foo add check (a = 1);
>> ERROR:  constraint must be added to child tables too
>> rhaas=# alter table only bar add check (a = 1);
>> ALTER TABLE
>> rhaas=# alter table only foo add check (a = 1);
>> ERROR:  constraint must be added to child tables too
> 
> If that's the case then we should really change that error message, in
> my view.  I'd be happier if we did support adding the constraint after
> child tables exist,

Do you mean to use ONLY even if the child tables exist and still succeed?
You can succeed in that case only by specifying NO INHERIT against the
constraint with traditional inheritance.  It does not work however with
partitioned tables, since we do not allow NO INHERIT constraints to be
defined on the partitioned tables; such a constraint would never be
enforced if we had allowed that and just sit there.  In the traditional
inheritance case, it is applied to the parent's own rows.

> but if we don't, we shouldn't imply to the user that
> they can add it after adding it to the children.

Hmm, the error message as it is *might* give the impression that we are
asking a user to go add the constraint to the child tables first.  Another
way the user might interpret the message is that it is asking to drop the
ONLY from the command (at least if they have read the documentation of
ONLY on the ALTER TABLE reference page).  But it perhaps isn't readily
apparent from the error message itself, so maybe a HINT message along
those lines might work.

>> So, regarding Amit's 0001:
>>
>> - I think we should update the relevant hunk of the documentation
>> rather than just removing it.
> 
> Alright.

You may have seen that the latest patch has taken care of this.  But maybe
you'll want to tweak my rewording further as you see fit.

>> - Should we similarly allow TRUNCATE ONLY foo and ALTER TABLE ONLY foo
>> .. to work on a partitioned table without partitions, or is that just
>> pointless tinkering?  That seems to be the only case where, after this
>> patch, an ONLY operation will fail on a childless partitioned table.
> 
> I'm less sure about TRUNCATE ONLY because that really isn't meaningful
> at all, is it?  A partitioned table doesn't have any data to truncate
> and truncating it doesn't have any impact on what happens later, does
> it?

That's right.  If you perform truncate (or truncate only) on an empty
partitioned table (i.e. with no partitions yet), it's essentially a no-op.
 Nor does it affect anything in the future.

> Having a sensible error be reported if someone tries to do that
> would be good though.

The latest patch implements the following:

-- create an empty partitioned table, aka without partitions
create table p (a int) partition by list (a);

-- no error, though nothing really happens
truncate only p;
TRUNCATE TABLE

-- add a partition
create table p1 partition of p for values in (1);

-- error, because a partition exists
truncate only p;
ERROR:  must truncate partitions too

-- ok, partition p1 will be truncated
truncate p;
TRUNCATE TABLE

I do now wonder if the error message "must truncate partitions *too*" is
somewhat inappropriate here.  The "too" seems to imply that table
specified in the command (which is partitioned) is somehow truncate-able,
which it is not.  Hmm.

>> Do you want to be responsible for committing these or should I do it?
> 
> Sure, though I won't be able to today and I've got some doubts about the
> other patches.  I'll have more time tomorrow though.

Thanks, I'll revise the patches per any review comments you might have.

Regards,
Amit



-- 
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] Tuplesort merge pre-reading

[Tom Lane]

I wrote:
> Heikki Linnakangas  writes:
>> I'm talking about the code that reads a bunch of from each tape, loading 
>> them into the memtuples array. That code was added by Tom Lane, back in 
>> 1999:
>> So apparently there was a benefit back then, but is it still worthwhile? 

> I'm fairly sure that the point was exactly what it said, ie improve
> locality of access within the temp file by sequentially reading as many
> tuples in a row as we could, rather than grabbing one here and one there.

[ blink... ]  Somehow, my mail reader popped up a message from 2016
as current, and I spent some time researching and answering it without
noticing the message date.

Never mind, nothing to see here ...

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] Tuplesort merge pre-reading

[Tom Lane]

Heikki Linnakangas  writes:
> I'm talking about the code that reads a bunch of from each tape, loading 
> them into the memtuples array. That code was added by Tom Lane, back in 
> 1999:

> commit cf627ab41ab9f6038a29ddd04dd0ff0ccdca714e
> Author: Tom Lane 
> Date:   Sat Oct 30 17:27:15 1999 +

>  Further performance improvements in sorting: reduce number of comparisons
>  during initial run formation by keeping both current run and next-run
>  tuples in the same heap (yup, Knuth is smarter than I am).  And, during
>  merge passes, make use of available sort memory to load multiple tuples
>  from any one input 'tape' at a time, thereby improving locality of
>  access to the temp file.

> So apparently there was a benefit back then, but is it still worthwhile? 

I'm fairly sure that the point was exactly what it said, ie improve
locality of access within the temp file by sequentially reading as many
tuples in a row as we could, rather than grabbing one here and one there.

It may be that the work you and Peter G. have been doing have rendered
that question moot.  But I'm a bit worried that the reason you're not
seeing any effect is that you're only testing situations with zero seek
penalty (ie your laptop's disk is an SSD).  Back then I would certainly
have been testing with temp files on spinning rust, and I fear that this
may still be an issue in that sort of environment.

The relevant mailing list thread seems to be "sort on huge table" in
pgsql-hackers in October/November 1999.  The archives don't seem to have
threaded that too successfully, but here's a message specifically
describing the commit you mention:

https://www.postgresql.org/message-id/2726.941493808%40sss.pgh.pa.us

and you can find the rest by looking through the archive summary pages
for that interval.

The larger picture to be drawn from that thread is that we were seeing
very different performance characteristics on different platforms.
The specific issue that Tatsuo-san reported seemed like it might be
down to weird read-ahead behavior in a 90s-vintage Linux kernel ...
but the point that this stuff can be environment-dependent is still
something to take to heart.

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] Foreign Join pushdowns not working properly for outer joins

[Ashutosh Bapat]

On Thu, Apr 13, 2017 at 7:35 AM, David Rowley
 wrote:
> On 13 April 2017 at 11:22, Peter Eisentraut
>  wrote:
>> Is this patch considered ready for review as a backpatch candidate?
>
> Yes, however, the v5 patch is based on master. The v4 patch should
> apply to 9.6. Diffing the two patches I see another tiny change to a
> comment, of which I think needs re-worded anyway.
>
> + * This function should usually set FDW options in fpinfo after the join is
> + * deemed safe to push down to save some CPU cycles. But We need server
> + * specific options like extensions to decide push-down safety. For
> + * checking extension shippability, we need foreign server as well.
> + */
>
> This might be better written as:
>
> Ordinarily, we might be tempted into delaying the merging of the FDW
> options until we've deemed the foreign join to be ok. However, we must
> do this before performing this test so that we know which quals can be
> evaluated on the foreign server. This may depend on the
> shippable_extensions.
>

This looks better. Here are patches for master and 9.6.
Since join pushdown was supported in 9.6 the patch should be
backported to 9.6 as well. Attached is the patch (_96) for 9.6,
created by rebasing on 9.6 branch and removing conflict. _v6 is
applicable on master.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company


foreign_outerjoin_pushdown_fix_96.patch
Description: Binary data


foreign_outerjoin_pushdown_fix_v6.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] Logical replication and inheritance

[Amit Langote]

On 2017/04/14 10:57, Petr Jelinek wrote:
> I don't think inheritance and partitioning should behave the same in
> terms of logical replication.

I see.

> 
> For me the current behavior with inherited tables is correct.

OK.

By the way, what do you think about the pg_dump example/issue I mentioned?
 Is that a pg_dump problem or backend?  To reiterate, if you add an
inheritance parent to a publication, dump the database, and restore it
into another, an error occurs.  Why?  Because every child table is added
*twice* because of the way publication tables are dumped.  Once by itself
and again via inheritance expansion when the parent is added.  Adding a
table again to the same publication is currently an error, which I was
wondering if it could be made a no-op instead.

> What I would like partitioned tables support to look like is that if we
> add partitioned table, the data decoded from any of the partitions would
> be sent as if the change was for that partitioned table so that the
> partitioning scheme on subscriber does not need to be same as on
> publisher. That's nontrivial to do though probably.

I agree that it'd be nontrivial.  I'm not sure if you're also implying
that a row decoded from a partition is *never* published as having been
inserted into the partition itself.  A row can end up in a partition via
both the inserts into the partitioned table and the partition itself.
Also, AFAICT, obviously the output pluggin would have to have a dedicated
logic to determine which table to publish a given row as coming from
(possibly the root partitioned table), since nothing decode-able from WAL
is going to have that information.

Also, with the current state of partitioning, if a row decoded and
published as coming from the partitioned table had no corresponding
partition defined on the destination server, an error will occur in the
subscription worker I'd guess.  Or may be we don't assume anything about
whether the table on the subscription end is partitioned or not.

Anyway, that perhaps also means that for time being, we might need to go
with the following option that Robert mentioned (I suppose strictly in the
context of partitioned tables, not general inheritance):

(1) That's an error; the user should publish the partitions instead.

That is, we should make adding a partitioned table to a publication a user
error (feature not supported).

Thanks,
Amit



-- 
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] Remove trailing spaces

[Peter Eisentraut]

On 3/29/17 04:11, Alexander Law wrote:
> Please consider committing the attached patches to remove trailing 
> spaces in strings in the source code.
> One patch is for localizable messages, and the other is just for 
> consistency (less important).

committed

-- 
Peter Eisentraut  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] Re: [COMMITTERS] pgsql: Add COMMENT and SECURITY LABEL support for publications and subs

[Peter Eisentraut]

On 4/4/17 09:59, Stephen Frost wrote:
>> Attached is a patch that adds the pg_dump support, but I'm struggling to
>> make the tests work.  Could you take a look?  Problem one I'm seeing is
>> that the tests assert that there are no comments in the post-data
>> section, which is no longer the case here.
> 
> If that's the case (and is intended), then you'll need to remove the
> 'section_post_data' entry from the COMMENTS catch-all 'unlike' and move
> that into the 'unlike' for each of the COMMENT tests which were
> depending on the catch-all to handle that.

done

-- 
Peter Eisentraut  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] Letting the client choose the protocol to use during a SASL exchange

[Michael Paquier]

On Fri, Apr 14, 2017 at 1:37 AM, Heikki Linnakangas  wrote:
> On 04/13/2017 05:53 AM, Michael Paquier wrote:
>> +* Parse the list of SASL authentication mechanisms in the
>> +* AuthenticationSASL message, and select the best mechanism that we
>> +* support.  (Only SCRAM-SHA-256 is supported at the moment.)
>>  */
>> -   if (strcmp(auth_mechanism, SCRAM_SHA256_NAME) == 0)
>> +   for (;;)
>> Just an idea here: being able to enforce the selection with an
>> environment variable (useful for testing as well in the future).
>
> Hmm. It wouldn't do much, as long as SCRAM-SHA-256 is the only supported
> mechanism. In general, there is no way to tell libpq to e.g. not do plain
> password authentication, which is more pressing than choosing a particular
> SASL mechanism. So I think we should have libpq options to control that, but
> it's a bigger feature than just adding a debug environment variable here.

Of course, my last sentence implied that this may be useful once more
than 1 mechanism is added. This definitely cannot be a connection
parameter. Your last sentence makes me guess that we agree on that.
But those are thoughts for later..

> Thanks for the review! I've pushed these patches, after a bunch of little
> cleanups here and there, and fixing a few garden-variety bugs in the
> GSS/SSPI changes.

Committed patches look good to me after a second lookup. Thanks!
-- 
Michael


-- 
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 and inheritance

[Petr Jelinek]

I don't think inheritance and partitioning should behave the same in
terms of logical replication.

For me the current behavior with inherited tables is correct.

What I would like partitioned tables support to look like is that if we
add partitioned table, the data decoded from any of the partitions would
be sent as if the change was for that partitioned table so that the
partitioning scheme on subscriber does not need to be same as on
publisher. That's nontrivial to do though probably.

-- 
  Petr Jelinek  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] SUBSCRIPTIONS and pg_upgrade

[Peter Eisentraut]

On 4/13/17 12:11, Robert Haas wrote:
> I wonder if we should have an --no-subscriptions option, now that they
> are dumped by default, just like we have --no-blobs, --no-owner,
> --no-password, --no-privileges, --no-acl, --no-tablespaces, and
> --no-security-labels.  It seems like there is probably a fairly large
> use case for excluding subscriptions even if you have sufficient
> permissions to dump them.

What purpose would that serve in practice?  The subscriptions are now
dumped disabled, so if they hang around, there is not much impact.

Perhaps an option that also omits publications would make sense?

Or a general filtering facility based on the object tag?

-- 
Peter Eisentraut  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] Quorum commit for multiple synchronous replication.

[Masahiko Sawada]

On Fri, Apr 14, 2017 at 9:38 AM, Michael Paquier
 wrote:
> On Fri, Apr 14, 2017 at 2:47 AM, Fujii Masao  wrote:
>> I'm thinking that it's less confusing to report always 0 as the priority of
>> async standby whatever the setting of synchronous_standby_names is.
>> Thought?
>
> Or we could have priority being reported to NULL for async standbys as
> well, the priority number has no meaning for them anyway...

I agree to set the same thing (priority or NULL) to all sync standby
in a quorum set. As Fujii-san mentioned,  I also think that it means
all standbys in a quorum set can be chosen equally. But to less
confusion for current user I'd not like to change current behavior of
the priority of async standby.

>
>> If we adopt this idea, in a quorum-based sync replication, I think that
>> the priorities of all the standbys listed in synchronous_standby_names
>> should be 1 instead of NULL. That is, those standbys have the same
>> (highest) priority, and which means that any of them can be chosen as
>> sync standby. Thought?
>
> Mainly my fault here to suggest that standbys in a quorum set should
> have a priority set to NULL. My 2c on the matter is that I would be
> fine with either having the async standbys having a priority of NULL
> or using a priority of 1 for standbys in a quorum set. Though,
> honestly, I find that showing a priority number for something where
> this has no real meaning is even more confusing..

This is just a thought but we can merge sync_priority and sync_state
into one column. The sync priority can have meaning only when the
standby is considered as a sync standby or a potential standby in
priority-based sync replication. For example, we can show something
like 'sync:N' as states of the sync standby and 'potential:N' as
states of the potential standby in priority-based sync replication,
where N means the priority. In quorum-based sync replication it is
just 'quorum'. It breaks backward compatibility, though.

Regards,

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


-- 
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] SUBSCRIPTIONS and pg_upgrade

[Petr Jelinek]

On 13/04/17 18:11, Robert Haas wrote:
> On Thu, Apr 13, 2017 at 12:05 PM, Peter Eisentraut
>  wrote:
>> On 4/12/17 18:31, Peter Eisentraut wrote:
>>> On 4/11/17 23:41, Noah Misch wrote:
 On Tue, Apr 11, 2017 at 11:21:24PM -0400, Peter Eisentraut wrote:
> On 4/9/17 22:16, Noah Misch wrote:
>> [Action required within three days.  This is a generic notification.]
>
> Patches have been posted.  Discussion is still going on a bit.

 By what day should the community look for your next update?
>>>
>>> tomorrow
>>
>> Everything has been committed, and this thread can be closed.
> 
> I wonder if we should have an --no-subscriptions option, now that they
> are dumped by default, just like we have --no-blobs, --no-owner,
> --no-password, --no-privileges, --no-acl, --no-tablespaces, and
> --no-security-labels.  It seems like there is probably a fairly large
> use case for excluding subscriptions even if you have sufficient
> permissions to dump them.
> 

+1, I'll look into writing patch for that

-- 
  Petr Jelinek  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] Declarative partitioning vs. information_schema

[Amit Langote]

On 2017/04/14 5:28, Robert Haas wrote:
> On Thu, Apr 6, 2017 at 3:14 AM, Amit Langote
>  wrote:
>>> The bulk of operations that work on traditional tables also work on 
>>> partitions
>>> and partitioned tables.  The next closest kind of relation, a materialized
>>> view, is far less table-like.  Therefore, I recommend showing both 
>>> partitions
>>> and partitioned tables in those views.  This is also consistent with the
>>> decision to use words like "partition" and "partitioned" in messages only 
>>> when
>>> partitioning is relevant to the error.  For example, ATWrongRelkindError()
>>> distinguishes materialized views from tables, but it does not distinguish
>>> tables based on their participation in partitioning.
>>
>> +1
> 
> OK, whoever wants to write the patch, please step forward.

Sorry, perhaps I'm missing something, but I thought there was no patch
left to be written, because the original patch (this thread) implemented
what Noah recommended.

As of HEAD (6cfaffc0ddc):

create table p (a int, b char) partition by list (a);
create table p1 partition of p for values in (1) partition by list (b);
create table p1a partition of p1 for values in ('a');

\d
   List of relations
 Schema | Name | Type  | Owner
+--+---+---
 public | p| table | amit
 public | p1   | table | amit
 public | p1a  | table | amit
(3 rows)

select tablename from pg_tables where schemaname = 'public';
 tablename
---
 p
 p1
 p1a
(3 rows)

select table_name from information_schema.tables where table_schema =
'public';
 table_name

 p
 p1
 p1a
(3 rows)

Also, it seems that this open item has been listed under Non-bugs, with
remark "firm support for status quo, lack of firm support for alternatives".

Thanks,
Amit



-- 
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] identity columns

[Michael Paquier]

On Thu, Apr 13, 2017 at 7:40 PM, Vitaly Burovoy
 wrote:
> On 4/6/17, Vitaly Burovoy  wrote:
>> On 4/6/17, Peter Eisentraut  wrote:
>>> As I tried to mention earlier, it is very difficult to implement the IF
>>> NOT EXISTS behavior here, because we need to run the commands the create
>>> the sequence before we know whether we will need it.
>>
>> In fact with the function "get_attidentity" it is not so hard. Please,
>> find the patch attached.
> ...
>> I hope it is still possible to get rid of the "ADD GENERATED" syntax.
>
> Is it possible to commit the patch from the previous letter?
> It was sent before the feature freeze, introduces no new feature (only
> syntax change discussed in this thread and not implemented due to lack
> of a time of the author) and can be considered as a fix to the
> committed patch (similar to the second round in "sequence data type").

It seems to me that at least the part about removing ADD GENERATED
could be applied as an adjustment for the committed patch (INE would
be a new feature), and if there is a time to adjust already committed
features for the upcoming release, it is now. So I have added an open
item.
--
Michael


-- 
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] Quorum commit for multiple synchronous replication.

[Michael Paquier]

On Fri, Apr 14, 2017 at 2:47 AM, Fujii Masao  wrote:
> I'm thinking that it's less confusing to report always 0 as the priority of
> async standby whatever the setting of synchronous_standby_names is.
> Thought?

Or we could have priority being reported to NULL for async standbys as
well, the priority number has no meaning for them anyway...

> If we adopt this idea, in a quorum-based sync replication, I think that
> the priorities of all the standbys listed in synchronous_standby_names
> should be 1 instead of NULL. That is, those standbys have the same
> (highest) priority, and which means that any of them can be chosen as
> sync standby. Thought?

Mainly my fault here to suggest that standbys in a quorum set should
have a priority set to NULL. My 2c on the matter is that I would be
fine with either having the async standbys having a priority of NULL
or using a priority of 1 for standbys in a quorum set. Though,
honestly, I find that showing a priority number for something where
this has no real meaning is even more confusing..
-- 
Michael


-- 
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] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Fabien COELHO]

On Mon, Apr 3, 2017 at 3:32 PM, Daniel Verite  wrote:

In interactive mode, the warning in untaken branches is misleading
when \endif is on the same line as the commands that
are skipped. For instance:

  postgres=# \if false \echo NOK \endif
  \echo command ignored; use \endif or Ctrl-C to exit current \if block
  postgres=#

From the point of view of the user, the execution flow has exited
the branch already when this warning is displayed.
Of course issuing the recommended \endif at this point doesn't work:

  postgres=# \endif
  \endif: no matching \if

Maybe that part of the message:
"use \endif or Ctrl-C to exit current \if block"
should be displayed only when coming back at the prompt,
and if the flow is still in an untaken branch at this point?


Is this an open item, or do we not care about fixing it?


I would suggest that this is not important enough to block anything.

Otherwise, I agree that displaying this interactive message only when it 
is pertinent is desirable, but this might change the underlying logic 
significantly: it may mean holding somewhere a message to be shown at next 
prompt, and being able to decide when to clear it.


There is also the question of what happens if there are multiple such 
messages, should they all be shown? Only the first? The last? Should it 
avoid repeats?


So I propose to call it a feature for now, especially that we do not 
expect people to write a lot of one-liner multiple-backslash-commands in 
interactive mode.


--
Fabien.


--
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 and PANIC during shutdown checkpoint in publisher

[Michael Paquier]

On Fri, Apr 14, 2017 at 3:03 AM, Fujii Masao  wrote:
> On Thu, Apr 13, 2017 at 12:36 PM, Michael Paquier
>  wrote:
>> On Thu, Apr 13, 2017 at 12:28 PM, Fujii Masao  wrote:
>>> On Thu, Apr 13, 2017 at 5:25 AM, Peter Eisentraut
>>>  wrote:
 On 4/12/17 09:55, Fujii Masao wrote:
> To fix this issue, we should terminate walsender for logical replication
> before shutdown checkpoint starts. Of course walsender for physical
> replication still needs to keep running until shutdown checkpoint ends,
> though.

 Can we turn it into a kind of read-only or no-new-commands mode instead,
 so it can keep streaming but not accept any new actions?
>>>
>>> So we make walsenders switch to that mode and wait for all the 
>>> already-ongoing
>>> their "write" commands to finish, and then we start a shutdown checkpoint?
>>> This is an idea, but seems a bit complicated. ISTM that it's simpler to
>>> terminate only walsenders for logical rep before shutdown checkpoint.
>>
>> Perhaps my memory is failing me here... But in clean shutdowns we do
>> shut down WAL senders after the checkpoint has completed so as we are
>> sure that they have flushed the LSN corresponding to the checkpoint
>> ending, right?
>
> Yes.
>
>> Why introducing an inconsistency for logical workers?
>> It seems to me that logical workers should fail under the same rules.
>
> Could you tell me why? You think that even walsender for logical rep
> needs to stream the shutdown checkpoint WAL record to the subscriber?
> I was not thinking that's true.

For physical replication, the property to wait that standbys have
flushed the LSN of the shutdown checkpoint can be important for
switchovers. For example, with a primary and a standby, it is possible
to stop cleanly the master, promote the standby, and then connect back
to the cluster the old primary as a standby to the now-new primary
with the guarantee that both are in a consistent state. It seems to me
that having similar guarantees for logical replication is important.

Now, I have not reviewed the code of logirep in details at the level
of Peter, Petr or yourself...
-- 
Michael


-- 
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] Small patch for pg_basebackup argument parsing

[Michael Paquier]

On Fri, Apr 14, 2017 at 6:32 AM, Pierre Ducroquet  wrote:
> Yesterday while doing a few pg_basebackup, I realized that the integer
> parameters were not properly checked against invalid input.
> It is not a critical issue, but this could be misleading for an user who
> writes -z max or -s 0.5…
> I've attached the patch to this mail. Should I add it to the next commit fest
> or is it not needed for such small patches ?

A call to atoi is actually equivalent to strtol with the rounding:
(int)strtol(str, (char **)NULL, 10);
So I don't think this is worth caring.

By doing a git grep "atoi(optarg)" you'll see far more places that
handle integer options this way as well...
-- 
Michael


-- 
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] Undefined psql variables

[Fabien COELHO]

Hello Robert,


Calling the server is already available:

  SELECT  AS varname \gset


Sure, but people are going to want to do it inline with the \if.


Yes... and my changed opinion is that the answer to this approach should 
be "no", only client side after if.



Anything that can be done that way can also be done this way, but
people will want it just to make the code look nicer.


That is what I thought, but I have not seen any sane/nice solution, and I 
wish to avoid the opposite.


I now think that whether an expression is server side or client side 
should be cristal clear, thus the rule "write a SELECT for server-side" 
and "write a backslash command" for client-side is pretty attractive.


I do not think it is so bad: this is probably a rare occurence (psql spent 
22 years without "\if") ; for server side expressions, it means that an 
intermediate meaningful variable name must be thought for, which is not 
necessarily a bad thing ; any significant SQL query would not fit cleanly 
on one line, especially if made longer by a special prefix.


Finally, it does not bring any new semantics.

I don't think we should restrict \if to be ONLY an SQL callout, but if 
people want that as an option, and I bet they do, then I think we should 
give it to them.


I changed my mind on this one. I think we should not for the reason stated 
above.


Now it would be possible to have some compromise, and we could accept some 
ugly prefix to mark server-side expressions after \if and no special 
prefix would mean client-side, but the I would prefer if we avoid that.



I somewhat disagree: Does building postgres should depend on lua? I think
adding such a mandatory dependency would not be a good idea. If it is not
mandatory, then it would mean that psql could be compiled with or without
lua embedding, thus psql would not be dependable because features may or may
not be available when writing a "psql script".


That's true, but you could say the same thing about SSL or NLS.


Hmmm. I'm not sure how NLS or SSL would show up inside a psql-script.

Another point I would like to make is that lua popularity is somewhere 
between COBOL and Fortran on the Tiobe index.


[...] I don't think it's likely that adding one or two additional simple 
constructs is going to be sufficient to keep people from wanting more.


I think that the next hurdle is high enough for not being jumped over in a 
hurry: for getting a while, one need to re-execute the body over and over, 
which requires holding the lines somewhere, meaning an significant 
infrastructure which does not exists. So someone would have to need it 
really badly to spend time on this one.



Generating a error message with ${foo:?} is nice, but what psql need is just
a way to test whether a variable is defined or not.


I'm not saying we should slavishly follow bash, but don't confuse what
you need with what other people need.  bash (well, sh, really) grew
that syntax for a reason, and it may be more than "there was this one
guy back in the seventies who wanted it, and ...".


Sure. I think that the reason is to be able to write shell scripts without 
bothering with undefined variables error handling. Lazy programmers:-)


--
Fabien.


--
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] Interval for launching the table sync worker

[Petr Jelinek]

On 13/04/17 12:23, Masahiko Sawada wrote:
> On Thu, Apr 13, 2017 at 11:53 AM, Masahiko Sawada  
> wrote:
>> On Wed, Apr 12, 2017 at 11:46 PM, Peter Eisentraut
>>  wrote:
>>> On 4/12/17 00:48, Masahiko Sawada wrote:
 On Wed, Apr 12, 2017 at 1:28 PM, Peter Eisentraut
> Perhaps instead of a global last_start_time, we store a per relation
> last_start_time in SubscriptionRelState?

 I was thinking the same. But a problem is that the list of
 SubscriptionRelState is refreshed whenever the syncing table state
 becomes invalid (table_state_valid = false). I guess we need to
 improve these logic including GetSubscriptionNotReadyRelations().
>>>
>>> The table states are invalidated on a syscache callback from
>>> pg_subscription_rel, which happens roughly speaking when a table
>>> finishes the initial sync.  So if we're worried about failing tablesync
>>> workers relaunching to quickly, this would only be a problem if a
>>> tablesync of another table finishes right in that restart window.  That
>>> doesn't seem a terrible issue to me.
>>>
>>
>> I think the table states are invalidated whenever the table sync
>> worker starts, because the table sync worker updates its status of
>> pg_subscription_rel and commits it before starting actual copy. So we
>> cannot rely on that. I thought we can store last_start_time into
>> pg_subscription_rel but it might be overkill. I'm now thinking to
>> change GetSubscriptionNotReadyRealtions so that last_start_time in
>> SubscriptionRelState is taken over to new list.
>>
> 
> Attached the latest patch. It didn't actually necessary to change
> GetSubscriptionNotReadyRelations. I just changed the logic refreshing
> the sync table state list.
> Please give me feedback.
> 

Hmm this might work. Although I was actually wondering if we could store
the last start timestamp in the worker shared memory and do some magic
with that (ie, not clearing subid and relid and try to then do rate
limiting based on subid+relid+timestamp stored in shmem). That would
then work same way for the main apply workers as well. It would have the
disadvantage that if some tables were consistently failing, no other
tables could get synchronized as the amount of workers is limited. OTOH
the approach chosen in the patch will first try all tables and only then
start rate limiting, not quite sure which is better.

-- 
  Petr Jelinek  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] logical replication apply to run with sync commit off by default

[Petr Jelinek]

On 12/04/17 06:10, Peter Eisentraut wrote:
> On 3/24/17 10:49, Petr Jelinek wrote:
>> Rebase after table copy patch got committed.
> 
> You could please sent another rebase of this, perhaps with some more
> documentation, as discussed downthread.
> 
> Also, I wonder why we don't offer the other values of
> synchronous_commit.  In any case, we should keep the values consistent.
> So on should be on and local should be local, but not mixing it.
> 

Now that I am back from vacation I took look at this again. The reason
why I did only boolean initially is that he other values just didn't
seem all that useful. But you are right it's better to be consistent and
there is no real reason why not to allow all of the possible values.

So how about the attached?

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services
From 40f16e060194280a3ce345a1cde57e22a9892007 Mon Sep 17 00:00:00 2001
From: Petr Jelinek 
Date: Mon, 6 Mar 2017 13:07:45 +0100
Subject: [PATCH] Add option to modify sync commit per subscription

This also changes default behaviour of subscription workers to
synchronous_commit = off
---
 doc/src/sgml/catalogs.sgml | 10 ++
 doc/src/sgml/ref/alter_subscription.sgml   |  2 ++
 doc/src/sgml/ref/create_subscription.sgml  | 30 +++-
 src/backend/catalog/pg_subscription.c  |  8 +
 src/backend/commands/subscriptioncmds.c| 56 --
 src/backend/replication/logical/launcher.c |  2 +-
 src/backend/replication/logical/worker.c   |  8 +
 src/bin/pg_dump/pg_dump.c  | 11 +-
 src/bin/pg_dump/pg_dump.h  |  1 +
 src/bin/pg_dump/t/002_pg_dump.pl   |  2 +-
 src/bin/psql/describe.c|  5 ++-
 src/include/catalog/pg_subscription.h  |  7 ++--
 src/test/regress/expected/subscription.out | 27 +++---
 src/test/regress/sql/subscription.sql  |  3 +-
 14 files changed, 141 insertions(+), 31 deletions(-)

diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 5883673..2d878ad 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -6531,6 +6531,16 @@
  
 
  
+  subsynccommit
+  text
+  
+  
+   Contains value for synchronous_commit setting of the
+   subscription workers.
+  
+ 
+
+ 
   subconninfo
   text
   
diff --git a/doc/src/sgml/ref/alter_subscription.sgml 
b/doc/src/sgml/ref/alter_subscription.sgml
index 640fac0..f71ee38 100644
--- a/doc/src/sgml/ref/alter_subscription.sgml
+++ b/doc/src/sgml/ref/alter_subscription.sgml
@@ -26,6 +26,7 @@ ALTER SUBSCRIPTION name WITH ( where suboption can 
be:
 
 SLOT NAME = slot_name
+| SYNCHRONOUS_COMMIT = synchronous_commit
 
 ALTER SUBSCRIPTION name SET 
PUBLICATION publication_name [, 
...] { REFRESH WITH ( puboption [, 
... ] ) | NOREFRESH }
 ALTER SUBSCRIPTION name REFRESH 
PUBLICATION WITH ( puboption [, 
... ] )
@@ -91,6 +92,7 @@ ALTER SUBSCRIPTION name DISABLE

 CONNECTION 'conninfo'
 SLOT NAME = slot_name
+SYNCHRONOUS_COMMIT = synchronous_commit
 
  
   These clauses alter properties originally set by
diff --git a/doc/src/sgml/ref/create_subscription.sgml 
b/doc/src/sgml/ref/create_subscription.sgml
index 3410d6f..3fc010e 100644
--- a/doc/src/sgml/ref/create_subscription.sgml
+++ b/doc/src/sgml/ref/create_subscription.sgml
@@ -32,6 +32,7 @@ CREATE SUBSCRIPTION subscription_nameslot_name
 | COPY DATA | NOCOPY DATA
+| SYNCHRONOUS_COMMIT = synchronous_commit
 | NOCONNECT
 
  
@@ -148,7 +149,34 @@ CREATE SUBSCRIPTION subscription_name
 

-NOCONNECT
+SYNCHRONOUS_COMMIT = synchronous_commit
+
+ 
+  The value of this parameter overrides the
+   setting. The default value is
+  off.
+ 
+ 
+  It's safe to use off for logical replication because
+  in case of transaction loss on subscriber due to missing sync, the data
+  will be resent from publisher.
+ 
+ 
+  The logical replication worker will also report position of the writes
+  and flushes to the publisher so the publisher will wait on the actual
+  flush when doing synchronous logical replication. This however means
+  that setting the SYNCHRONOUS_COMMIT for subscriber
+  to off when the subscription is used for synchronous
+  replication may increase the latency for COMMIT on
+  the publisher. In this scenario it may be advantageous to set the
+  SYNCHRONOUS_COMMIT to local or
+  higher.
+ 
+
+   
+
+   
+NOCONNECT
 
  
   Instructs CREATE SUBSCRIPTION to skip the initial
diff --git a/src/backend/catalog/pg_subscription.c 
b/src/backend/catalog/pg_subscription.c
index f5ba9f6..4f294d3 100644
--- a/src/backend/catalog/pg_subscription.c
+++ b/src/backend/catalog/pg_subscription.c
@@ -85,6 +85,14 @@ GetSubscription(Oid subid, bool missing_ok)
Assert(!isnull

[HACKERS] Small patch for pg_basebackup argument parsing

[Pierre Ducroquet]

Hi

Yesterday while doing a few pg_basebackup, I realized that the integer
parameters were not properly checked against invalid input.
It is not a critical issue, but this could be misleading for an user who
writes -z max or -s 0.5…
I've attached the patch to this mail. Should I add it to the next commit fest
or is it not needed for such small patches ?

Thanks

 PierreFrom c5301085514b8e5acd9ffa5b10ae6521677b4d72 Mon Sep 17 00:00:00 2001
From: Pierre Ducroquet 
Date: Thu, 13 Apr 2017 23:25:51 +0200
Subject: [PATCH] Check the results of atoi in pg_basebackup

Passing invalid strings in integer parameters does not trigger any error
in pg_basebackup right now. This behaviour could be misleading and give
the impression that a setting is considered (-z max, -s 0.5) while it
was reset to 0 instead.
---
 src/bin/pg_basebackup/pg_basebackup.c | 11 ++-
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index 40ec0e17dc..e69dbd1ed6 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -87,7 +87,7 @@ static IncludeWal includewal = STREAM_WAL;
 static bool fastcheckpoint = false;
 static bool writerecoveryconf = false;
 static bool do_sync = true;
-static int	standby_message_timeout = 10 * 1000;		/* 10 sec = default */
+static long int	standby_message_timeout = 10 * 1000;		/* 10 sec = default */
 static pg_time_t last_progress_report = 0;
 static int32 maxrate = 0;		/* no limit by default */
 static char *replication_slot = NULL;
@@ -2063,6 +2063,7 @@ BaseBackup(void)
 int
 main(int argc, char **argv)
 {
+	char *strtol_endptr = NULL;
 	static struct option long_options[] = {
 		{"help", no_argument, NULL, '?'},
 		{"version", no_argument, NULL, 'V'},
@@ -2203,8 +2204,8 @@ main(int argc, char **argv)
 #endif
 break;
 			case 'Z':
-compresslevel = atoi(optarg);
-if (compresslevel < 0 || compresslevel > 9)
+compresslevel = strtol(optarg, &strtol_endptr, 10);
+if (compresslevel < 0 || compresslevel > 9 || (strtol_endptr != optarg + strlen(optarg))
 {
 	fprintf(stderr, _("%s: invalid compression level \"%s\"\n"),
 			progname, optarg);
@@ -2242,8 +2243,8 @@ main(int argc, char **argv)
 dbgetpassword = 1;
 break;
 			case 's':
-standby_message_timeout = atoi(optarg) * 1000;
-if (standby_message_timeout < 0)
+standby_message_timeout = strtol(optarg, &strtol_endptr, 10) * 1000;
+if ((standby_message_timeout < 0) || (strtol_endptr != optarg + strlen(optarg)))
 {
 	fprintf(stderr, _("%s: invalid status interval \"%s\"\n"),
 			progname, optarg);
--
2.11.0



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

Re: [HACKERS] Row Level Security UPDATE Confusion

[Stephen Frost]

Rod, all,

* Joe Conway (m...@joeconway.com) wrote:
> On 04/13/2017 01:31 PM, Stephen Frost wrote:
> > * Robert Haas (robertmh...@gmail.com) wrote:
> >> On Thu, Apr 6, 2017 at 4:05 PM, Rod Taylor  wrote:
> >> > I'm a little confused on why a SELECT policy fires against the NEW record
> >> > for an UPDATE when using multiple FOR policies. The ALL policy doesn't 
> >> > seem
> >> > to have that restriction.
> >> 
> >> My guess is that you have found a bug.
> > 
> > Indeed.  Joe's been looking into it and I'm hoping to find some time to
> > dig into it shortly.
> 
> >> CREATE POLICY split_select ON t FOR SELECT TO split
> >> USING (value > 0);
> >> CREATE POLICY split_update ON t FOR UPDATE TO split
> >> USING (true) WITH CHECK (true);
> 
> Yes -- from what I can see in gdb:

Actually, looking at this again, the complaint appears to be that you
can't "give away" records.  That was a topic of much discussion and I'm
reasonably sure that was what we ended up deciding made the most sense.
You have to be able to see records to be able to update them (you can't
update records you can't see), and you have to be able to see the result
of your update.  I don't doubt that we could improve the documentation
around this (and apparently the code comments, according to Joe..).

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Row Level Security UPDATE Confusion

[Joe Conway]

On 04/13/2017 01:31 PM, Stephen Frost wrote:
> * Robert Haas (robertmh...@gmail.com) wrote:
>> On Thu, Apr 6, 2017 at 4:05 PM, Rod Taylor  wrote:
>> > I'm a little confused on why a SELECT policy fires against the NEW record
>> > for an UPDATE when using multiple FOR policies. The ALL policy doesn't seem
>> > to have that restriction.
>> 
>> My guess is that you have found a bug.
> 
> Indeed.  Joe's been looking into it and I'm hoping to find some time to
> dig into it shortly.


>> CREATE POLICY split_select ON t FOR SELECT TO split
>> USING (value > 0);
>> CREATE POLICY split_update ON t FOR UPDATE TO split
>> USING (true) WITH CHECK (true);

Yes -- from what I can see in gdb:

1) add_with_check_options() adds both (value > 0) and (true) to
   withCheckOptions -- this seems correct as the USING expression
   is used for WITH CHECK when the latter is not specified

2) ExecWithCheckOptions() checks (value > 0) which fails, and it
   immediately throws an ERROR, i.e. it never checks the (true)
   expression and therefore never ORs the results -- this seems
   incorrect, it uses restrictive not permissive

Joe

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



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] Cutting initdb's runtime (Perl question embedded)

[Andres Freund]

On 2017-04-13 14:05:43 -0400, Tom Lane wrote:
> Andres Freund  writes:
> > On 2017-04-13 12:56:14 -0400, Tom Lane wrote:
> >> Andres Freund  writes:
> >>> Cool.  I wonder if we also should remove AtEOXact_CatCache()'s
> >>> cross-checks - the resowner replacement has been in place for a while,
> >>> and seems robust enough.  They're now the biggest user of time.
> 
> >> Hm, biggest user of time in what workload?  I've not noticed that
> >> function particularly.
> 
> > Just initdb.  I presume it's because the catcaches will frequently be
> > relatively big there.
> 
> Hm.  That ties into something I was looking at yesterday.  The only
> reason that function is called so much is that bootstrap mode runs a
> separate transaction for *each line of the bki file* (cf do_start,
> do_end in bootparse.y).  Which seems pretty silly.

Indeed.


> On the whole, though, we may be looking at diminishing returns here.
> I just did some "perf" measurement of the overall "initdb" cycle,
> and what I'm seeing suggests that bootstrap mode as such is now a
> pretty small fraction of the overall cycle:
> 
> +   51.07% 0.01%28  postgres postgres 
>  [.] PostgresMain  #
> ...
> +   13.52% 0.00% 0  postgres postgres 
>  [.] AuxiliaryProcessMain  #
> 
> That says that the post-bootstrap steps are now the bulk of the time,
> which agrees with naked-eye observation.

The AtEOXact_CatCache weren't only in bootstrap mode, but yea, it's by
far smaller wins in comparison to the regprocin thing.

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] [POC] hash partitioning

[Robert Haas]

On Fri, Mar 17, 2017 at 7:57 AM, Yugo Nagata  wrote:
> I also understanded that my design has a problem during pg_dump and
> pg_upgrade, and that some information to identify the partition
> is required not depending the command order. However, I feel that
> Amul's design is a bit complicated with the rule to specify modulus.
>
> I think we can use simpler syntax, for example, as below.
>
>  CREATE TABLE h1 PARTITION OF h FOR (0);
>  CREATE TABLE h2 PARTITION OF h FOR (1);
>  CREATE TABLE h3 PARTITION OF h FOR (2);

I don't see how that can possibly work.  Until you see all the table
partitions, you don't know what the partitioning constraint for any
given partition should be, which seems to me to be a fatal problem.

I agree that Amul's syntax - really, I proposed it to him - is not the
simplest, but I think all the details needed to reconstruct the
partitioning constraint need to be explicit.  Otherwise, I'm pretty
sure things we're going to have lots of problems that we can't really
solve cleanly.  We can later invent convenience syntax that makes
common configurations easier to set up, but we should invent the
syntax that spells out all the details first.

-- 
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] Quals not pushed down into lateral

[Andres Freund]

On 2017-04-13 16:34:12 -0400, Robert Haas wrote:
> On Thu, Mar 16, 2017 at 4:45 AM, Andres Freund  wrote:
> > During citus development we noticed that restrictions aren't pushed down
> > into lateral subqueries, even if they semantically could.  For example,
> > in this dumbed down example:
> >
> > postgres[31776][1]=# CREATE TABLE t_2(id serial primary key);
> > postgres[31776][1]=# CREATE TABLE t_1(id serial primary key);
> >
> > Comparing:
> >
> > postgres[31776][1]=# EXPLAIN SELECT * FROM t_1 JOIN (SELECT * FROM t_2 
> > GROUP BY id) s ON (t_1.id = s.id) WHERE t_1.id = 3;
> > postgres[31776][1]=# EXPLAIN SELECT * FROM t_1, LATERAL (SELECT * FROM t_2 
> > WHERE t_1.id = t_2.id GROUP BY id) s WHERE t_1.id = 3;
> 
> Interesting.  That does seem like we are missing a trick.

Yea.

> Not exactly related, but I think we need to improve optimization
> around CTEs, too.  AFAICT, what we've got right now, almost everybody
> hates.

That's certainly an issue, but it's a lot harder to resolve because
we've, for years, told people to intentionally use CTEs as optimization
barriers :(

- 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] Quals not pushed down into lateral

[Robert Haas]

On Thu, Mar 16, 2017 at 4:45 AM, Andres Freund  wrote:
> During citus development we noticed that restrictions aren't pushed down
> into lateral subqueries, even if they semantically could.  For example,
> in this dumbed down example:
>
> postgres[31776][1]=# CREATE TABLE t_2(id serial primary key);
> postgres[31776][1]=# CREATE TABLE t_1(id serial primary key);
>
> Comparing:
>
> postgres[31776][1]=# EXPLAIN SELECT * FROM t_1 JOIN (SELECT * FROM t_2 GROUP 
> BY id) s ON (t_1.id = s.id) WHERE t_1.id = 3;
> postgres[31776][1]=# EXPLAIN SELECT * FROM t_1, LATERAL (SELECT * FROM t_2 
> WHERE t_1.id = t_2.id GROUP BY id) s WHERE t_1.id = 3;

Interesting.  That does seem like we are missing a trick.

Not exactly related, but I think we need to improve optimization
around CTEs, too.  AFAICT, what we've got right now, almost everybody
hates.

-- 
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] Row Level Security UPDATE Confusion

[Stephen Frost]

* Robert Haas (robertmh...@gmail.com) wrote:
> On Thu, Apr 6, 2017 at 4:05 PM, Rod Taylor  wrote:
> > I'm a little confused on why a SELECT policy fires against the NEW record
> > for an UPDATE when using multiple FOR policies. The ALL policy doesn't seem
> > to have that restriction.
> 
> My guess is that you have found a bug.

Indeed.  Joe's been looking into it and I'm hoping to find some time to
dig into it shortly.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Robert Haas]

On Mon, Apr 3, 2017 at 3:32 PM, Daniel Verite  wrote:
> In interactive mode, the warning in untaken branches is misleading
> when \endif is on the same line as the commands that
> are skipped. For instance:
>
>   postgres=# \if false \echo NOK \endif
>   \echo command ignored; use \endif or Ctrl-C to exit current \if block
>   postgres=#
>
> From the point of view of the user, the execution flow has exited
> the branch already when this warning is displayed.
> Of course issuing the recommended \endif at this point doesn't work:
>
>   postgres=# \endif
>   \endif: no matching \if
>
> Maybe that part of the message:
> "use \endif or Ctrl-C to exit current \if block"
> should be displayed only when coming back at the prompt,
> and if the flow is still in an untaken branch at this point?

Is this an open item, or do we not care about fixing it?

-- 
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] Declarative partitioning vs. information_schema

[Robert Haas]

On Thu, Apr 6, 2017 at 3:14 AM, Amit Langote
 wrote:
>> The bulk of operations that work on traditional tables also work on 
>> partitions
>> and partitioned tables.  The next closest kind of relation, a materialized
>> view, is far less table-like.  Therefore, I recommend showing both partitions
>> and partitioned tables in those views.  This is also consistent with the
>> decision to use words like "partition" and "partitioned" in messages only 
>> when
>> partitioning is relevant to the error.  For example, ATWrongRelkindError()
>> distinguishes materialized views from tables, but it does not distinguish
>> tables based on their participation in partitioning.
>
> +1

OK, whoever wants to write the patch, please step forward.

/me steps backward.

-- 
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] pg_upgrade vs extension upgrades

[Robert Haas]

On Thu, Apr 13, 2017 at 3:48 PM, Magnus Hagander  wrote:
>> Well, pg_upgrade creates ./analyze_new_cluster.sh, but that just
>> contains:
>>
>> "/u/pgsql/bin/vacuumdb" --all --analyze-in-stages
>>
>> Seems like we should just get rid of ./analyze_new_cluster.sh and tell
>> the user to run vacuumdb directly.  I guess I will have to wait for PG
>> 11 to do that though.
>
> Yeah, at this point that probably makes a lot of sense, now that we don't
> need the logic in the script anymore.
>
> FWIW, I'm not sure the feature freeze means we can't *remove* a feature? But
> I'll defer to others on that.

I would view this as unnecessary tinkering that could just as well
wait until v11, although it's such a small change that I am not
prepared to spend much time arguing if you're determined to force it
through.  The point of the feature freeze is to focus effort on the
things that we need to do in order to be able to get a beta and then a
final release out the door, and to get them out the door on time and
with adequate quality.  Whether or not a change which doesn't further
those goals is technically speaking a feature isn't really relevant in
my view.  The point is to mop up the loose ends and ship, and to avoid
things that have a chance of creating new loose ends.

-- 
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] Row Level Security UPDATE Confusion

[Robert Haas]

On Thu, Apr 6, 2017 at 4:05 PM, Rod Taylor  wrote:
> I'm a little confused on why a SELECT policy fires against the NEW record
> for an UPDATE when using multiple FOR policies. The ALL policy doesn't seem
> to have that restriction.

My guess is that you have found a bug.

-- 
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] pg_upgrade vs extension upgrades

[Magnus Hagander]

On Thu, Apr 13, 2017 at 9:13 PM, Bruce Momjian  wrote:

> On Thu, Apr 13, 2017 at 09:02:27PM +0200, Magnus Hagander wrote:
> > On Thu, Apr 13, 2017 at 12:30 AM, Peter Eisentraut <
> > peter.eisentr...@2ndquadrant.com> wrote:
> >
> > On 4/10/17 11:30, Magnus Hagander wrote:
> > > After you've run pg_upgrade, you have to loop through all your
> databases
> > > and do an "ALTER EXTENSION abc UPDATE" once for each extension.
> > >
> > > Is there a reason we shouldn't have pg_upgrade emit a script that
> does
> > > this, similar to how it emits a script to run ANALYZE?
> >
> > Shouldn't pg_dump do this, and perhaps by default?
> >
> >
> > If I restore a dump into another instance, I need to upgrade all my
> > extensions to that installations's versions, no?  That's not
> particular
> > to pg_upgrade.
> >
> > Sure, there's an argument to be made for that.  But pg_dump (or in this
> case,
> > it would more be pg_restore I guess) also doesn't run ANALYZE or
> generate a
> > script to do that, does it? ISTM that we have already decided that
> pg_upgrade
> > has a different requirement on providing those things, whereas pg_dump/
> > pg_restore is more of a low-level tool where people have to figure more
> things
> > out themselves.
>
> Well, pg_upgrade creates ./analyze_new_cluster.sh, but that just
> contains:
>
> "/u/pgsql/bin/vacuumdb" --all --analyze-in-stages
>
> Seems like we should just get rid of ./analyze_new_cluster.sh and tell
> the user to run vacuumdb directly.  I guess I will have to wait for PG
> 11 to do that though.
>
>
Yeah, at this point that probably makes a lot of sense, now that we don't
need the logic in the script anymore.

FWIW, I'm not sure the feature freeze means we can't *remove* a feature?
But I'll defer to others on that.

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

[HACKERS] some review comments on logical rep code

[Fujii Masao]

Hi,

Though I've read only a part of the logical rep code yet, I'd like to
share some (relatively minor) review comments that I got so far.

In ApplyWorkerMain(), DatumGetInt32() should be used to get integer
value from the argument, instead of DatumGetObjectId().

No one resets on_commit_launcher_wakeup flag to false.

ApplyLauncherWakeup() should be static function.

Seems not only CREATE SUBSCRIPTION but also ALTER SUBSCRIPTION's
subcommands (e.g., ENABLED one) should wake the launcher up on commit.

Why is IsBackendPid() check necessary in ApplyLauncherWakeup()?

Normal statements that the walsender for logical rep runs are logged
by log_replication_commands. So if log_statement is also set,
those commands are logged twice.

LogicalRepCtx->launcher_pid isn't reset to 0 when the launcher emits
an error. The callback function to reset it needs to be registered
via on_shmem_exit().

Typo: "an subscriber" should be "a subscriber" in some places.

/* Get conninfo */
datum = SysCacheGetAttr(SUBSCRIPTIONOID,
tup,
Anum_pg_subscription_subconninfo,
&isnull);
Assert(!isnull);

This assertion makes me think that pg_subscription.subconnifo should
have NOT NULL constraint. Also subslotname and subpublications
should have NOT NULL constraint because of the same reason.

SpinLockAcquire(&MyLogicalRepWorker->relmutex);
MyLogicalRepWorker->relstate =
  GetSubscriptionRelState(MyLogicalRepWorker->subid,
MyLogicalRepWorker->relid,
&MyLogicalRepWorker->relstate_lsn,
false);
SpinLockRelease(&MyLogicalRepWorker->relmutex);

Non-"short-term" function like GetSubscriptionRelState() should not
be called while spinlock is being held.

Regards,

-- 
Fujii Masao


-- 
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_upgrade vs extension upgrades

[Bruce Momjian]

On Thu, Apr 13, 2017 at 09:02:27PM +0200, Magnus Hagander wrote:
> On Thu, Apr 13, 2017 at 12:30 AM, Peter Eisentraut <
> peter.eisentr...@2ndquadrant.com> wrote:
> 
> On 4/10/17 11:30, Magnus Hagander wrote:
> > After you've run pg_upgrade, you have to loop through all your databases
> > and do an "ALTER EXTENSION abc UPDATE" once for each extension.
> >
> > Is there a reason we shouldn't have pg_upgrade emit a script that does
> > this, similar to how it emits a script to run ANALYZE?
> 
> Shouldn't pg_dump do this, and perhaps by default? 
> 
> 
> If I restore a dump into another instance, I need to upgrade all my
> extensions to that installations's versions, no?  That's not particular
> to pg_upgrade.
>
> Sure, there's an argument to be made for that.  But pg_dump (or in this case,
> it would more be pg_restore I guess) also doesn't run ANALYZE or generate a
> script to do that, does it? ISTM that we have already decided that pg_upgrade
> has a different requirement on providing those things, whereas pg_dump/
> pg_restore is more of a low-level tool where people have to figure more things
> out themselves.

Well, pg_upgrade creates ./analyze_new_cluster.sh, but that just
contains:

"/u/pgsql/bin/vacuumdb" --all --analyze-in-stages

Seems like we should just get rid of ./analyze_new_cluster.sh and tell
the user to run vacuumdb directly.  I guess I will have to wait for PG
11 to do that though.

-- 
  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_upgrade vs extension upgrades

[Magnus Hagander]

On Thu, Apr 13, 2017 at 6:06 PM, Robert Haas  wrote:

> On Thu, Apr 13, 2017 at 11:48 AM, Peter Eisentraut
>  wrote:
> > On 4/12/17 22:56, Bruce Momjian wrote:
> >> Is pg_upgrade the right place for an extension upgrade script?  When
> >> pg_upgrade started creating an incremental-analyze script, people
> >> thought it should be more generic so it was moved to vacuumdb
> >> --analyze-in-stages.  Seems we should do the same thing for the
> >> extension upgrade script.
> >
> > Yeah, many of the things that pg_upgrade would do or suggest after an
> > upgrade could also apply to other upgrade methods, such as by logical
> > replication.  So offering them separately would be good.
>
> And in theory, extension upgrades could happen any time, not just when
> there's a major version upgrade.  You could be using a
> separately-bundled extension, or we could bump an extension version in
> a minor release to fix some issue.
>
> I think we should invent a clever name for a new utility, like
> pg_maintain or something.  It could let you know about (and optionally
> perform) a variety of optional maintenance tasks:
>
> - upgrading extensions
> - reindexing indexes for which the version has been bumped, like hash
> indexes in v10
> - reindexing or dropping indexes marked invalid that were left behind
> by a CIC failure
> - analyzing tables that lack (full?) statistics
> - triggering heap scans to reclaim HEAP_MOVED_* bits, if we have that
> kind of thing someday
>
>
I agree it makes a lot of sense to have a separate tool that can do these
things, and that pg_upgrade can point the user towards it.

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

Re: [HACKERS] pg_upgrade vs extension upgrades

[Magnus Hagander]

On Thu, Apr 13, 2017 at 12:30 AM, Peter Eisentraut <
peter.eisentr...@2ndquadrant.com> wrote:

> On 4/10/17 11:30, Magnus Hagander wrote:
> > After you've run pg_upgrade, you have to loop through all your databases
> > and do an "ALTER EXTENSION abc UPDATE" once for each extension.
> >
> > Is there a reason we shouldn't have pg_upgrade emit a script that does
> > this, similar to how it emits a script to run ANALYZE?
>
> Shouldn't pg_dump do this, and perhaps by default?


> If I restore a dump into another instance, I need to upgrade all my
> extensions to that installations's versions, no?  That's not particular
> to pg_upgrade.
>
>
Sure, there's an argument to be made for that.  But pg_dump (or in this
case, it would more be pg_restore I guess) also doesn't run ANALYZE or
generate a script to do that, does it? ISTM that we have already decided
that pg_upgrade has a different requirement on providing those things,
whereas pg_dump/pg_restore is more of a low-level tool where people have to
figure more things out themselves.


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

Re: [HACKERS] Cutting initdb's runtime (Perl question embedded)

[Tom Lane]

Andres Freund  writes:
> On 2017-04-13 12:56:14 -0400, Tom Lane wrote:
>> Andres Freund  writes:
>>> Cool.  I wonder if we also should remove AtEOXact_CatCache()'s
>>> cross-checks - the resowner replacement has been in place for a while,
>>> and seems robust enough.  They're now the biggest user of time.

>> Hm, biggest user of time in what workload?  I've not noticed that
>> function particularly.

> Just initdb.  I presume it's because the catcaches will frequently be
> relatively big there.

Hm.  That ties into something I was looking at yesterday.  The only
reason that function is called so much is that bootstrap mode runs a
separate transaction for *each line of the bki file* (cf do_start,
do_end in bootparse.y).  Which seems pretty silly.  I experimented
with collapsing all the transactions for consecutive DATA lines into
one transaction, but couldn't immediately make it work due to memory
management issues.  I didn't try collapsing the entire run into a
single transaction, but maybe that would actually be easier, though
no doubt more wasteful of memory.

>> I agree that it doesn't seem like we need to spend a lot of time
>> cross-checking there, though.  Maybe keep the code but #ifdef it
>> under some nondefault debugging symbol.

> Hm, if we want to keep it, maybe tie it to CLOBBER_CACHE_ALWAYS or such,
> so it gets compiled at least sometimes? Not a great fit, but ...

Don't like that, because CCA is by definition not the normal cache
behavior.  It would make a bit of sense to tie it to CACHEDEBUG,
but as you say, it'd never get tested normally if we do that.

On the whole, though, we may be looking at diminishing returns here.
I just did some "perf" measurement of the overall "initdb" cycle,
and what I'm seeing suggests that bootstrap mode as such is now a
pretty small fraction of the overall cycle:

+   51.07% 0.01%28  postgres postgres   
   [.] PostgresMain  #
...
+   13.52% 0.00% 0  postgres postgres   
   [.] AuxiliaryProcessMain  #

That says that the post-bootstrap steps are now the bulk of the time,
which agrees with naked-eye observation.

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] Undefined psql variables

[Pavel Stehule]

2017-04-13 19:46 GMT+02:00 Corey Huinker :

>
>> > I suggest to reuse pgbench expression engine, developed by Haas
>> Robert:-)
>>
>> Not a bad idea (though I'm sure there are other reasonable options, too).
>>
>>
> I don't want to stand in the way of any progress in getting expressions
> into \if and some subspecies of \set. But, assuming we don't get it into
> v10, our documentations currently says this about expressions:
>
> Expressions that do not properly evaluate to true or false will generate a
> warning and be treated as false.
>
>
> We should probably amend that to say something about the potential future
> directions of expressions, perhaps this:
>
> Expressions that do not properly evaluate to true or false will generate a
> warning and be treated as false, though that behavior is subject to change
> in future versions of psql.
>
>
> That should guard us against people getting too attached to that behavior
> in the interim.
>
> With that said, I'm starting to like the idea of not boxing ourselves into
> one type of expression. Maybe the first token could be the expression
> context with the expression to follow
>
> The expression type we have now
> \if true
>
> "defined" is a context (or "mode") that expects one token that might be a
> psql varname
> \if defined varname
>
> "sql" is a context that treats the rest of the line as a SQL statement to
> the current connection, and looks at the first column of first row for
> "truth"
> \if sql SELECT EXISTS(SELECT null FROM item WHERE manufacturer = 'Frobozz')
>
> "pgbench" could invoke the pgbench expression engine.
> \if pgbench 
>
> Anything else is treated as an external expression evaluator. If an
> expression has an unknown context "foo", check the ENV vars for EXPR_FOO,
> and pipe the remaining expression tokens to $PSQL_EXPR_FOO if it exists,
> and read the output of that for psql-boolean truth.  I think having a
> context/mode token could allow us to have lots of pluggable expression
> types with minimal effort to psql itself.
>
> "python" invokes a python interpreter (if PSQL_EXPR_PYTHON is defined,
> fails otherwise)
> \if python print(:'varname' == 'Approved' or :'othervar' == 'Special')
> which would echo
> print('Approved' == 'Approved' or 'Regular' == 'Special')
> to python, which would give the response "True", which is true
>
> likewise with "bash" (assuming PSQL_EXPR_BASH=bash)
> \if bash expr :'varname' = 'Approved'
> would echo
> expr 'Approved' = 'Approved'
> to bash, which would return 1, which would be true.
>
> So we'd get all that, with only having to internally code for an external
> launcher, naked booleans, pgbench, defined, and I suppose we should have a
> negation
>
>
> \if not  
>
>
> Which I guess would allow
>
> \if not not not  
>
>
it looks like overengineering - I don't think so string comparation should
be supported in two three languages.

Can live with it, but more prefer simple pgbench only language there

The one line commands is limited due readability

we can introduce "language blocks" where can be possible set some values.
This looks really scary.

bash is supported already

probably you can write today

\if `basexpr`





>
> For consistency, we might want to change the default context to require an
> explicit "bool", so
>
> \if bool true
>
> but if we want to do that, we should change it very soon.
>
> tl;dr:
> My proposal is:
> * do the bare minimum of expression testing in psql (simple scalar truth,
> variable definition, negation)
> * do platform independent client-only-expressions in pgbench mode
> * allow inline \gset-ish expressions with sql-mode
> * and allow for platform/install dependent expressions via PSQL_EXPR_* env
> vars.
>

Re: [HACKERS] logical replication and PANIC during shutdown checkpoint in publisher

[Fujii Masao]

On Thu, Apr 13, 2017 at 12:36 PM, Michael Paquier
 wrote:
> On Thu, Apr 13, 2017 at 12:28 PM, Fujii Masao  wrote:
>> On Thu, Apr 13, 2017 at 5:25 AM, Peter Eisentraut
>>  wrote:
>>> On 4/12/17 09:55, Fujii Masao wrote:
 To fix this issue, we should terminate walsender for logical replication
 before shutdown checkpoint starts. Of course walsender for physical
 replication still needs to keep running until shutdown checkpoint ends,
 though.
>>>
>>> Can we turn it into a kind of read-only or no-new-commands mode instead,
>>> so it can keep streaming but not accept any new actions?
>>
>> So we make walsenders switch to that mode and wait for all the 
>> already-ongoing
>> their "write" commands to finish, and then we start a shutdown checkpoint?
>> This is an idea, but seems a bit complicated. ISTM that it's simpler to
>> terminate only walsenders for logical rep before shutdown checkpoint.
>
> Perhaps my memory is failing me here... But in clean shutdowns we do
> shut down WAL senders after the checkpoint has completed so as we are
> sure that they have flushed the LSN corresponding to the checkpoint
> ending, right?

Yes.

> Why introducing an inconsistency for logical workers?
> It seems to me that logical workers should fail under the same rules.

Could you tell me why? You think that even walsender for logical rep
needs to stream the shutdown checkpoint WAL record to the subscriber?
I was not thinking that's true.

Regards,

-- 
Fujii Masao


-- 
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] Interval for launching the table sync worker

[Petr Jelinek]

On 13/04/17 13:01, Kyotaro HORIGUCHI wrote:
> Ouch! I replied to wrong mail.
> 
> At Thu, 13 Apr 2017 19:55:04 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI 
>  wrote in 
> <20170413.195504.89348773.horiguchi.kyot...@lab.ntt.co.jp>
>> I confused sync and apply workers.
>> sync worker failure at start causes immediate retries.
>>
>> At Thu, 13 Apr 2017 11:53:27 +0900, Masahiko Sawada  
>> wrote in 
>>> On Wed, Apr 12, 2017 at 11:46 PM, Peter Eisentraut
>>>  wrote:
 On 4/12/17 00:48, Masahiko Sawada wrote:
> On Wed, Apr 12, 2017 at 1:28 PM, Peter Eisentraut
>> Perhaps instead of a global last_start_time, we store a per relation
>> last_start_time in SubscriptionRelState?
>
> I was thinking the same. But a problem is that the list of
> SubscriptionRelState is refreshed whenever the syncing table state
> becomes invalid (table_state_valid = false). I guess we need to
> improve these logic including GetSubscriptionNotReadyRelations().

 The table states are invalidated on a syscache callback from
 pg_subscription_rel, which happens roughly speaking when a table
 finishes the initial sync.  So if we're worried about failing tablesync
 workers relaunching to quickly, this would only be a problem if a
 tablesync of another table finishes right in that restart window.  That
 doesn't seem a terrible issue to me.

>>>
>>> I think the table states are invalidated whenever the table sync
>>> worker starts, because the table sync worker updates its status of
>>> pg_subscription_rel and commits it before starting actual copy. So we
>>> cannot rely on that. I thought we can store last_start_time into
>>> pg_subscription_rel but it might be overkill. I'm now thinking to
>>> change GetSubscriptionNotReadyRealtions so that last_start_time in
>>> SubscriptionRelState is taken over to new list.
> 
> The right target of "This" below is found at the following URL.
> 
> https://www.postgresql.org/message-id/CAD21AoBt_XUdppddFak661_LBM2t3CfK52aLKHG%2Bekd7SkzLmg%40mail.gmail.com
> 
>> This resolves the problem but, if I understand correctly, the
>> many pallocs in process_syncing_tables_for_apply() is working on
>> ApplyContext and the context is reset before the next visit here
>> (in LogicalRepApplyLoop).
>>
>> Although this is not a problem of this patch, this is a problem
>> generally.

Huh? We explicitly switch to CacheMemoryContext before pallocing
anything that should survive long term.

-- 
  Petr Jelinek  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] snapbuild woes

[Petr Jelinek]

On 13/04/17 07:02, Andres Freund wrote:
> 
> On April 12, 2017 9:58:12 PM PDT, Noah Misch  wrote:
>> On Wed, Apr 12, 2017 at 10:21:51AM -0700, Andres Freund wrote:
>>> On 2017-04-12 11:03:57 -0400, Peter Eisentraut wrote:
 On 4/12/17 02:31, Noah Misch wrote:
>>> But I hope you mean to commit these snapbuild patches before
>> the postgres 10
>>> release?  As far as I know, logical replication is still very
>> broken without
>>> them (or at least some of that set of 5 patches - I don't know
>> which ones
>>> are essential and which may not be).
>>
>> Yes, these should go into 10 *and* earlier releases, and I don't
>> plan to
>> wait for 2017-07.
>
> [Action required within three days.  This is a generic
>> notification.]

 I'm hoping for a word from Andres on this.
>>>
>>> Feel free to reassign to me.
>>
>> Thanks for volunteering; I'll do that shortly.  Please observe the
>> policy on
>> open item ownership[1] and send a status update within three calendar
>> days of
>> this message.  Include a date for your subsequent status update.
>>
>> [1]
>> https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com
> 
> Will, volunteering might be the wrong word.  These ate all my fault, although 
> none look v10 specific.
> 

Yeah none of this is v10 specific, the importance to v10 is that it
affects the logical replication in core, not just extensions like in
9.4-9.6.

-- 
  Petr Jelinek  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] snapbuild woes

[Petr Jelinek]

Thanks for looking at this!

On 13/04/17 02:29, Andres Freund wrote:
> Hi,
> On 2017-03-03 01:30:11 +0100, Petr Jelinek wrote:
> 
>> From 7d5b48c8cb80e7c867b2096c999d08feda50b197 Mon Sep 17 00:00:00 2001
>> From: Petr Jelinek 
>> Date: Fri, 24 Feb 2017 21:39:03 +0100
>> Subject: [PATCH 1/5] Reserve global xmin for create slot snasphot export
>>
>> Otherwise the VACUUM or pruning might remove tuples still needed by the
>> exported snapshot.
>> ---
>>  src/backend/replication/logical/logical.c | 21 -
>>  1 file changed, 20 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/backend/replication/logical/logical.c 
>> b/src/backend/replication/logical/logical.c
>> index 5529ac8..57c392c 100644
>> --- a/src/backend/replication/logical/logical.c
>> +++ b/src/backend/replication/logical/logical.c
>> @@ -267,12 +267,18 @@ CreateInitDecodingContext(char *plugin,
>>   * the slot machinery about the new limit. Once that's done the
>>   * ProcArrayLock can be released as the slot machinery now is
>>   * protecting against vacuum.
>> + *
>> + * Note that we only store the global xmin temporarily in the in-memory
>> + * state so that the initial snapshot can be exported. After initial
>> + * snapshot is done global xmin should be reset and not tracked anymore
>> + * so we are fine with losing the global xmin after crash.
>>   * 
>>   */
>>  LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);
>>  
>>  slot->effective_catalog_xmin = GetOldestSafeDecodingTransactionId();
>>  slot->data.catalog_xmin = slot->effective_catalog_xmin;
>> +slot->effective_xmin = slot->effective_catalog_xmin;
> 
> 
>>  void
>>  FreeDecodingContext(LogicalDecodingContext *ctx)
>>  {
>> +ReplicationSlot *slot = MyReplicationSlot;
>> +
>>  if (ctx->callbacks.shutdown_cb != NULL)
>>  shutdown_cb_wrapper(ctx);
>>  
>> +/*
>> + * Cleanup global xmin for the slot that we may have set in
>> + * CreateInitDecodingContext().
> 
> Hm.  Is that actually a meaningful point to do so? For one, it gets
> called by pg_logical_slot_get_changes_guts(), but more importantly, the
> snapshot is exported till SnapBuildClearExportedSnapshot(), which is the
> next command?  If we rely on the snapshot magic done by ExportSnapshot()
> it'd be worthwhile to mention that...
> 

(Didn't see the patch for couple of months so don't remember all the
detailed decisions anymore)

Yes we rely on backend's xmin to be set for exported snapshot. We only
care about global xmin for exported snapshots really, I assumed that's
clear enough from "so that the initial snapshot can be exported" but I
guess there should be more clear comment about this where we actually
clean this up.

> 
>> We do not take ProcArrayLock or similar
>> + * since we only reset xmin here and there's not much harm done by a
>> + * concurrent computation missing that.
>> + */
> 
> Hum.  I was prepared to complain about this, but ISTM, that there's
> absolutely no risk because the following
> ReplicationSlotsComputeRequiredXmin(false); actually does all the
> necessary locking?  But still, I don't see much point in the
> optimization.
> 

Well, if we don't need it in LogicalConfirmReceivedLocation, I don't see
why we need it here. Please enlighten me.

> 
> 
>> This patch changes the code so that stored snapshots are only used for
>> logical decoding restart but not for initial slot snapshot.
> 
> Yea, that's a very good point...
> 
>> @@ -1284,13 +1286,13 @@ SnapBuildFindSnapshot(SnapBuild *builder, XLogRecPtr 
>> lsn, xl_running_xacts *runn
>>  
>>  return false;
>>  }
>> -/* c) valid on disk state */
>> -else if (SnapBuildRestore(builder, lsn))
>> +/* c) valid on disk state and not exported snapshot */
>> +else if (!TransactionIdIsNormal(builder->initial_xmin_horizon) &&
>> + SnapBuildRestore(builder, lsn))
> 
> Hm. Is this a good signaling mechanism? It'll also trigger for the SQL
> interface, where it'd strictly speaking not be required, right?
> 

Good point. Maybe we should really tell snapshot builder if the snapshot
is going to be exported or not explicitly (see the rant all the way down).

> 
>> From 3318a929e691870f3c1ca665bec3bfa8ea2af2a8 Mon Sep 17 00:00:00 2001
>> From: Petr Jelinek 
>> Date: Sun, 26 Feb 2017 01:07:33 +0100
>> Subject: [PATCH 3/5] Prevent snapshot builder xmin from going backwards
> 
> A bit more commentary would be good. What does that protect us against?
> 

I think I explained that in the email. We might export snapshot with
xmin smaller than global xmin otherwise.

> 
> 
>> From 53193b40f26dd19c712f3b9b77af55f81eb31cc4 Mon Sep 17 00:00:00 2001
>> From: Petr Jelinek 
>> Date: Wed, 22 Feb 2017 00:57:33 +0100
>> Subject: [PATCH 4/5] Fix xl_running_xacts usage in snapshot builder
>>
>> Due to race condition, the xl_running_xacts might contain no longer
>> running transactions. Previous coding tried to get around this by
>>

Re: [HACKERS] Undefined psql variables

[Pavel Stehule]

>
> > I suggest to reuse pgbench expression engine, developed by Haas Robert:-)
>
> Not a bad idea (though I'm sure there are other reasonable options, too).
>

I checked the pgbench code - and I think it can work well - just add
logical operators and compare operators.

Don't need to create more complex language there.


> > Generating a error message with ${foo:?} is nice, but what psql need is
> just
> > a way to test whether a variable is defined or not.
>
> I'm not saying we should slavishly follow bash, but don't confuse what
> you need with what other people need.  bash (well, sh, really) grew
> that syntax for a reason, and it may be more than "there was this one
> guy back in the seventies who wanted it, and ...".
>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>

Re: [HACKERS] Quorum commit for multiple synchronous replication.

[Fujii Masao]

On Thu, Apr 13, 2017 at 9:23 PM, Masahiko Sawada  wrote:
> On Thu, Apr 13, 2017 at 5:17 PM, Kyotaro HORIGUCHI
>  wrote:
>> Hello,
>>
>> At Thu, 6 Apr 2017 16:17:31 +0900, Masahiko Sawada  
>> wrote in 
>>> On Thu, Apr 6, 2017 at 10:51 AM, Noah Misch  wrote:
>>> > On Thu, Apr 06, 2017 at 12:48:56AM +0900, Fujii Masao wrote:
>>> >> On Wed, Apr 5, 2017 at 3:45 PM, Noah Misch  wrote:
>>> >> > On Mon, Dec 19, 2016 at 09:49:58PM +0900, Fujii Masao wrote:
>>> >> >> Regarding this feature, there are some loose ends. We should work on
>>> >> >> and complete them until the release.
>>> >> >>
>>> >> >> (1)
>>> >> >> Which synchronous replication method, priority or quorum, should be
>>> >> >> chosen when neither FIRST nor ANY is specified in s_s_names? Right 
>>> >> >> now,
>>> >> >> a priority-based sync replication is chosen for keeping backward
>>> >> >> compatibility. However some hackers argued to change this decision
>>> >> >> so that a quorum commit is chosen because they think that most users
>>> >> >> prefer to a quorum.
>>> >> >>
>>> >> >> (2)
>>> >> >> There will be still many source comments and documentations that
>>> >> >> we need to update, for example, in high-availability.sgml. We need to
>>> >> >> check and update them throughly.
>>> >> >>
>>> >> >> (3)
>>> >> >> The priority value is assigned to each standby listed in s_s_names
>>> >> >> even in quorum commit though those priority values are not used at 
>>> >> >> all.
>>> >> >> Users can see those priority values in pg_stat_replication.
>>> >> >> Isn't this confusing? If yes, it might be better to always assign 1 as
>>> >> >> the priority, for example.
>>> >> >
>>> >> > [Action required within three days.  This is a generic notification.]
>>> >> >
>>> >> > The above-described topic is currently a PostgreSQL 10 open item.  
>>> >> > Fujii,
>>> >> > since you committed the patch believed to have created it, you own 
>>> >> > this open
>>> >> > item.  If some other commit is more relevant or if this does not 
>>> >> > belong as a
>>> >> > v10 open item, please let us know.  Otherwise, please observe the 
>>> >> > policy on
>>> >> > open item ownership[1] and send a status update within three calendar 
>>> >> > days of
>>> >> > this message.  Include a date for your subsequent status update.  
>>> >> > Testers may
>>> >> > discover new open items at any time, and I want to plan to get them 
>>> >> > all fixed
>>> >> > well in advance of shipping v10.  Consequently, I will appreciate your 
>>> >> > efforts
>>> >> > toward speedy resolution.  Thanks.
>>> >> >
>>> >> > [1] 
>>> >> > https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com
>>> >>
>>> >> Thanks for the notice!
>>> >>
>>> >> Regarding the item (2), Sawada-san told me that he will work on it after
>>> >> this CommitFest finishes. So we would receive the patch for the item from
>>> >> him next week. If there will be no patch even after the end of next week
>>> >> (i.e., April 14th), I will. Let's wait for Sawada-san's action at first.
>>> >
>>> > Sounds reasonable; I will look for your update on 14Apr or earlier.
>>> >
>>> >> The items (1) and (3) are not bugs. So I don't think that they need to be
>>> >> resolved before the beta release. After the feature freeze, many users
>>> >> will try and play with many new features including quorum-based syncrep.
>>> >> Then if many of them complain about (1) and (3), we can change the code
>>> >> at that timing. So we need more time that users can try the feature.
>>> >
>>> > I've moved (1) to a new section for things to revisit during beta.  If 
>>> > someone
>>> > feels strongly that the current behavior is Wrong and must change, speak 
>>> > up as
>>> > soon as you reach that conclusion.  Absent such arguments, the behavior 
>>> > won't
>>> > change.
>>> >
>>> >> BTW, IMO (3) should be fixed so that pg_stat_replication reports NULL
>>> >> as the priority if quorum-based sync rep is chosen. It's less confusing.
>>> >
>>> > Since you do want (3) to change, please own it like any other open item,
>>> > including the mandatory status updates.
>>>
>>> I agree to report NULL as the priority. I'll send a patch for this as well.
>>
>>
>> In the comment,
>
> Thank you for reviewing!
>
>>
>> +  /*
>> +   * The priority appers NULL as it is not used in quorum-based
>> +   * sync replication.
>> +   */
>>
>> appers should be appears. But the comment would be better to be
>> something follows.
>
> Will fix.
>
>>
>> "The priority value is useless for quorum-based sync replication" or
>>
>> "The priority field is NULL for quorum-based sync replication
>>  since the value is useless."
>>
>> Or, or, or.. something other.
>
> Will fix with later part.
>
>>
>>
>> This part,
>>
>> +if (SyncRepConfig &&
>> +SyncRepConfig->syncrep_method == SYNC_REP_QUORUM)
>> +nulls[9] = true;
>> +else
>> +values[9] = Int32GetDatum(priority);
>>
>> I looked on how syncrep_method is used in the code and found t

Re: [HACKERS] Undefined psql variables

[Corey Huinker]

>
>
> > I suggest to reuse pgbench expression engine, developed by Haas Robert:-)
>
> Not a bad idea (though I'm sure there are other reasonable options, too).
>
>
I don't want to stand in the way of any progress in getting expressions
into \if and some subspecies of \set. But, assuming we don't get it into
v10, our documentations currently says this about expressions:

Expressions that do not properly evaluate to true or false will generate a
warning and be treated as false.


We should probably amend that to say something about the potential future
directions of expressions, perhaps this:

Expressions that do not properly evaluate to true or false will generate a
warning and be treated as false, though that behavior is subject to change
in future versions of psql.


That should guard us against people getting too attached to that behavior
in the interim.

With that said, I'm starting to like the idea of not boxing ourselves into
one type of expression. Maybe the first token could be the expression
context with the expression to follow

The expression type we have now
\if true

"defined" is a context (or "mode") that expects one token that might be a
psql varname
\if defined varname

"sql" is a context that treats the rest of the line as a SQL statement to
the current connection, and looks at the first column of first row for
"truth"
\if sql SELECT EXISTS(SELECT null FROM item WHERE manufacturer = 'Frobozz')

"pgbench" could invoke the pgbench expression engine.
\if pgbench 

Anything else is treated as an external expression evaluator. If an
expression has an unknown context "foo", check the ENV vars for EXPR_FOO,
and pipe the remaining expression tokens to $PSQL_EXPR_FOO if it exists,
and read the output of that for psql-boolean truth.  I think having a
context/mode token could allow us to have lots of pluggable expression
types with minimal effort to psql itself.

"python" invokes a python interpreter (if PSQL_EXPR_PYTHON is defined,
fails otherwise)
\if python print(:'varname' == 'Approved' or :'othervar' == 'Special')
which would echo
print('Approved' == 'Approved' or 'Regular' == 'Special')
to python, which would give the response "True", which is true

likewise with "bash" (assuming PSQL_EXPR_BASH=bash)
\if bash expr :'varname' = 'Approved'
would echo
expr 'Approved' = 'Approved'
to bash, which would return 1, which would be true.

So we'd get all that, with only having to internally code for an external
launcher, naked booleans, pgbench, defined, and I suppose we should have a
negation


\if not  


Which I guess would allow

\if not not not  


For consistency, we might want to change the default context to require an
explicit "bool", so

\if bool true

but if we want to do that, we should change it very soon.

tl;dr:
My proposal is:
* do the bare minimum of expression testing in psql (simple scalar truth,
variable definition, negation)
* do platform independent client-only-expressions in pgbench mode
* allow inline \gset-ish expressions with sql-mode
* and allow for platform/install dependent expressions via PSQL_EXPR_* env
vars.

Re: [HACKERS] bugfix: xpath encoding issue

[Pavel Stehule]

2017-04-13 17:19 GMT+02:00 Alvaro Herrera :

> Pavel Stehule wrote:
> > Hi
> >
> > When I tested XMLTABLE function I found a bug of XPATH function -
> > xpath_internal
> >
> > There xmltype is not correctly encoded to xmlChar due possible invalid
> > encoding info in header. It is possible when XML was loaded with recv
> > function and has not UTF8 encoding.
>
> Hmm ... is it possible to create a test that verifies this?  I suppose
> we could have a non-utf8 value as bytea.
>

I have not any idea. The problem is in utf8 encoded xml with bad header.
There is dependency to database encoding and supported locales.

Maybe I can simulate it with cast without function from bytea to xml - but
it looks more than little bit dirty

Regards

Pavel



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

Re: [HACKERS] tablesync patch broke the assumption that logical rep depends on?

[Fujii Masao]

On Fri, Apr 14, 2017 at 1:28 AM, Peter Eisentraut
 wrote:
> On 4/10/17 13:28, Fujii Masao wrote:
>>  src/backend/replication/logical/launcher.c
>>  * Worker started and attached to our shmem. This check is safe
>>  * because only launcher ever starts the workers, so nobody can steal
>>  * the worker slot.
>>
>> The tablesync patch enabled even worker to start another worker.
>> So the above assumption is not valid for now.
>>
>> This issue seems to cause the corner case where the launcher picks up
>> the same worker slot that previously-started worker has already picked
>> up to start another worker.
>
> I think what the comment should rather say is that workers are always
> started through logicalrep_worker_launch() and worker slots are always
> handed out while holding LogicalRepWorkerLock exclusively, so nobody can
> steal the worker slot.
>
> Does that make sense?

No unless I'm missing something.

logicalrep_worker_launch() picks up unused worker slot (slot's proc == NULL)
while holding LogicalRepWorkerLock. But it releases the lock before the slot
is marked as used (i.e., slot is set to non-NULL). Then newly-launched worker
calls logicalrep_worker_attach() and marks the slot as used.

So if another logicalrep_worker_launch() starts after LogicalRepWorkerLock
is released before the slot is marked as used, it can pick up the same slot
because that slot looks unused.

Regards,

-- 
Fujii Masao


-- 
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] Re: Query fails when SRFs are part of FROM clause (Commit id: 69f4b9c85f)

[Tom Lane]

Robert Haas  writes:
> So I'm not sure what the right thing to do here is.

Andres' points about composite vs noncomposite function result types
seem pretty compelling: we could make the behavior better for scalar
results, but if it then diverges from what happens for composite
results, I don't think that's a step forward.

In the end, no matter how much work we put in, there are going to be some
corner cases that act differently from before.  Considering that none of
the previous corner-case behavior here was particularly carefully thought
through, that's not necessarily disastrous.  We should also consider that
contorting the logic to be bug-compatible with prior behavior may preclude
additional optimization work in future.

I'm a bit inclined to agree with the idea of explicitly requiring SRFs
in FROM to appear only at the top level of the expression.  That was
certainly the only case that the old code was really designed to support,
and I doubt that the documentation claims that it works otherwise.  Also,
to the extent that that ensures we give a clear error rather than possibly
giving results that differ from pre-v10, it's probably going to be less
of a foot-gun for users.

In any case we'd have some documentation work to do here.  Maybe we should
first look at what, if anything, the docs currently say in this area, and
how they'd need to be adjusted if we stick with the currently-implemented
behavior.  As Andres noted, some of the code comments need work too.

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] Re: Query fails when SRFs are part of FROM clause (Commit id: 69f4b9c85f)

[Andres Freund]

On 2017-04-13 12:53:27 -0400, Robert Haas wrote:
> On Wed, Apr 12, 2017 at 6:58 PM, Andres Freund  wrote:
> > This yields plenty weird behaviour in < v10. E.g. the following is
> > disallowed:
> >   SELECT * FROM int4mul(generate_series(1,3), 1);
> >   ERROR:  0A000: set-valued function called in context that cannot accept a 
> > set
> > as is
> >   SELECT * FROM CAST(int4mul(generate_series(1,3), 1) AS bigint);
> >   ERROR:  0A000: set-valued function called in context that cannot accept a 
> > set
> > because the cast is implemented as int8(expr) which avoids the fallback
> > path as it's a FuncExpr, but
> >   SELECT * FROM CAST(int4mul(generate_series(1,3), 1) AS text);
> > works because the cast is implemented as a io coercion, which is not a
> > funcexpr. Therefore it uses the fallback ExecEvalExpr().
> 
> I don't think it's remotely reasonable to try to reproduce this kind
> of behavior exactly.  I think the question is: if we do nothing here,
> will users be pissed?  The answer is not clear to me.  Rushabh's
> original report cast this as a possible bug, not a query he actually
> needed to work for any particular real-world purpose.  On the other
> hand, I don't quite understand why any of these examples should fail.
> If you can select from generate_series() as if it were a table, it
> seems like you ought to be able to also apply one or more functions to
> the result and select from the result.  On the third hand, if this
> only sort of half-worked in v9.6, it's hard to say it's a must-have
> for v10.

I'd say it really didn't work at v9.6, and it wasn't intended to,
there's always[TM] been explicit code to check for that:

/* We don't allow sets in the arguments of the table function */
if (argDone != ExprSingleResult)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
 errmsg("set-valued function called in 
context that cannot accept a set")));

it's just that it ended up not being entirely reliably checked...


I think there's some argument to be made that SRF in arguments ought to
work for reasons of consistency, but the required complications and the
lack of field demand make me skeptical about it being worth it.


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] Cutting initdb's runtime (Perl question embedded)

[Andres Freund]

On 2017-04-13 12:56:14 -0400, Tom Lane wrote:
> Andres Freund  writes:
> > Cool.  I wonder if we also should remove AtEOXact_CatCache()'s
> > cross-checks - the resowner replacement has been in place for a while,
> > and seems robust enough.  They're now the biggest user of time.
> 
> Hm, biggest user of time in what workload?  I've not noticed that
> function particularly.

Just initdb.  I presume it's because the catcaches will frequently be
relatively big there.


> I agree that it doesn't seem like we need to spend a lot of time
> cross-checking there, though.  Maybe keep the code but #ifdef it
> under some nondefault debugging symbol.

Hm, if we want to keep it, maybe tie it to CLOBBER_CACHE_ALWAYS or such,
so it gets compiled at least sometimes? Not a great fit, but ...

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] Optimization for updating foreign tables in Postgres FDW

[Robert Haas]

On Thu, Apr 21, 2016 at 10:53 AM, Robert Haas  wrote:
> On Thu, Apr 21, 2016 at 8:44 AM, Michael Paquier
>  wrote:
>> On Thu, Apr 21, 2016 at 5:22 PM, Etsuro Fujita
>>  wrote:
>>> Attached is an updated version of the patch, which modified Michael's
>>> version of the patch, as I proposed in [1] (see "Other changes:").  I
>>> modified comments for pgfdw_get_result/pgfdw_exec_query also, mainly because
>>> words like "non-blocking mode" there seems confusing (note that we have
>>> PQsetnonbloking).
>>
>> OK, so that is what I sent except that the comments mentioning PG_TRY
>> are moved to their correct places. That's fine for me. Thanks for
>> gathering everything in a single patch and correcting it.
>
> I have committed this patch.  Thanks for working on this.  Sorry for the 
> delay.

This 9.6-era patch, as it turns out, has a problem, which is that we
now respond to an interrupt by sending a cancel request and a
NON-interruptible ABORT TRANSACTION command to the remote side.  If
the reason that the user is trying to interrupt is that the network
connection has been cut, they interrupt the original query only to get
stuck in a non-interruptible wait for ABORT TRANSACTION.  That is
non-optimal.

It is not exactly clear to me how to fix this.  Could we get by with
just slamming the remote connection shut, instead of sending an
explicit ABORT TRANSACTION?  The remote side ought to treat a
disconnect as equivalent to an ABORT anyway, I think, but maybe our
local state would get confused.  (I have not checked.)

Thoughts?

-- 
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] Cutting initdb's runtime (Perl question embedded)

[Tom Lane]

Andres Freund  writes:
> Cool.  I wonder if we also should remove AtEOXact_CatCache()'s
> cross-checks - the resowner replacement has been in place for a while,
> and seems robust enough.  They're now the biggest user of time.

Hm, biggest user of time in what workload?  I've not noticed that
function particularly.

I agree that it doesn't seem like we need to spend a lot of time
cross-checking there, though.  Maybe keep the code but #ifdef it
under some nondefault debugging symbol.

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] Re: Query fails when SRFs are part of FROM clause (Commit id: 69f4b9c85f)

[Robert Haas]

On Wed, Apr 12, 2017 at 6:58 PM, Andres Freund  wrote:
> This yields plenty weird behaviour in < v10. E.g. the following is
> disallowed:
>   SELECT * FROM int4mul(generate_series(1,3), 1);
>   ERROR:  0A000: set-valued function called in context that cannot accept a 
> set
> as is
>   SELECT * FROM CAST(int4mul(generate_series(1,3), 1) AS bigint);
>   ERROR:  0A000: set-valued function called in context that cannot accept a 
> set
> because the cast is implemented as int8(expr) which avoids the fallback
> path as it's a FuncExpr, but
>   SELECT * FROM CAST(int4mul(generate_series(1,3), 1) AS text);
> works because the cast is implemented as a io coercion, which is not a
> funcexpr. Therefore it uses the fallback ExecEvalExpr().

I don't think it's remotely reasonable to try to reproduce this kind
of behavior exactly.  I think the question is: if we do nothing here,
will users be pissed?  The answer is not clear to me.  Rushabh's
original report cast this as a possible bug, not a query he actually
needed to work for any particular real-world purpose.  On the other
hand, I don't quite understand why any of these examples should fail.
If you can select from generate_series() as if it were a table, it
seems like you ought to be able to also apply one or more functions to
the result and select from the result.  On the third hand, if this
only sort of half-worked in v9.6, it's hard to say it's a must-have
for v10.

So I'm not sure what the right thing to do here is.

-- 
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] Cutting initdb's runtime (Perl question embedded)

[Andres Freund]

On 2017-04-13 12:13:30 -0400, Tom Lane wrote:
> Andreas Karlsson  writes:
> > Here is my proof of concept patch. It does basically the same thing as 
> > Andres's patch except that it handles quoted values a bit better and 
> > does not try to support anything other than the regproc type.
> 
> > The patch speeds up initdb without fsync from 0.80 seconds to 0.55 
> > seconds, which is a nice speedup, while adding a negligible amount of 
> > extra work on compilation.
> 
> I've pushed this with some mostly-cosmetic adjustments:

Cool.  I wonder if we also should remove AtEOXact_CatCache()'s
cross-checks - the resowner replacement has been in place for a while,
and seems robust enough.  They're now the biggest user of time.

- 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] Letting the client choose the protocol to use during a SASL exchange

[Heikki Linnakangas]

On 04/13/2017 05:53 AM, Michael Paquier wrote:

Thanks for the updated patches! I had a close look at them.

Let's begin with 0001...

/*
 * Negotiation generated data to be sent to the client.
 */
-   elog(DEBUG4, "sending SASL response token of length %u", outputlen);
+   elog(DEBUG4, "sending SASL challenge of length %u", outputlen);

sendAuthRequest(port, AUTH_REQ_SASL_CONT, output, outputlen);
+
+   pfree(output);
}
This will leak one byte if output is of length 0. I think that the
pfree call should be moved out of this if condition and only called if
output is not NULL. That's a nit, and in the code this scenario cannot
happen, but I would recommend to be correct.


Fixed.


+static int
+pg_SASL_init(PGconn *conn, int payloadLen)
 {
+   charauth_mechanism[21];
So this assumes that any SASL mechanism name will never be longer than
20 characters. Looking at the link of IANA that Alvaro is providing
upthread this is true, could you add a comment that this relies on
such an assumption?


I picked 20 characters for the buffer, because that's what the SASL spec 
says is the maximum, but note that the code doesn't actually rely on 
that. It checks the length of the incoming string, and throws a "SASL 
mechanism not supported" error if it doesn't fit in the buffer. 20 is 
enough to hold "SCRAM-SHA-256", which is the only supported mechanism.


Also note that the second patch replaces this code, anyway..


+   if (initialresponselen != 0)
+   {
+   res = pqPacketSend(conn, 'p', initialresponse, initialresponselen);
+   free(initialresponse);
+
+   if (res != STATUS_OK)
+   return STATUS_ERROR;
+   }
Here also initialresponse could be free'd only if it is not NULL.


Fixed.


And now for 0002...

No much changes in the docs I like the split done for GSS and SSPI messages.

+   /*
+* The StringInfo guarantees that there's a \0 byte after the
+* response.
+*/
+   Assert(input[inputlen] == '\0');
+   pq_getmsgend(&buf);
Shouldn't this also check input == NULL? This could crash the
assertion and pg_be_scram_exchange is able to handle a NULL input
message.


Yep, fixed!


+* Parse the list of SASL authentication mechanisms in the
+* AuthenticationSASL message, and select the best mechanism that we
+* support.  (Only SCRAM-SHA-256 is supported at the moment.)
 */
-   if (strcmp(auth_mechanism, SCRAM_SHA256_NAME) == 0)
+   for (;;)
Just an idea here: being able to enforce the selection with an
environment variable (useful for testing as well in the future).


Hmm. It wouldn't do much, as long as SCRAM-SHA-256 is the only supported 
mechanism. In general, there is no way to tell libpq to e.g. not do 
plain password authentication, which is more pressing than choosing a 
particular SASL mechanism. So I think we should have libpq options to 
control that, but it's a bigger feature than just adding a debug 
environment variable here.


Thanks for the review! I've pushed these patches, after a bunch of 
little cleanups here and there, and fixing a few garden-variety bugs in 
the GSS/SSPI changes.


Álvaro, you're good to go and implement the JDBC support based on this. 
Thanks! Please keep me informed on how it goes, and let me know if you 
run into any trouble, or if there's any discrepancies or ambiguity in 
the docs that we should fix.


- Heikki



--
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_statistic_ext.staenabled might not be the best column name

[Alvaro Herrera]

Tom Lane wrote:
> Peter Eisentraut  writes:
> > On 4/13/17 08:37, Heikki Linnakangas wrote:
> >> We have a bunch of > 3-character prefixes already: amop*, amproc*, 
> >> enum*, cast*. But I think I nevertheless like "ste" better.
> 
> > "stx" perhaps?
> 
> > I would also be in favor of changing it to something other than "sta".
> 
> "stx" sounds pretty good to me --- it seems like e.g. "stxkind" is
> more visibly distinct from "stakind" than "stekind" would be.
> 
> Any objections out there?

stx sounds good to me too.  I'll see about a patch this afternoon.

-- 
Á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] tablesync patch broke the assumption that logical rep depends on?

[Peter Eisentraut]

On 4/10/17 13:28, Fujii Masao wrote:
>  src/backend/replication/logical/launcher.c
>  * Worker started and attached to our shmem. This check is safe
>  * because only launcher ever starts the workers, so nobody can steal
>  * the worker slot.
> 
> The tablesync patch enabled even worker to start another worker.
> So the above assumption is not valid for now.
> 
> This issue seems to cause the corner case where the launcher picks up
> the same worker slot that previously-started worker has already picked
> up to start another worker.

I think what the comment should rather say is that workers are always
started through logicalrep_worker_launch() and worker slots are always
handed out while holding LogicalRepWorkerLock exclusively, so nobody can
steal the worker slot.

Does that make sense?

-- 
Peter Eisentraut  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] Undefined psql variables

[Robert Haas]

On Thu, Apr 13, 2017 at 8:56 AM, Fabien COELHO  wrote:
> Calling the server is already available:
>
>   SELECT  AS varname \gset

Sure, but people are going to want to do it inline with the \if.
Anything that can be done that way can also be done this way, but
people will want it just to make the code look nicer.   I don't think
we should restrict \if to be ONLY an SQL callout, but if people want
that as an option, and I bet they do, then I think we should give it
to them.

> I somewhat disagree: Does building postgres should depend on lua? I think
> adding such a mandatory dependency would not be a good idea. If it is not
> mandatory, then it would mean that psql could be compiled with or without
> lua embedding, thus psql would not be dependable because features may or may
> not be available when writing a "psql script".

That's true, but you could say the same thing about SSL or NLS.  In
practice, any vendor distribution of PostgreSQL will be built with
those options even though, for good reason, they are not hard
dependencies.  I don't see why this should be any different, assuming
the dependency is something that those vendors are for the most part
already packaging anyway.

> Does programming as such in psql is such a good idea?

I think we've pretty much crossed that line already with \if.

> ISTM that cpp-like capabilities (include, if, variables, some expressions)
> are somewhat both useful and enough for the limited use cases I have
> encountered. Similar languages are offered in other instances, such as
> readline inputrc or vim vimrc.

Yeah, but nobody would claim that cpp is a fun thing to program with,
and it's pretty clear from discussions here that some people are
squeezing last possible bit of juice out of every existing facility
and still wanting more.  I don't think it's likely that adding one or
two additional simple constructs is going to be sufficient to keep
people from wanting more.  I mean, I would have switched to Perl and
DBD::Pg rather than write some of the crazy psql scripts that have
been posted here, but we're not building PostgreSQL to meet my needs
particularly.

> I suggest to reuse pgbench expression engine, developed by Haas Robert:-)

Not a bad idea (though I'm sure there are other reasonable options, too).

> Generating a error message with ${foo:?} is nice, but what psql need is just
> a way to test whether a variable is defined or not.

I'm not saying we should slavishly follow bash, but don't confuse what
you need with what other people need.  bash (well, sh, really) grew
that syntax for a reason, and it may be more than "there was this one
guy back in the seventies who wanted it, and ...".

-- 
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] pg_statistic_ext.staenabled might not be the best column name

[Tom Lane]

Peter Eisentraut  writes:
> On 4/13/17 08:37, Heikki Linnakangas wrote:
>> We have a bunch of > 3-character prefixes already: amop*, amproc*, 
>> enum*, cast*. But I think I nevertheless like "ste" better.

> "stx" perhaps?

> I would also be in favor of changing it to something other than "sta".

"stx" sounds pretty good to me --- it seems like e.g. "stxkind" is
more visibly distinct from "stakind" than "stekind" would be.

Any objections out there?

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] Cutting initdb's runtime (Perl question embedded)

[Tom Lane]

Andreas Karlsson  writes:
> Here is my proof of concept patch. It does basically the same thing as 
> Andres's patch except that it handles quoted values a bit better and 
> does not try to support anything other than the regproc type.

> The patch speeds up initdb without fsync from 0.80 seconds to 0.55 
> seconds, which is a nice speedup, while adding a negligible amount of 
> extra work on compilation.

I've pushed this with some mostly-cosmetic adjustments:

* created a single subroutine that understands how to split DATA lines,
rather than having several copies of the regex

* rearranged the code so that the data structure returned by
Catalog::Catalogs() isn't scribbled on (which was already
happening before your patch, but it seemed pretty ugly to me)

* stripped out the bootstrap-time name lookup code from all of reg*
not just regproc.

There's certainly lots more that could be done in the genbki code,
but I think all we can justify at this stage of the development
cycle is to get the low-hanging fruit for testing speedups.

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] SUBSCRIPTIONS and pg_upgrade

[Robert Haas]

On Thu, Apr 13, 2017 at 12:05 PM, Peter Eisentraut
 wrote:
> On 4/12/17 18:31, Peter Eisentraut wrote:
>> On 4/11/17 23:41, Noah Misch wrote:
>>> On Tue, Apr 11, 2017 at 11:21:24PM -0400, Peter Eisentraut wrote:
 On 4/9/17 22:16, Noah Misch wrote:
> [Action required within three days.  This is a generic notification.]

 Patches have been posted.  Discussion is still going on a bit.
>>>
>>> By what day should the community look for your next update?
>>
>> tomorrow
>
> Everything has been committed, and this thread can be closed.

I wonder if we should have an --no-subscriptions option, now that they
are dumped by default, just like we have --no-blobs, --no-owner,
--no-password, --no-privileges, --no-acl, --no-tablespaces, and
--no-security-labels.  It seems like there is probably a fairly large
use case for excluding subscriptions even if you have sufficient
permissions to dump them.

-- 
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] pg_upgrade vs extension upgrades

[Robert Haas]

On Thu, Apr 13, 2017 at 11:48 AM, Peter Eisentraut
 wrote:
> On 4/12/17 22:56, Bruce Momjian wrote:
>> Is pg_upgrade the right place for an extension upgrade script?  When
>> pg_upgrade started creating an incremental-analyze script, people
>> thought it should be more generic so it was moved to vacuumdb
>> --analyze-in-stages.  Seems we should do the same thing for the
>> extension upgrade script.
>
> Yeah, many of the things that pg_upgrade would do or suggest after an
> upgrade could also apply to other upgrade methods, such as by logical
> replication.  So offering them separately would be good.

And in theory, extension upgrades could happen any time, not just when
there's a major version upgrade.  You could be using a
separately-bundled extension, or we could bump an extension version in
a minor release to fix some issue.

I think we should invent a clever name for a new utility, like
pg_maintain or something.  It could let you know about (and optionally
perform) a variety of optional maintenance tasks:

- upgrading extensions
- reindexing indexes for which the version has been bumped, like hash
indexes in v10
- reindexing or dropping indexes marked invalid that were left behind
by a CIC failure
- analyzing tables that lack (full?) statistics
- triggering heap scans to reclaim HEAP_MOVED_* bits, if we have that
kind of thing someday

-- 
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] SUBSCRIPTIONS and pg_upgrade

[Peter Eisentraut]

On 4/12/17 18:31, Peter Eisentraut wrote:
> On 4/11/17 23:41, Noah Misch wrote:
>> On Tue, Apr 11, 2017 at 11:21:24PM -0400, Peter Eisentraut wrote:
>>> On 4/9/17 22:16, Noah Misch wrote:
 [Action required within three days.  This is a generic notification.]
>>>
>>> Patches have been posted.  Discussion is still going on a bit.
>>
>> By what day should the community look for your next update?
> 
> tomorrow

Everything has been committed, and this thread can be closed.

-- 
Peter Eisentraut  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] pg_statistic_ext.staenabled might not be the best column name

[Peter Eisentraut]

On 4/13/17 08:37, Heikki Linnakangas wrote:
>> We could go with "ste" perhaps, or break the convention of 3-character
>> prefixes and go with "stae".
> We have a bunch of > 3-character prefixes already: amop*, amproc*, 
> enum*, cast*. But I think I nevertheless like "ste" better.

"stx" perhaps?

I would also be in favor of changing it to something other than "sta".

-- 
Peter Eisentraut  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] pg_upgrade vs extension upgrades

[Peter Eisentraut]

On 4/12/17 22:56, Bruce Momjian wrote:
> Is pg_upgrade the right place for an extension upgrade script?  When
> pg_upgrade started creating an incremental-analyze script, people
> thought it should be more generic so it was moved to vacuumdb
> --analyze-in-stages.  Seems we should do the same thing for the
> extension upgrade script.

Yeah, many of the things that pg_upgrade would do or suggest after an
upgrade could also apply to other upgrade methods, such as by logical
replication.  So offering them separately would be good.

-- 
Peter Eisentraut  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] pg_dump emits ALTER TABLE ONLY partitioned_table

[Robert Haas]

On Thu, Apr 13, 2017 at 11:05 AM, Stephen Frost  wrote:
> Sure, though I won't be able to today and I've got some doubts about the
> other patches.  I'll have more time tomorrow though.

OK, cool.  I'll mark you down as the owner on the open items list.

-- 
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] bugfix: xpath encoding issue

[Alvaro Herrera]

Pavel Stehule wrote:
> Hi
> 
> When I tested XMLTABLE function I found a bug of XPATH function -
> xpath_internal
> 
> There xmltype is not correctly encoded to xmlChar due possible invalid
> encoding info in header. It is possible when XML was loaded with recv
> function and has not UTF8 encoding.

Hmm ... is it possible to create a test that verifies this?  I suppose
we could have a non-utf8 value as bytea.

-- 
Á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] Small doc fix for xmltable

[Alvaro Herrera]

Arjen Nienhuis wrote:
> Hi,
> 
> In the devel docs for xmltable there should be a comma after XMLNAMESPACES()

Thanks, pushed.

-- 
Á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] example for xmltable with XMLNAMESPACES

[Alvaro Herrera]

Arjen Nienhuis wrote:
> It wasn't completely clear for me how to use namespaces in xmltable().
> Maybe add this to the documentation. It shows the default namespace
> and quoting the namespace name.

Thanks, pushed.

-- 
Á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] xmltable doc fix and example for XMLNAMESPACES

[Alvaro Herrera]

Pavel Stehule wrote:
> Hi
> 
> I am sending a patch with changes in XMLTABLE documentation proposed by
> Arjen.

Thanks, pushed with some rewording.

-- 
Á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_dump emits ALTER TABLE ONLY partitioned_table

[Stephen Frost]

Robert,

* Robert Haas (robertmh...@gmail.com) wrote:
> So I think I was indeed confused before, and I think you're basically
> right here, but on one point I think you are not right -- ALTER TABLE
> ONLY .. CHECK () doesn't work on a table with inheritance children
> regardless of whether the children already have the matching
> constraint:

To be clear- I wasn't thinking about what's possible today with
inheiritance or partitioning or in PG at all, but rather focusing on
what a user is likely to expect and understand from the messaging.

> rhaas=# create table foo (a int, b text);
> CREATE TABLE
> rhaas=# create table bar () inherits (foo);
> CREATE TABLE
> rhaas=# alter table only foo add check (a = 1);
> ERROR:  constraint must be added to child tables too
> rhaas=# alter table only bar add check (a = 1);
> ALTER TABLE
> rhaas=# alter table only foo add check (a = 1);
> ERROR:  constraint must be added to child tables too

If that's the case then we should really change that error message, in
my view.  I'd be happier if we did support adding the constraint after
child tables exist, but if we don't, we shouldn't imply to the user that
they can add it after adding it to the children.

> So, regarding Amit's 0001:
> 
> - I think we should update the relevant hunk of the documentation
> rather than just removing it.

Alright.

> - Should we similarly allow TRUNCATE ONLY foo and ALTER TABLE ONLY foo
> .. to work on a partitioned table without partitions, or is that just
> pointless tinkering?  That seems to be the only case where, after this
> patch, an ONLY operation will fail on a childless partitioned table.

I'm less sure about TRUNCATE ONLY because that really isn't meaningful
at all, is it?  A partitioned table doesn't have any data to truncate
and truncating it doesn't have any impact on what happens later, does
it?  Having a sensible error be reported if someone tries to do that
would be good though.

> Do you want to be responsible for committing these or should I do it?

Sure, though I won't be able to today and I've got some doubts about the
other patches.  I'll have more time tomorrow though.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] pg_stats_ext view does not seem all that useful

[Alvaro Herrera]

Tomas Vondra wrote:
> On 04/10/2017 12:12 PM, David Rowley wrote:
> > During my review and time spent working on the functional dependencies
> > part of extended statistics I wondered what was the use for the
> > pg_stats_ext view. I was unsure why the length of the serialised
> > dependencies was useful.
> > 
> > Perhaps we could improve the view, but I'm not all that sure what
> > value it would add. Maybe we need to discuss this, but in the
> > meantime, I've attached a patch which just removes the view completely
> 
> Yeah, let's get rid of the view. It was quite useful before introducing the
> custom data types (and implicit casts to text), because pg_statistic_ext
> would simply return the whole bytea value. But since then the view kinda
> lost the purpose and no one really noticed.

Thanks, pushed.

-- 
Á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] Shouldn't duplicate addition to publication be a no-op?

[Amit Langote]

On Thu, Apr 13, 2017 at 9:33 PM, Tom Lane  wrote:
> Amit Langote  writes:
>> I wonder if trying to add a relation to a publication that it is already a
>> part should be considered a no-op, instead of causing an error (which
>> happens in the ALTER PUBLICATION ADD TABLES case).
>
> On what grounds?
>
> The equivalent case for inheritance is an error:
>
> regression=# create table foo (a int);
> CREATE TABLE
> regression=# create table bar () inherits (foo);
> CREATE TABLE
> regression=# alter table bar inherit foo;
> ERROR:  relation "foo" would be inherited from more than once

Hmm, yes.  Making it a no-op might be surprising to some.

> (Your example purporting to show the contrary contains a typo.)

Oops, I had meant: alter publication mypub add table foo;

> If there's a reason why this case should act differently from that
> precedent, you haven't shown it.

Maybe we won't solve it by doing what I proposed, but if there is a
database like this:

create table foo (a int);
create table bar () inherits(foo);
create publication mypub for table foo;

Dumping and restoring it into another database is not without errors,
because of the order in which things are dumped:

$ createdb test
$ pg_dump -s | psql -e test


CREATE PUBLICATION mypub WITH (PUBLISH INSERT, PUBLISH UPDATE, PUBLISH DELETE);
ALTER PUBLICATION mypub ADD TABLE bar;
ALTER PUBLICATION mypub ADD TABLE foo;
ERROR:  relation "bar" is already member of publication "mypub"

But perhaps that's a pg_dump issue, not this.  I haven't closely
looked.  Or perhaps something that will be resolved in the nearby
"Logical replication and inheritance" thread.

Thanks,
Amit


-- 
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] Undefined psql variables

[Fabien COELHO]

Hello Robert,

My 0.02€ about your interesting questions and points.


On Sun, Apr 2, 2017 at 3:56 PM, Tom Lane  wrote:

So my view of this is that "send the expression to the server" ought
to be just one option for \if, not the only way to do it.


I heartily agree.  There should be some kind of client-side expression
language, and one thing it should allow is calling out to the server.


Calling the server is already available:

  SELECT  AS varname \gset

What is missing is some client-side expressions.

As \if is a client-side thing, I now think that it should just rely on 
client-side evaluation. Note that it is possible to do better, but 
solutions are either ugly (strange prefixes) or too clever and possibly 
not extensible (regex filtering), and from a user experience point of view 
I finally thing that ugly or clever should be avoided.



Then people who only want to call out to the server can do that, but
people who want to do something else have the option.  Insisting that
this facility isn't allowed to do anything other than consult the
server is (1) inconsistent with what we've already got in v10 and (2)
boxing ourselves into a corner for no very good reason.

Now, the optimal shape for that client-side expression language is not
very clear to me.  Do we want to invent our own language, or maybe
consider using something that already exists?


It's been previously suggested that we should somehow embed Lua, and 
this might not be a bad place to consider doing something like that.


I somewhat disagree: Does building postgres should depend on lua? I think 
adding such a mandatory dependency would not be a good idea. If it is not 
mandatory, then it would mean that psql could be compiled with or without 
lua embedding, thus psql would not be dependable because features may or 
may not be available when writing a "psql script".


For me, client embedded language pg-{lua,pl,tcl,i?py,bf...} (chose your 
favorite:-) projects could make sense, but it does not have to be done 
within the existing psql client, especially with trying to keep upward 
compatibility... If started, such a thing should be a distinct project, 
possibly hosted within postgres source tree if it works well at some 
point.


That might be a way to add a lot of power without having to invent an 
entirely new programming language one bit at a time.


Does programming as such in psql is such a good idea?

ISTM that cpp-like capabilities (include, if, variables, some expressions) 
are somewhat both useful and enough for the limited use cases I have 
encountered. Similar languages are offered in other instances, such as 
readline inputrc or vim vimrc.


If I have something really complicated, then I really want a programming 
language, probably I do not want to learn a new one just for this purpose, 
so I switch to something else that I already know which will do some SQL 
when necessary.


If we want to invent our own expression language, what kind of syntax 
should it use?


After about 35 years of programming, I've convinced myself that mixing 
languages is most often a bad idea (think HTML/CSS/JS/PHP/SQL all in one 
file). Currently psql has SQL, backslash commands, :* client-side 
variables, all with good justifications. That is somewhat 3 languages (or 
2.5 if counting variable substitions for half a language), and I think it 
should not go up if avoidable.


This leads to the opinion that if there is a client side language (or 
client-side expressions as we are considering here), then it should look 
like SQL, hence my constant ranting about the "defined varname" somehow 
perlish thing. Tom helped forge this opinion when argumenting about some 
pgbench changes I submitted, and he is the one suggesting this.



Upon what kind of design principles should it be based?


I submit that client side expressions should be a subset of SQL and 
possible existing or extended variable substitution.


There's likely to be vigorous debate on these topics, and probably also 
complaints that the good designs are too much work and the 
easy-to-implement designs are too limiting. (Regular readers of this 
mailing list will likely be able to guess which side of those debates 
I'll be on, but we need to have them all the same.)


I suggest to reuse pgbench expression engine, developed by Haas Robert:-)

I have submitted a patch to add some functions and boolean support, which 
seems like a definite requirement for "\if". Although pgbench expressions 
are a bit overkill for psql, I think that developing another expression 
engine is a bad idea, just reuse the one.



Regarding the ostensible topic of this thread, one thought I had while
reading through these various responses is that the original need
would be well-served by the (somewhat dubious) syntax that bash uses
for variable substitution.


Obviously, we aren't going to change the interpolate-this-variable 
character from : to $, but bash has ${parameter:-word} to substitute a 
default for 

Re: [HACKERS] pg_statistic_ext.staenabled might not be the best column name

[Heikki Linnakangas]

On 04/13/2017 03:28 PM, Tom Lane wrote:

Tomas Vondra  writes:

On 04/12/2017 03:36 PM, David Rowley wrote:

"stakind" seems like a better name. I'd have personally gone with
"statype" but pg_statistic already thinks stakind is better.



+1 to stakind


I agree with that, but as long as we're rethinking column names here,
was it a good idea to use the same "sta" prefix in pg_statistic_ext
as in pg_statistic?  I do not think there's anyplace else where we're
using the same table-identifying prefix in two different catalogs,
and it seems a little pointless to follow that convention at all if
we're not going to make it a unique prefix.

We could go with "ste" perhaps, or break the convention of 3-character
prefixes and go with "stae".


We have a bunch of > 3-character prefixes already: amop*, amproc*, 
enum*, cast*. But I think I nevertheless like "ste" better.


That said, we also have two existing tables with the same prefix: 
pg_constraint and pg_conversion. Both use "con" as the prefix. Yes, it 
is a bit confusing, let's not to make the same mistake again.


- Heikki



--
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] Shouldn't duplicate addition to publication be a no-op?

[Tom Lane]

Amit Langote  writes:
> I wonder if trying to add a relation to a publication that it is already a
> part should be considered a no-op, instead of causing an error (which
> happens in the ALTER PUBLICATION ADD TABLES case).

On what grounds?

The equivalent case for inheritance is an error:

regression=# create table foo (a int);
CREATE TABLE
regression=# create table bar () inherits (foo);
CREATE TABLE
regression=# alter table bar inherit foo;
ERROR:  relation "foo" would be inherited from more than once

(Your example purporting to show the contrary contains a typo.)

If there's a reason why this case should act differently from that
precedent, you haven't shown it.

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] Document the order of changing certain settings when using hot-standby servers

[Yorick Peterse]

Aleksander,

> What you actually meant probably was "do so on ALL standby servers
> first", right?

Good point, right now it can give you the idea that applying it to just
1 standby (instead of all of them) is good enough, when instead you
need to apply it to all of them.

Attached is an adjusted version of my changes to better reflect this.

Yorick
diff --git a/doc/src/sgml/high-availability.sgml b/doc/src/sgml/high-availability.sgml
index 51359d6236..8f382d0b93 100644
*** a/doc/src/sgml/high-availability.sgml
--- b/doc/src/sgml/high-availability.sgml
***
*** 2098,2104  LOG:  database system is ready to accept read only connections
  be equal to or greater than the value on the primary. If these parameters
  are not set high enough then the standby will refuse to start.
  Higher values can then be supplied and the server
! restarted to begin recovery again.  These parameters are:
  

 
--- 2098,2108 
  be equal to or greater than the value on the primary. If these parameters
  are not set high enough then the standby will refuse to start.
  Higher values can then be supplied and the server
! restarted to begin recovery again. If you want to increase these values you
! should do so on all standby servers first, before applying the changes to
! the primary. If you instead want to decrease these values you should do so
! on the primary first, before applying the changes to all standby servers.
! These parameters are:
  

 

-- 
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_statistic_ext.staenabled might not be the best column name

[Tom Lane]

Tomas Vondra  writes:
> On 04/12/2017 03:36 PM, David Rowley wrote:
>> "stakind" seems like a better name. I'd have personally gone with
>> "statype" but pg_statistic already thinks stakind is better.

> +1 to stakind

I agree with that, but as long as we're rethinking column names here,
was it a good idea to use the same "sta" prefix in pg_statistic_ext
as in pg_statistic?  I do not think there's anyplace else where we're
using the same table-identifying prefix in two different catalogs,
and it seems a little pointless to follow that convention at all if
we're not going to make it a unique prefix.

We could go with "ste" perhaps, or break the convention of 3-character
prefixes and go with "stae".

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] Quorum commit for multiple synchronous replication.

[Masahiko Sawada]

On Thu, Apr 13, 2017 at 5:17 PM, Kyotaro HORIGUCHI
 wrote:
> Hello,
>
> At Thu, 6 Apr 2017 16:17:31 +0900, Masahiko Sawada  
> wrote in 
>> On Thu, Apr 6, 2017 at 10:51 AM, Noah Misch  wrote:
>> > On Thu, Apr 06, 2017 at 12:48:56AM +0900, Fujii Masao wrote:
>> >> On Wed, Apr 5, 2017 at 3:45 PM, Noah Misch  wrote:
>> >> > On Mon, Dec 19, 2016 at 09:49:58PM +0900, Fujii Masao wrote:
>> >> >> Regarding this feature, there are some loose ends. We should work on
>> >> >> and complete them until the release.
>> >> >>
>> >> >> (1)
>> >> >> Which synchronous replication method, priority or quorum, should be
>> >> >> chosen when neither FIRST nor ANY is specified in s_s_names? Right now,
>> >> >> a priority-based sync replication is chosen for keeping backward
>> >> >> compatibility. However some hackers argued to change this decision
>> >> >> so that a quorum commit is chosen because they think that most users
>> >> >> prefer to a quorum.
>> >> >>
>> >> >> (2)
>> >> >> There will be still many source comments and documentations that
>> >> >> we need to update, for example, in high-availability.sgml. We need to
>> >> >> check and update them throughly.
>> >> >>
>> >> >> (3)
>> >> >> The priority value is assigned to each standby listed in s_s_names
>> >> >> even in quorum commit though those priority values are not used at all.
>> >> >> Users can see those priority values in pg_stat_replication.
>> >> >> Isn't this confusing? If yes, it might be better to always assign 1 as
>> >> >> the priority, for example.
>> >> >
>> >> > [Action required within three days.  This is a generic notification.]
>> >> >
>> >> > The above-described topic is currently a PostgreSQL 10 open item.  
>> >> > Fujii,
>> >> > since you committed the patch believed to have created it, you own this 
>> >> > open
>> >> > item.  If some other commit is more relevant or if this does not belong 
>> >> > as a
>> >> > v10 open item, please let us know.  Otherwise, please observe the 
>> >> > policy on
>> >> > open item ownership[1] and send a status update within three calendar 
>> >> > days of
>> >> > this message.  Include a date for your subsequent status update.  
>> >> > Testers may
>> >> > discover new open items at any time, and I want to plan to get them all 
>> >> > fixed
>> >> > well in advance of shipping v10.  Consequently, I will appreciate your 
>> >> > efforts
>> >> > toward speedy resolution.  Thanks.
>> >> >
>> >> > [1] 
>> >> > https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com
>> >>
>> >> Thanks for the notice!
>> >>
>> >> Regarding the item (2), Sawada-san told me that he will work on it after
>> >> this CommitFest finishes. So we would receive the patch for the item from
>> >> him next week. If there will be no patch even after the end of next week
>> >> (i.e., April 14th), I will. Let's wait for Sawada-san's action at first.
>> >
>> > Sounds reasonable; I will look for your update on 14Apr or earlier.
>> >
>> >> The items (1) and (3) are not bugs. So I don't think that they need to be
>> >> resolved before the beta release. After the feature freeze, many users
>> >> will try and play with many new features including quorum-based syncrep.
>> >> Then if many of them complain about (1) and (3), we can change the code
>> >> at that timing. So we need more time that users can try the feature.
>> >
>> > I've moved (1) to a new section for things to revisit during beta.  If 
>> > someone
>> > feels strongly that the current behavior is Wrong and must change, speak 
>> > up as
>> > soon as you reach that conclusion.  Absent such arguments, the behavior 
>> > won't
>> > change.
>> >
>> >> BTW, IMO (3) should be fixed so that pg_stat_replication reports NULL
>> >> as the priority if quorum-based sync rep is chosen. It's less confusing.
>> >
>> > Since you do want (3) to change, please own it like any other open item,
>> > including the mandatory status updates.
>>
>> I agree to report NULL as the priority. I'll send a patch for this as well.
>
>
> In the comment,

Thank you for reviewing!

>
> +  /*
> +   * The priority appers NULL as it is not used in quorum-based
> +   * sync replication.
> +   */
>
> appers should be appears. But the comment would be better to be
> something follows.

Will fix.

>
> "The priority value is useless for quorum-based sync replication" or
>
> "The priority field is NULL for quorum-based sync replication
>  since the value is useless."
>
> Or, or, or.. something other.

Will fix with later part.

>
>
> This part,
>
> +if (SyncRepConfig &&
> +SyncRepConfig->syncrep_method == SYNC_REP_QUORUM)
> +nulls[9] = true;
> +else
> +values[9] = Int32GetDatum(priority);
>
> I looked on how syncrep_method is used in the code and found that
> it is always used as "== SYNC_REP_PRIORITY" or else. It doesn't
> matter since currently there's only two alternatives for the
> variable, but can be problematic when the third alternative comes
> in.

Re: [HACKERS] Letting the client choose the protocol to use during a SASL exchange

[Álvaro Hernández Tortosa]

My only desire would be to have a final spec and implement the full parser
now, not have to change it in the future. We already know today all the
requirements, so please pick one and I will follow it :)



On Apr 13, 2017 13:47, "Heikki Linnakangas"  wrote:

> On 04/13/2017 02:35 PM, Álvaro Hernández Tortosa wrote:
>
>> On 13/04/17 13:24, Heikki Linnakangas wrote:
>>
>>> Right, when we get channel binding, the server will list
>>> "SCRAM-SHA-256" and "SCRAM-SHA-256-PLUS" as the list of mechanisms.
>>> And if we get channel binding using something else than tls-unique,
>>> then those will be added as extra mechanisms, too, e.g.
>>> "SCRAM-SHA-256-PLUS tls-awesome".
>>>
>>
>>  And how about supporting different SCRAM mechanisms with different
>> possible channel bindings? Separate by space too? So given a field, is
>> the first item the SCRAM mechanism, and all the remaning the channel
>> binding methods? I.e.:
>>
>> SCRAM-SHA-256-PLUS tls-awesome tls-awesome2 tls-awesome3\0...
>>
>
> I think we're going in circles.. Yeah, we could do that. Or they could be
> listed as separate mechanisms:
>
> SCRAM-SHA-256-PLUS\0
> SCRAM-SHA-256-PLUS tls-awesome\0
> SCRAM-SHA-256-PLUS tls-awesome2\0
> SCRAM-SHA-256-PLUS tls-awesome3\0
> \0
>
> One alternative is that you could list extra channel bindings that are
> supported by all the mechanisms, as separate entries. So the list could be:
>
> binding tls-unique\0
> binding tls-awesome\0
> binding tls-awesome2\0
> binding tls-awesome3\0
> SCRAM-SHA-256-PLUS\0
> SCRAM-SHA-512-PLUS\0
> \0
>
> which would mean that those bindings are supported by all the mechanisms
> that follow. I think this would achieve the same thing as your proposed
> separate field, with the current proposed protocol.
>
> But again, I'm 99% sure we won't need it, and we don't need to decide the
> exact syntax for channel bindings yet. We have the flexibility now, so we
> can cross the bridge when we get there.
>
> - Heikki
>
>

Re: [HACKERS] Letting the client choose the protocol to use during a SASL exchange

[Heikki Linnakangas]

On 04/13/2017 02:35 PM, Álvaro Hernández Tortosa wrote:

On 13/04/17 13:24, Heikki Linnakangas wrote:

Right, when we get channel binding, the server will list
"SCRAM-SHA-256" and "SCRAM-SHA-256-PLUS" as the list of mechanisms.
And if we get channel binding using something else than tls-unique,
then those will be added as extra mechanisms, too, e.g.
"SCRAM-SHA-256-PLUS tls-awesome".


 And how about supporting different SCRAM mechanisms with different
possible channel bindings? Separate by space too? So given a field, is
the first item the SCRAM mechanism, and all the remaning the channel
binding methods? I.e.:

SCRAM-SHA-256-PLUS tls-awesome tls-awesome2 tls-awesome3\0...


I think we're going in circles.. Yeah, we could do that. Or they could 
be listed as separate mechanisms:


SCRAM-SHA-256-PLUS\0
SCRAM-SHA-256-PLUS tls-awesome\0
SCRAM-SHA-256-PLUS tls-awesome2\0
SCRAM-SHA-256-PLUS tls-awesome3\0
\0

One alternative is that you could list extra channel bindings that are 
supported by all the mechanisms, as separate entries. So the list could be:


binding tls-unique\0
binding tls-awesome\0
binding tls-awesome2\0
binding tls-awesome3\0
SCRAM-SHA-256-PLUS\0
SCRAM-SHA-512-PLUS\0
\0

which would mean that those bindings are supported by all the mechanisms 
that follow. I think this would achieve the same thing as your proposed 
separate field, with the current proposed protocol.


But again, I'm 99% sure we won't need it, and we don't need to decide 
the exact syntax for channel bindings yet. We have the flexibility now, 
so we can cross the bridge when we get there.


- Heikki



--
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] Letting the client choose the protocol to use during a SASL exchange

[Álvaro Hernández Tortosa]

On 13/04/17 04:54, Michael Paquier wrote:

On Thu, Apr 13, 2017 at 6:37 AM, Álvaro Hernández Tortosa
 wrote:

 By looking at the them, and unless I'm missing something, I don't see
how the extra information for the future implementation of channel binding
would be added (without changing the protocol). Relevant part is:

The message body is a list of SASL authentication mechanisms, in the
server's order of preference. A zero byte is required as terminator after
the last authentication mechanism name. For each mechanism, there is the
following:



 String



 Name of a SASL authentication mechanism.




 How do you plan to implement it, in future versions, without modifying
the AuthenticationSASL message? Or is it OK to add new fields to a message
in future PostgreSQL versions, without considering that a protocol change?

I don't quite understand the complain here, it is perfectly fine to
add as many null-terminated names as you want with this model. The
patches would make the server just send one mechanism name now, but
nothing prevents the addition of more.


I think I explained in my previous reply, but just in case: there 
are two lists here: SCRAM mechanism and channel binding mechanisms. They 
are orthogonal, you could pick them separately (only with the -PLUS 
variants, of course). All two (both SCRAM and channel binding 
mechanisms) have to be advertised by the server.



Álvaro

--

Álvaro Hernández Tortosa


---
<8K>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] Letting the client choose the protocol to use during a SASL exchange

[Álvaro Hernández Tortosa]

On 13/04/17 13:24, Heikki Linnakangas wrote:

On 04/13/2017 05:54 AM, Michael Paquier wrote:

On Thu, Apr 13, 2017 at 6:37 AM, Álvaro Hernández Tortosa
 wrote:
By looking at the them, and unless I'm missing something, I 
don't see
how the extra information for the future implementation of channel 
binding

would be added (without changing the protocol). Relevant part is:

The message body is a list of SASL authentication mechanisms, in the
server's order of preference. A zero byte is required as terminator 
after
the last authentication mechanism name. For each mechanism, there is 
the

following:



String



Name of a SASL authentication mechanism.




How do you plan to implement it, in future versions, without 
modifying
the AuthenticationSASL message? Or is it OK to add new fields to a 
message
in future PostgreSQL versions, without considering that a protocol 
change?


I don't quite understand the complain here, it is perfectly fine to
add as many null-terminated names as you want with this model. The
patches would make the server just send one mechanism name now, but
nothing prevents the addition of more.


Right, when we get channel binding, the server will list 
"SCRAM-SHA-256" and "SCRAM-SHA-256-PLUS" as the list of mechanisms. 
And if we get channel binding using something else than tls-unique, 
then those will be added as extra mechanisms, too, e.g. 
"SCRAM-SHA-256-PLUS tls-awesome".


And how about supporting different SCRAM mechanisms with different 
possible channel bindings? Separate by space too? So given a field, is 
the first item the SCRAM mechanism, and all the remaning the channel 
binding methods? I.e.:


SCRAM-SHA-256-PLUS tls-awesome tls-awesome2 tls-awesome3\0...

Please note that if this is the solution chosen:

- A lot of parsing and convention is required (first arg is the SCRAM 
mechanism, succesive are channel binding; tls-unique is always 
"implied", etc)
- Channel binding names will be repeated for every SCRAM mechanism with 
"-PLUS". This is not needed, SCRAM mechanisms and channel binding are 
separate things.
- Channel binding names will not be present on others, making the parser 
even more complex.


All this vs, again, stating SCRAM mechanisms on one list and 
channel binding on another list, which is to my much more KISS. But... 
anyway, if this is the decision made, at least I think this should be 
documented now, because client parsers need to be designed one way or 
another.



Thanks,

Álvaro


--

Álvaro Hernández Tortosa


---
<8K>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] Letting the client choose the protocol to use during a SASL exchange

[Heikki Linnakangas]

On 04/13/2017 05:54 AM, Michael Paquier wrote:

On Thu, Apr 13, 2017 at 6:37 AM, Álvaro Hernández Tortosa
 wrote:

By looking at the them, and unless I'm missing something, I don't see
how the extra information for the future implementation of channel binding
would be added (without changing the protocol). Relevant part is:

The message body is a list of SASL authentication mechanisms, in the
server's order of preference. A zero byte is required as terminator after
the last authentication mechanism name. For each mechanism, there is the
following:



String



Name of a SASL authentication mechanism.




How do you plan to implement it, in future versions, without modifying
the AuthenticationSASL message? Or is it OK to add new fields to a message
in future PostgreSQL versions, without considering that a protocol change?


I don't quite understand the complain here, it is perfectly fine to
add as many null-terminated names as you want with this model. The
patches would make the server just send one mechanism name now, but
nothing prevents the addition of more.


Right, when we get channel binding, the server will list "SCRAM-SHA-256" 
and "SCRAM-SHA-256-PLUS" as the list of mechanisms. And if we get 
channel binding using something else than tls-unique, then those will be 
added as extra mechanisms, too, e.g. "SCRAM-SHA-256-PLUS tls-awesome".


I don't think that needs to be discussed in the docs yet, because a 
client will simply ignore any mechanisms it doesn't understand. Although 
perhaps it would be good to mention explicitly that even though SASL 
defines that mechanism names have a particular format - all ASCII 
upper-case, max 20 chars - the client should accept and ignore arbitrary 
strings, not limited to that format.


- Heikki



--
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] Interval for launching the table sync worker

[Kyotaro HORIGUCHI]

Ouch! I replied to wrong mail.

At Thu, 13 Apr 2017 19:55:04 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI 
 wrote in 
<20170413.195504.89348773.horiguchi.kyot...@lab.ntt.co.jp>
> I confused sync and apply workers.
> sync worker failure at start causes immediate retries.
> 
> At Thu, 13 Apr 2017 11:53:27 +0900, Masahiko Sawada  
> wrote in 
> > On Wed, Apr 12, 2017 at 11:46 PM, Peter Eisentraut
> >  wrote:
> > > On 4/12/17 00:48, Masahiko Sawada wrote:
> > >> On Wed, Apr 12, 2017 at 1:28 PM, Peter Eisentraut
> > >>> Perhaps instead of a global last_start_time, we store a per relation
> > >>> last_start_time in SubscriptionRelState?
> > >>
> > >> I was thinking the same. But a problem is that the list of
> > >> SubscriptionRelState is refreshed whenever the syncing table state
> > >> becomes invalid (table_state_valid = false). I guess we need to
> > >> improve these logic including GetSubscriptionNotReadyRelations().
> > >
> > > The table states are invalidated on a syscache callback from
> > > pg_subscription_rel, which happens roughly speaking when a table
> > > finishes the initial sync.  So if we're worried about failing tablesync
> > > workers relaunching to quickly, this would only be a problem if a
> > > tablesync of another table finishes right in that restart window.  That
> > > doesn't seem a terrible issue to me.
> > >
> > 
> > I think the table states are invalidated whenever the table sync
> > worker starts, because the table sync worker updates its status of
> > pg_subscription_rel and commits it before starting actual copy. So we
> > cannot rely on that. I thought we can store last_start_time into
> > pg_subscription_rel but it might be overkill. I'm now thinking to
> > change GetSubscriptionNotReadyRealtions so that last_start_time in
> > SubscriptionRelState is taken over to new list.

The right target of "This" below is found at the following URL.

https://www.postgresql.org/message-id/CAD21AoBt_XUdppddFak661_LBM2t3CfK52aLKHG%2Bekd7SkzLmg%40mail.gmail.com

> This resolves the problem but, if I understand correctly, the
> many pallocs in process_syncing_tables_for_apply() is working on
> ApplyContext and the context is reset before the next visit here
> (in LogicalRepApplyLoop).
> 
> Although this is not a problem of this patch, this is a problem
> generally.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center



-- 
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] Interval for launching the table sync worker

[Kyotaro HORIGUCHI]

I confused sync and apply workers.
sync worker failure at start causes immediate retries.

At Thu, 13 Apr 2017 11:53:27 +0900, Masahiko Sawada  
wrote in 
> On Wed, Apr 12, 2017 at 11:46 PM, Peter Eisentraut
>  wrote:
> > On 4/12/17 00:48, Masahiko Sawada wrote:
> >> On Wed, Apr 12, 2017 at 1:28 PM, Peter Eisentraut
> >>> Perhaps instead of a global last_start_time, we store a per relation
> >>> last_start_time in SubscriptionRelState?
> >>
> >> I was thinking the same. But a problem is that the list of
> >> SubscriptionRelState is refreshed whenever the syncing table state
> >> becomes invalid (table_state_valid = false). I guess we need to
> >> improve these logic including GetSubscriptionNotReadyRelations().
> >
> > The table states are invalidated on a syscache callback from
> > pg_subscription_rel, which happens roughly speaking when a table
> > finishes the initial sync.  So if we're worried about failing tablesync
> > workers relaunching to quickly, this would only be a problem if a
> > tablesync of another table finishes right in that restart window.  That
> > doesn't seem a terrible issue to me.
> >
> 
> I think the table states are invalidated whenever the table sync
> worker starts, because the table sync worker updates its status of
> pg_subscription_rel and commits it before starting actual copy. So we
> cannot rely on that. I thought we can store last_start_time into
> pg_subscription_rel but it might be overkill. I'm now thinking to
> change GetSubscriptionNotReadyRealtions so that last_start_time in
> SubscriptionRelState is taken over to new list.

This resolves the problem but, if I understand correctly, the
many pallocs in process_syncing_tables_for_apply() is working on
ApplyContext and the context is reset before the next visit here
(in LogicalRepApplyLoop).

Although this is not a problem of this patch, this is a problem
generally.


regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center



-- 
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 and inheritance

[Amit Langote]

On 2017/04/13 0:10, Robert Haas wrote:
> On Wed, Apr 5, 2017 at 8:25 AM, Peter Eisentraut
>  wrote:
>> After thinking about it some more, I think the behavior we want would be
>> that changes to inheritance would reflect in the publication membership.
>>  So if you have a partitioned table, adding more partitions over time
>> would automatically add them to the publication.
> 
> I think the threshold question here is: is the partitioned table a
> member of the publication, or are the partitions members of the
> publication?

One more question could be - are we going to handle this specially only
for the partitioned tables?  Of course, if we use an approach with general
inheritance in mind, it will cover the partitioned table case, but we
might have to reconsider if the generality makes a solution prohibitive.

> Probably both should be allowed.  If you add a partition
> to the publication by name, then we should publish exactly that
> partition, full stop.

Later if someone adds the parent, current code would try to add the
partition again.

create table bar (a int);
create publication mypub for table bar;

create table foo (a int);
create table baz () inherits (foo);

alter table bar inherit foo;
alter publication mypub add table foo;
ERROR:  relation "bar" is already member of publication "mypub"

One way to avoid the error, is to add ONLY foo:

alter publication mypub add table only foo;
ALTER PUBLICATION

But then other children (baz) is not added.

There should be some way to avoid the error if the parent is added to the
same publication as the one which a child is already part of.  I think it
should be a no-op.  Sent a patch for that in a separate thread.

> If you add the partitioned table, then there is
> a choice of behaviors, including:
> 
> (1) That's an error; the user should publish the partitions instead.
> (2) That's a shorthand for all partitions, resolved at the time the
> table is added to the publication.  Later changes affect nothing.
> (3) All partitions are published, and changes in the set of partitions
> change what is published.
> 
> In my view, (3) is more desirable than (1) which is more desirable than (2).

Agree with that order.

>> We could implement this either by updating pg_publication_rel as
>> inheritance changes, or perhaps more easily by checking and expanding
>> inheritance in the output plugin/walsender.  There might be subtle
>> behavioral differences between those two approaches that we should think
>> through.  But I think one of these two should be done.
> 
> The latter approach sounds better to me.

Which, IIUC, means OpenTableList() called by both CreatePublication() and
AlterPublicationTables() does not expand inheritance and hence
pg_publication_rel entries are created only for the specified relation.
That is an important consideration because of pg_dump.  See below:

create table foo (a int);
create table bar () inherits (foo);
create publication mypub for table foo;  -- bar is added too.

$ pg_dump -s | psql -e test

ALTER PUBLICATION mypub ADD TABLE foo;
ERROR:  relation "bar" is already member of publication "mypub"

This however ain't a problem if we consider my above suggestion to make
duplicate addition to publication a no-op.

Thanks,
Amit



-- 
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: [[Parallel] Shared] Hash

[Thomas Munro]

On Thu, Apr 13, 2017 at 10:04 PM, Oleg Golovanov  wrote:
> bash-4.2$ grep Hunk *.log | grep FAILED
> 0005-hj-leader-gate-v11.patch.log:Hunk #1 FAILED at 14.
> 0010-hj-parallel-v11.patch.log:Hunk #2 FAILED at 2850.
> 0010-hj-parallel-v11.patch.log:Hunk #1 FAILED at 21.
> 0010-hj-parallel-v11.patch.log:Hunk #3 FAILED at 622.
> 0010-hj-parallel-v11.patch.log:Hunk #6 FAILED at 687.
> 0010-hj-parallel-v11.patch.log:Hunk #1 FAILED at 21.
> 0010-hj-parallel-v11.patch.log:Hunk #3 FAILED at 153.

Hi Oleg

Thanks for looking at this.  It conflicted with commit 9c7f5229.  Here
is a rebased patch set.

This version also removes some code for dealing with transient record
types which didn't work out.  I'm trying to deal with that problem
separately[1] and in a general way so that the parallel hash join
patch doesn't have to deal with it at all.

[1] 
https://www.postgresql.org/message-id/CAEepm=0ZtQ-SpsgCyzzYpsXS6e=kzwqk3g5ygn3mdv7a8da...@mail.gmail.com

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


parallel-shared-hash-v12.tgz
Description: GNU Zip compressed 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] sequence data type

[Vitaly Burovoy]

On 4/4/17, Peter Eisentraut  wrote:
> On 3/30/17 22:47, Vitaly Burovoy wrote:
>> It seemed not very hard to fix it.
>> Please find attached patch to be applied on top of your one.
>>
>> I've added more tests to cover different cases of changing bounds when
>> data type is changed.
>
> Committed all that.  Thanks!

Thank you very much!

-- 
Best regards,
Vitaly Burovoy


-- 
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] identity columns

[Vitaly Burovoy]

On 4/6/17, Vitaly Burovoy  wrote:
> On 4/6/17, Peter Eisentraut  wrote:
>> As I tried to mention earlier, it is very difficult to implement the IF
>> NOT EXISTS behavior here, because we need to run the commands the create
>> the sequence before we know whether we will need it.
>
> In fact with the function "get_attidentity" it is not so hard. Please,
> find the patch attached.
...
> I hope it is still possible to get rid of the "ADD GENERATED" syntax.

Hello hackers,

Is it possible to commit the patch from the previous letter?
It was sent before the feature freeze, introduces no new feature (only
syntax change discussed in this thread and not implemented due to lack
of a time of the author) and can be considered as a fix to the
committed patch (similar to the second round in "sequence data type").

-- 
Best regards,
Vitaly Burovoy


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