Re: [HACKERS] Small patch for pg_basebackup argument parsing

[Ryan Murphy]

Great, thanks Pierre!

I don't have a chance to try the patch tonight, but I will on the weekend
if no one else beats me to it.

On Wed, Sep 13, 2017 at 12:53 PM Pierre Ducroquet 
wrote:

> On Wednesday, September 13, 2017 2:06:50 AM CEST Daniel Gustafsson wrote:
> > > On 05 Jul 2017, at 08:32, Michael Paquier 
> > > wrote:>
> > > On Wed, Jul 5, 2017 at 2:57 PM, Ryan Murphy 
> wrote:
> > >> I tried to apply your patch to test it (though reading Robert's last
> > >> comment it seems we wish to have it adjusted before committing)... but
> > >> in any case I was not able to apply your patch to the tip of the
> master
> > >> branch (my git apply failed).  I'm setting this to Waiting On Author
> for
> > >> a new patch, and I also agree with Robert that the test can be simpler
> > >> and can go in the other order.  If you don't have time to make another
> > >> patch, let me know, I may be able to make one.>
> > > git is unhappy even if forcibly applied with patch -p1. You should
> > > check for whitespaces at the same time:
> > > $ git diff --check
> > > src/bin/pg_basebackup/pg_receivewal.c:483: indent with spaces.
> > > +char   *strtol_endptr = NULL
> > > And there are more than this one.
> >
> > Like Michael said above, this patch no longer applies and have some
> > whitespace issues.  The conflicts in applying are quite trivial though,
> so
> > it should be easy to create a new version.  Do you plan to work on this
> > during this Commitfest so we can move this patch forward?
>
> Yes, my bad. Attached to this email is the new version, that now applies on
> master.
>
> Sorry for the delay :(
>

Re: [HACKERS] Add Roman numeral conversion to to_number

[Oliver Ford]

I'll fix the brace, but there are two other patches in the first email for
tests and docs. For some reason the commitfest app didn't pick them up.

On Friday, 15 September 2017, Doug Doole  wrote:

> The following review has been posted through the commitfest application:
> make installcheck-world:  tested, passed
> Implements feature:   tested, passed
> Spec compliant:   not tested
> Documentation:not tested
>
> Code looks fine, but one niggly complaint at line 146 of the patch file
> ("while (*cp) {"). A K style brace slipped in, which doesn't match the
> formatting of the file.
>
> Given that this is providing new formatting options, there should be new
> tests added that validate the options and error handling.
>
> There's also the "do we want this?" debate from the discussion thread that
> still needs to be resolved. (I don't have an opinion either way.)
>
> I'm sending this back to the author to address the first two issues.
>
> The new status of this patch is: Waiting on Author
>
> --
> 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] path toward faster partition pruning

[Amit Langote]

Hi Dilip,

Thanks for looking at the patch.

On 2017/09/15 13:43, Dilip Kumar wrote:
> On Wed, Sep 6, 2017 at 4:08 PM, Amit Langote
>> [PATCH 2/5] WIP: planner-side changes for partition-pruning
>>
>> This patch adds a stub get_partitions_for_keys in partition.c with a
>> suitable interface for the caller to pass bounding keys extracted from the
>> query and other related information.
>>
>> Importantly, it implements the logic within the planner to match query's
>> scan keys to a parent table's partition key and form the bounding keys
>> that will be passed to partition.c to compute the list of partitions that
>> satisfy those bounds.
> 
> + Node   *leftop = get_leftop(clause);
> +
> + if (IsA(leftop, RelabelType))
> + leftop = (Node *) ((RelabelType *) leftop)->arg;
> +
> + if (equal(leftop, partkey))
> 
> It appears that this patch always assume that clause will be of form
> "var op const", but it can also be "const op var"
> 
> That's the reason in below example where in both the queries condition
> is same it can only prune in the first case but not in the second.
> 
> postgres=# explain select * from t where t.a < 2;
>QUERY PLAN
> 
>  Append  (cost=0.00..2.24 rows=1 width=8)
>->  Seq Scan on t1  (cost=0.00..2.24 rows=1 width=8)
>  Filter: (a < 2)
> (3 rows)
> 
> postgres=# explain select * from t where 2>t.a;
>QUERY PLAN
> 
>  Append  (cost=0.00..4.49 rows=2 width=8)
>->  Seq Scan on t1  (cost=0.00..2.24 rows=1 width=8)
>  Filter: (2 > a)
>->  Seq Scan on t2  (cost=0.00..2.25 rows=1 width=8)
>  Filter: (2 > a)
> (5 rows)

Yeah, there are a bunch of smarts still missing in that patch as it is.

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] path toward faster partition pruning

[Dilip Kumar]

On Wed, Sep 6, 2017 at 4:08 PM, Amit Langote
 wrote:
> On 2017/09/04 10:10, Amit Langote wrote:
>> On 2017/09/02 2:52, Robert Haas wrote:

>
> [PATCH 2/5] WIP: planner-side changes for partition-pruning
>
> This patch adds a stub get_partitions_for_keys in partition.c with a
> suitable interface for the caller to pass bounding keys extracted from the
> query and other related information.
>
> Importantly, it implements the logic within the planner to match query's
> scan keys to a parent table's partition key and form the bounding keys
> that will be passed to partition.c to compute the list of partitions that
> satisfy those bounds.
>

+ Node   *leftop = get_leftop(clause);
+
+ if (IsA(leftop, RelabelType))
+ leftop = (Node *) ((RelabelType *) leftop)->arg;
+
+ if (equal(leftop, partkey))

It appears that this patch always assume that clause will be of form
"var op const", but it can also be "const op var"

That's the reason in below example where in both the queries condition
is same it can only prune in the first case but not in the second.

postgres=# explain select * from t where t.a < 2;
   QUERY PLAN

 Append  (cost=0.00..2.24 rows=1 width=8)
   ->  Seq Scan on t1  (cost=0.00..2.24 rows=1 width=8)
 Filter: (a < 2)
(3 rows)

postgres=# explain select * from t where 2>t.a;
   QUERY PLAN

 Append  (cost=0.00..4.49 rows=2 width=8)
   ->  Seq Scan on t1  (cost=0.00..2.24 rows=1 width=8)
 Filter: (2 > a)
   ->  Seq Scan on t2  (cost=0.00..2.25 rows=1 width=8)
 Filter: (2 > a)
(5 rows)

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com


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

Re: [HACKERS] [COMMITTERS] pgsql: Add support for coordinating record typmods among parallel worke

[Tom Lane]

Andres Freund  writes:
> Sorry for missing that during review - unfortunately I don't have a computer 
> with me now - so I won't get around to this till tomorrow...

It turns out that this breaks my local build, too, so I went ahead and
pushed a fix.

At least two buildfarm members think there's something deeper broken
here:

2017-09-15 00:08:34.061 EDT [97092:75] LOG:  worker process: parallel worker 
for PID 101175 (PID 101256) was terminated by signal 11: Segmentation fault
2017-09-15 00:08:34.061 EDT [97092:76] DETAIL:  Failed process was running: 
select * from pg_get_object_address('operator of access method', 
'{btree,integer_ops,1}', '{int4,bool}');

I wonder if this indicates you forgot to consider transmission of state
to parallel worker processes.

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] [COMMITTERS] pgsql: passwordcheck: Add test suite

[Michael Paquier]

On Fri, Sep 15, 2017 at 12:02 PM, Michael Paquier
 wrote:
> On Fri, Sep 15, 2017 at 11:46 AM, Peter Eisentraut  wrote:
>> passwordcheck: Add test suite
>>
>> Also improve one error message.
>>
>> Reviewed-by: David Steele 
>
> Sorry for showing up late for this topic.
> +REGRESS_OPTS = --temp-config $(srcdir)/passwordcheck.conf
> +REGRESS = passwordcheck
> +# disabled because these tests require setting shared_preload_libraries
> +NO_INSTALLCHECK = 1
> You could have avoided all that by just issuing "load
> 'passwordcheck';" at the beginning of the test. And you gain support
> for installcheck this way.

In short you just need the attached ;)
-- 
Michael


0001-Simplify-new-test-suite-handling-of-passwordcheck.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] POC: Sharing record typmods between backends

[Tom Lane]

Thomas Munro  writes:
> On Fri, Sep 15, 2017 at 3:03 PM, Andres Freund  wrote:
>> - added typedefs to typedefs.list

> Should I do this manually with future patches?

FWIW, I'm not on board with that.  I think the version of typedefs.list
in the tree should reflect the last official pgindent run.  There's also
a problem that it only works well if *every* committer faithfully updates
typedefs.list, which isn't going to happen.

For local pgindent'ing, I pull down

https://buildfarm.postgresql.org/cgi-bin/typedefs.pl

and then add any typedefs created by the patch I'm working on to that.
But I don't put the result into the commit.  Maybe we need a bit better
documentation and/or tool support for using an unofficial typedef list.

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] POC: Sharing record typmods between backends

[Thomas Munro]

On Fri, Sep 15, 2017 at 3:03 PM, Andres Freund  wrote:
> On 2017-09-04 18:14:39 +1200, Thomas Munro wrote:
>> Thanks for the review and commits so far.  Here's a rebased, debugged
>> and pgindented version of the remaining patches.
>
> I've pushed this with minor modifications:

Thank you!

> - added typedefs to typedefs.list

Should I do this manually with future patches?

> - re-pgindented, there were some missing reindents in headers
> - added a very brief intro into session.c, moved some content repeated
>   in various places to the header - some of them were bound to become
>   out-of-date due to future uses of the facility.
> - moved NULL setting in detach hook directly after the respective
>   resource deallocation, for the not really probable case of it being
>   reinvoked due to an error in a later dealloc function
>
> Two remarks:
> - I'm not sure I like the order in which things are added to the typemod
>   hashes, I wonder if some more careful organization could get rid of
>   the races. Doesn't seem critical, but would be a bit nicer.

I will have a think about whether I can improve that.  In an earlier
version I did things in a different order and had different problems.
The main hazard to worry about here is that you can't let any typmod
number escape into shmem where it might be read by others (for example
a concurrent session that wants a typmod for a TupleDesc that happens
to match) until the typmod number is resolvable back to a TupleDesc
(meaning you can look it up in shared_typmod_table).  Not
wasting/leaking memory in various failure cases is a secondary (but
obviously important) concern.

> - I'm not yet quite happy with the Session facility. I think it'd be
>   nicer if we'd a cleaner split between the shared memory notion of a
>   session and the local memory version of it. The shared memory version
>   would live in a ~max_connections sized array, referenced from
>   PGPROC. In a lot of cases it'd completely obsolete the need for a
>   shm_toc, because you could just store handles etc in there.  The local
>   memory version then would just store local pointers etc into that.
>
>   But I think we can get there incrementally.

+1 to all of the above.  I fully expect this to get changed around quite a lot.

I'll keep an eye out for problem reports.

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


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

Re: [HACKERS] POC: Sharing record typmods between backends

[Andres Freund]

Hi,

On 2017-09-04 18:14:39 +1200, Thomas Munro wrote:
> Thanks for the review and commits so far.  Here's a rebased, debugged
> and pgindented version of the remaining patches.

I've pushed this with minor modifications:
- added typedefs to typedefs.list
- re-pgindented, there were some missing reindents in headers
- added a very brief intro into session.c, moved some content repeated
  in various places to the header - some of them were bound to become
  out-of-date due to future uses of the facility.
- moved NULL setting in detach hook directly after the respective
  resource deallocation, for the not really probable case of it being
  reinvoked due to an error in a later dealloc function


Two remarks:
- I'm not sure I like the order in which things are added to the typemod
  hashes, I wonder if some more careful organization could get rid of
  the races. Doesn't seem critical, but would be a bit nicer.

- I'm not yet quite happy with the Session facility. I think it'd be
  nicer if we'd a cleaner split between the shared memory notion of a
  session and the local memory version of it. The shared memory version
  would live in a ~max_connections sized array, referenced from
  PGPROC. In a lot of cases it'd completely obsolete the need for a
  shm_toc, because you could just store handles etc in there.  The local
  memory version then would just store local pointers etc into that.

  But I think we can get there incrementally.

It's very nice to push commits that have stats like
 6 files changed, 27 insertions(+), 1110 deletions(-)
even if it essentially has been paid forward by a lot of previous work
;)

Thanks for the work on this!

Regards,

Andres


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

Re: [HACKERS] [COMMITTERS] pgsql: passwordcheck: Add test suite

[Michael Paquier]

On Fri, Sep 15, 2017 at 11:46 AM, Peter Eisentraut  wrote:
> passwordcheck: Add test suite
>
> Also improve one error message.
>
> Reviewed-by: David Steele 

Sorry for showing up late for this topic.
+REGRESS_OPTS = --temp-config $(srcdir)/passwordcheck.conf
+REGRESS = passwordcheck
+# disabled because these tests require setting shared_preload_libraries
+NO_INSTALLCHECK = 1
You could have avoided all that by just issuing "load
'passwordcheck';" at the beginning of the test. And you gain support
for installcheck this way.
-- 
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] additional contrib test suites

[Peter Eisentraut]

On 9/14/17 11:01, David Steele wrote:
> On 9/8/17 1:32 PM, Peter Eisentraut wrote:
>>
>> Yes, some of the error messages had changed.  Fixed patches attached.
> 
> Patches apply and all tests pass.  A few comments:
> 
> * [PATCH v2 1/7] adminpack: Add test suite
> 
> There are no regular tests for pg_logdir_ls().

Added a comment about that.

> * [PATCH v2 4/7] chkpass: Add test suite
> 
> Well, this is kind of scary:
> 
> +CREATE EXTENSION chkpass;
> +WARNING:  type input function chkpass_in should not be volatile
> 
> I guess the only side effect is that this column cannot be indexed?  The
> docs say that, so OK, but is there anything else a user should be
> worried about?

Well, we're just testing, not judging. ;-)

> The rest looks good.  I'll mark this "Ready for Committer" since I'm the
> only reviewer.  I don't think anything you might add based on my
> comments above requires a re-review.
> 
> As for testing on more platforms, send it to the build farm?

OK, committed, let's see.

-- 
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] parallelize queries containing initplans

[Amit Kapila]

On Thu, Aug 31, 2017 at 11:23 AM, Amit Kapila  wrote:
> On Mon, Aug 21, 2017 at 2:40 PM, Amit Kapila  wrote:
>> On Mon, Aug 21, 2017 at 1:44 PM, Haribabu Kommi
>>  wrote:
>>>
>>>
>>> Thanks for adding more details. It is easy to understand.
>>>
>>> I marked the patch as ready for committer in the commitfest.
>>>
>
> Rebased the patch.  The output of test case added by the patch is also
> slightly changed because of the recent commit
> 7df2c1f8daeb361133ac8bdeaf59ceb0484e315a.  I have verified that the
> new test result is as expected.
>

The latest patch again needs to be rebased.  Find rebased patch
attached with this email.


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


pq_pushdown_initplan_v9.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] Trouble with amcheck

[Tom Lane]

Stephen Frost  writes:
> * Andres Freund (and...@anarazel.de) wrote:
>> I'm very unconvinced by this, given that one use of installcheck is to
>> run against an existing server. For which one might not even have access
>> to the relevant directories to install extensions into.

> Sure, but if the extensions aren't in place and you're trying to run
> make installcheck-world, it's not like it's somehow going to succeed.

I'm more or less with Andres: this is just pilot error.  If you didn't
do install-world you shouldn't expect installcheck-world to succeed.

> Now, that said, perhaps a bit more smarts would be in order here to,
> instead, check that the extensions are available before trying to run
> the checks for them.  I'm thinking about something like this: check if
> the extension is available and, if not, skip the check of that module,
> with a warning or notification that it was skipped because it wasn't
> available.

Meh.  I'm worried that that would have undesirable failure modes,
ie letting something pass when it should have failed.

Maybe we need some documentation improvements, though, to clarify
the relationships between these make targets.

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] Trouble with amcheck

[Andres Freund]

On 2017-09-14 22:36:38 -0400, Stephen Frost wrote:
> Andres,
> 
> * Andres Freund (and...@anarazel.de) wrote:
> > On 2017-09-15 02:22:49 +, Douglas Doole wrote:
> > > Thanks all. Making and installing the contribs got me rolling again. (I
> > > tried "make world" but ran into trouble with the XML docs. But that's pain
> > > and suffering for another day.)
> > > 
> > > I'd agree that "make installcheck-world" should imply that all prereqs are
> > > met - that's certainsly the normal behaviour for make.
> > 
> > I'm very unconvinced by this, given that one use of installcheck is to
> > run against an existing server. For which one might not even have access
> > to the relevant directories to install extensions into.
> 
> Sure, but if the extensions aren't in place and you're trying to run
> make installcheck-world, it's not like it's somehow going to succeed.
> 
> Failing earlier on the install seems like a reasonable thing to do
> rather than failing later halfway through the check process.

But, uh, aren't you now provoking errors because of non-existing rights
to install stuff on the system?  I'm all for improving error messages
and/or adding extra checks, but this doesn't seem to be a solution.


> Now, that said, perhaps a bit more smarts would be in order here to,
> instead, check that the extensions are available before trying to run
> the checks for them.  I'm thinking about something like this: check if
> the extension is available and, if not, skip the check of that module,
> with a warning or notification that it was skipped because it wasn't
> available.

I think that'd just lead to people not noticing that they're not
executing all tests. I'm ok with adding a hard error with a better
message or such tho.

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] Trouble with amcheck

[Stephen Frost]

Andres,

* Andres Freund (and...@anarazel.de) wrote:
> On 2017-09-15 02:22:49 +, Douglas Doole wrote:
> > Thanks all. Making and installing the contribs got me rolling again. (I
> > tried "make world" but ran into trouble with the XML docs. But that's pain
> > and suffering for another day.)
> > 
> > I'd agree that "make installcheck-world" should imply that all prereqs are
> > met - that's certainsly the normal behaviour for make.
> 
> I'm very unconvinced by this, given that one use of installcheck is to
> run against an existing server. For which one might not even have access
> to the relevant directories to install extensions into.

Sure, but if the extensions aren't in place and you're trying to run
make installcheck-world, it's not like it's somehow going to succeed.

Failing earlier on the install seems like a reasonable thing to do
rather than failing later halfway through the check process.

Now, that said, perhaps a bit more smarts would be in order here to,
instead, check that the extensions are available before trying to run
the checks for them.  I'm thinking about something like this: check if
the extension is available and, if not, skip the check of that module,
with a warning or notification that it was skipped because it wasn't
available.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] taking stdbool.h into use

[Tom Lane]

Peter Eisentraut  writes:
> 0005-Make-casting-between-bool-and-GinTernaryValue-more-r.patch
> 0008-Use-stdbool.h-if-available.patch

> These need some more work based on Tom's feedback.

> Attached is a new patch set.  Based on the discussion so far, 0001
> through 0007 might be ready; the other two need some more work.

Do you need me to do another round of tests on prairiedog/gaur/pademelon?

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] Trouble with amcheck

[Andres Freund]

On 2017-09-15 02:22:49 +, Douglas Doole wrote:
> Thanks all. Making and installing the contribs got me rolling again. (I
> tried "make world" but ran into trouble with the XML docs. But that's pain
> and suffering for another day.)
> 
> I'd agree that "make installcheck-world" should imply that all prereqs are
> met - that's certainsly the normal behaviour for make.

I'm very unconvinced by this, given that one use of installcheck is to
run against an existing server. For which one might not even have access
to the relevant directories to install extensions into.

Regards,

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] Trouble with amcheck

[Douglas Doole]

Thanks all. Making and installing the contribs got me rolling again. (I
tried "make world" but ran into trouble with the XML docs. But that's pain
and suffering for another day.)

I'd agree that "make installcheck-world" should imply that all prereqs are
met - that's certainsly the normal behaviour for make.

- Doug

Re: [HACKERS] path toward faster partition pruning

[Amit Langote]

On 2017/09/15 10:55, David Rowley wrote:
> On 21 August 2017 at 18:37, Amit Langote  
> wrote:
>> I've been working on implementing a way to perform plan-time
>> partition-pruning that is hopefully faster than the current method of
>> using constraint exclusion to prune each of the potentially many
>> partitions one-by-one.  It's not fully cooked yet though.
> 
> I'm interested in seeing improvements in this area, so I've put my
> name down to review this.

Great, thanks!

I will post rebased patches later today, although I think the overall
design of the patch on the planner side of things is not quite there yet.
Of course, your and others' feedback is greatly welcome.

Also, I must inform to all of those who're looking at this thread that I
won't be able to respond to emails from tomorrow (9/16, Sat) until 9/23,
Sat, due to some personal business.

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] Clarification in pg10's pgupgrade.html step 10 (upgrading standby servers)

[Stephen Frost]

Michael,

* Michael Paquier (michael.paqu...@gmail.com) wrote:
> On Fri, Sep 15, 2017 at 10:21 AM, Stephen Frost  wrote:
> > No, one of the baseline requirements of pg_upgrade is to *not* screw
> > with the existing cluster.  Removing its WAL or "cleaning it up"
> > definitely seems like it's violating that principle.
> 
> Not necessarily. Using --link it is game over for rollback once the
> new cluster has started. 

Yes, but not *before* the new cluster has started.

> My reference to clean up the contents of
> pg_xlog refers to the moment the new cluster has been started. 

Alright, that's technically beyond the scope of pg_upgrade then...

> This
> could be achieved with a pre-upgrade flag present on-disk that the
> startup process looks at to perform actions needed. This flag of
> course needs to depend on the version of the binary used to start
> Postgres, and is written by pg_upgrade itself which links the new
> cluster's pg_wal into the location of the old cluster. In short, if an
> upgrade to PG version N is done, and that the flag related to the
> cleanup of N is found, then actions happen. If Postgres is started
> with a binary version N-1, nothing happens, and existing flags are
> cleaned up. What I am not sure is if such extra machinery is worth
> doing as things can be saved by just moving the soft link position of
> the cluster after running pg_upgrade and before starting the new
> cluster.

Ugh.  That strikes me as far more complication than would be good for
this..

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] path toward faster partition pruning

[David Rowley]

On 21 August 2017 at 18:37, Amit Langote  wrote:
> I've been working on implementing a way to perform plan-time
> partition-pruning that is hopefully faster than the current method of
> using constraint exclusion to prune each of the potentially many
> partitions one-by-one.  It's not fully cooked yet though.

I'm interested in seeing improvements in this area, so I've put my
name down to review this.

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


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

Re: [HACKERS] Trouble with amcheck

[Michael Paquier]

On Fri, Sep 15, 2017 at 10:31 AM, Stephen Frost  wrote:
> Yes, I was working with someone earlier today who ran into exactly the
> same issue.  If you don't 'make world' or make the individual contrib
> modules, then 'make installcheck-world' isn't going to work.

Or should installcheck-world imply world? It is easy for newcomers or
even advanced hackers to fall into this trap from time to time.
-- 
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] Clarification in pg10's pgupgrade.html step 10 (upgrading standby servers)

[Michael Paquier]

On Fri, Sep 15, 2017 at 10:21 AM, Stephen Frost  wrote:
> No, one of the baseline requirements of pg_upgrade is to *not* screw
> with the existing cluster.  Removing its WAL or "cleaning it up"
> definitely seems like it's violating that principle.

Not necessarily. Using --link it is game over for rollback once the
new cluster has started. My reference to clean up the contents of
pg_xlog refers to the moment the new cluster has been started. This
could be achieved with a pre-upgrade flag present on-disk that the
startup process looks at to perform actions needed. This flag of
course needs to depend on the version of the binary used to start
Postgres, and is written by pg_upgrade itself which links the new
cluster's pg_wal into the location of the old cluster. In short, if an
upgrade to PG version N is done, and that the flag related to the
cleanup of N is found, then actions happen. If Postgres is started
with a binary version N-1, nothing happens, and existing flags are
cleaned up. What I am not sure is if such extra machinery is worth
doing as things can be saved by just moving the soft link position of
the cluster after running pg_upgrade and before starting the new
cluster.

> I tend to agree that it'd be good for the documentation to address this,
> but this is all really getting to be a bit much for a manpage to be able
> to handle, I think..

Definitely.
-- 
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] Trouble with amcheck

[Stephen Frost]

Peter, Douglas,

* Peter Geoghegan (p...@bowt.ie) wrote:
> On Thu, Sep 14, 2017 at 5:03 PM, Douglas Doole  wrote:
> > I just cloned PostgreSQL to a new machine today (Ubuntu 17.04). "make
> > install" and "make check-world" run fine but "make installcheck-world" is
> > having trouble with amcheck:
> >
> > In contrib/amcheck/results:
> >
> > CREATE EXTENSION amcheck;
> > ERROR:  could not open extension control file
> > "/home/doole/pgCommunity/install/share/postgresql/extension/amcheck.control":
> > No such file or directory
> >
> > I expect I'm missing something in the machine set up, but I'm stumped as to
> > what.
> 
> I think you need to build and install contrib, so that it is available
> to the server that you're running an installcheck against. amcheck is
> alphabetically first among contrib modules that have tests, IIRC.

Yes, I was working with someone earlier today who ran into exactly the
same issue.  If you don't 'make world' or make the individual contrib
modules, then 'make installcheck-world' isn't going to work.

I do think it'd be nice if we could provide a better error message in
such a case..

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Process startup infrastructure is a mess

[Stephen Frost]

Andres, Simon,

* Andres Freund (and...@anarazel.de) wrote:
> On 2017-09-15 01:06:54 +0100, Simon Riggs wrote:
> > If we add something to an area then its a good time to refactor it
> > since we were going to get bugs anyway.
> 
> We've added something to the area on a regular basis. As in last in
> v10. The fundamental problem with your argument is that that makes it
> impossible for most contributors to get feature in that touch these
> parts of the code - many neither have the time nor the knowledge to do
> such a refactoring. By your argument we should have rejected logical
> replication for v10 because it complicated this further, without
> cleaning it up.
> 
> Besides that, it also makes it harder to develop new features, not just
> to get them in.

I'm definitely in agreement with Andres on this one.  This isn't
refactoring of little-to-never changed code, it's refactoring bits of
the system which are changed with some regularity and looks likely to
continue to need change as we add more features moving forward, and
perhaps add greater controls over process startup.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] SCRAM in the PG 10 release notes

[Michael Paquier]

On Fri, Sep 15, 2017 at 12:10 AM, Alvaro Hernandez  wrote:
>> On the JDBC driver, strictly speaking, code has not been released yet.
>> It is scheduled for v 42.2.0, and maybe the wiki should also mention from
>> what version of the driver it is supported (I guess for all cases, unless
>> their versioning would be synced with PostgreSQL's).

Adding the information that JDBC is able to speak the SASL protocol
with Postgres from version 42.2.0 looks like a good addition to me, so
I added it on the wiki page.
-- 
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] Clarification in pg10's pgupgrade.html step 10 (upgrading standby servers)

[Stephen Frost]

Michael, all,

* Michael Paquier (michael.paqu...@gmail.com) wrote:
> On Fri, Sep 15, 2017 at 8:23 AM, Andreas Joseph Krogh
>  wrote:
> > I tested upgrading from 9.6 to 10 now, using pg_upgrade, and pg_upgrade
> > creates the new data-dir with pg_wal "in it" (just like regular initdb), so
> > pg_upgrade seems not to care about where the old version's pg_xlog was. You
> > have to move (by symlinking) pg_wal to a separate location manually *after*
> > running pg_upgrade on the master.
> 
> That's true, this should definitely be mentioned in the documentation.

Uh, this seems like something that should be *fixed*, not documented.
That initdb accepts an alternative location for pg_xlog/pg_wal now
raises that to a first-class feature, in my view, and other tools should
recognize the case and deal with it correctly.

Of course, that having been said, there's some technical challenges
there.  One being that we don't really want to mess with the old
cluster's WAL during pg_upgrade.  Frustratingly, we have a way to deal
with that case today for tablespaces, it was just never applied to WAL
when we started allowing WAL to be stored elsewhere (formally).  That
seems like it was a mistake to me.

Then again, the more I think about this, the more I wonder about putting
more-or-less everything in PGDATA into per-catalog-version directories
and making everything self-contained.  Part of the ugly bit with the
rsync is exactly that we have to go up an extra level for the data
directories themselves, and users can actually put them anywhere so
there might not even *be* a common parent directory to use.

> An improvement could be done as well here for pg_upgrade: when using
> --link, the new PGDATA created could consider as well the source
> pg_wal and create a link to it, and then clean up its contents. I am
> not completely sure if this would be worth doing as people are likely
> used to the current flow though. The documentation needs to outline
> the matter at least.

No, one of the baseline requirements of pg_upgrade is to *not* screw
with the existing cluster.  Removing its WAL or "cleaning it up"
definitely seems like it's violating that principle.

I tend to agree that it'd be good for the documentation to address this,
but this is all really getting to be a bit much for a manpage to be able
to handle, I think..

Thaks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] PATCH : Generational memory allocator (was PATCH: two slab-like memory allocators)

[Tomas Vondra]

On 09/14/2017 04:21 PM, Simon Riggs wrote:
> On 14 August 2017 at 01:35, Tomas Vondra  wrote:
>> Hi,
>>
>> Attached is a rebased version of the Generational context, originally
>> submitted with SlabContext (which was already committed into Pg 10).
>>
>> The main change is that I've abandoned the pattern of defining a Data
>> structure and then a pointer typedef, i.e.
>>
>> typedef struct GenerationContextData { ... } GenerationContextData;
>> typedef struct GenerationContextData *GenerationContext;
>>
>> Now it's just
>>
>> typedef struct GenerationContext { ... } GenerationContext;
>>
>> mostly because SlabContext was committed like that, and because Andres was
>> complaining about this code pattern ;-)
>>
>> Otherwise the design is the same as repeatedly discussed before.
>>
>> To show that this is still valuable change (even after SlabContext and
>> adding doubly-linked list to AllocSet), I've repeated the test done by
>> Andres in [1] using the test case described in [2], that is
>>
>>   -- generate data
>>   SELECT COUNT(*) FROM (SELECT test1()
>>   FROM generate_series(1, 5)) foo;
>>
>>   -- benchmark (measure time and VmPeak)
>>   SELECT COUNT(*) FROM (SELECT *
>>   FROM pg_logical_slot_get_changes('test', NULL,
>> NULL, 'include-xids', '0')) foo;
>>
>> with different values passed to the first step (instead of the 5). The
>> VmPeak numbers look like this:
>>
>>  N   masterpatched
>> --
>> 10   1155220 kB  361604 kB
>> 20   2020668 kB  434060 kB
>> 30   2890236 kB  502452 kB
>> 40   3751592 kB  570816 kB
>> 50   4621124 kB  639168 kB
>>
>> and the timing (on assert-enabled build):
>>
>>  N   masterpatched
>> --
>> 10  1103.182 ms 412.734 ms
>> 20  2216.711 ms 820.438 ms
>> 30  3320.095 ms1223.576 ms
>> 40  4584.919 ms1621.261 ms
>> 50  5590.444 ms2113.820 ms
>>
>> So it seems it's still a significant improvement, both in terms of memory
>> usage and timing. Admittedly, this is a single test, so ideas of other
>> useful test cases are welcome.
> 
> This all looks good.
> 
> What I think this needs is changes to
>src/backend/utils/mmgr/README
> which decribe the various options that now exist (normal?, slab) and
> will exist (generational)
> 
> Don't really care about the name, as long as its clear when to use it
> and when not to use it.
> 
> This description of generational seems wrong: "When the allocated
> chunks have similar lifespan, this works very well and is extremely
> cheap."
> They don't need the same lifespan they just need to be freed in groups
> and in the order they were allocated.
> 

Agreed, should be described differently. What matters is (mostly) FIFO
pattern of the palloc/pfree requests, which allows us to release the memory.

>
> For this patch specifically, we need additional comments in 
> reorderbuffer.c to describe the memory allocation pattern in that 
> module so that it is clear that the choice of allocator is useful
> and appropriate, possibly with details of how that testing was
> performed, so it can be re-tested later or tested on a variety of
> platforms.
> 

Including details about the testing into reorderbuffer.c does not seem
very attractive to me. I don't recall any other place describing the
tests in detail. Why not to discuss the details here, and then include a
link to this thread in the commit message?

In any case, I doubt anyone will repeat the testing on a variety of
platforms (and I don't have any such comprehensive test suite anyway).
What will likely happen is someone bumping into a poorly performing
corner case, we will analyze it and fix it as usual.

>
> Particularly in reorderbuffer, surely we will almost immediately
> reuse chunks again, so is it worth issuing free() and then malloc()
> again soon after? Does that cause additional overhead we could also
> avoid? Could we possibly keep the last/few free'd chunks around
> rather than re-malloc?
> 

I haven't seen anything like that in tests I've done. The malloc/free
overhead is negligible thanks as our allocators significantly reduce the
number of calls to those functions. If we happen to run into such case,
it shouldn't be difficult to keep a few empty blocks. But perhaps we can
leave that as a future optimization.

>
> Seems like we should commit this soon.
> 

Thanks.

regards

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


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

Re: [HACKERS] Add Roman numeral conversion to to_number

[Doug Doole]

The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:   tested, passed
Spec compliant:   not tested
Documentation:not tested

Code looks fine, but one niggly complaint at line 146 of the patch file ("while 
(*cp) {"). A K style brace slipped in, which doesn't match the formatting of 
the file.

Given that this is providing new formatting options, there should be new tests 
added that validate the options and error handling.

There's also the "do we want this?" debate from the discussion thread that 
still needs to be resolved. (I don't have an opinion either way.)

I'm sending this back to the author to address the first two issues.

The new status of this patch is: Waiting on Author

-- 
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] Process startup infrastructure is a mess

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Andres Freund
> I think we should seriously consider doing a larger refactoring of this
> soon.  I've some ideas about what to do, but I'd welcome some thoughts on
> whether others consider this a serious problem or not, and what they think
> we should do about this, first.

Thank you for raising this.  I think so, too.  Some other things that quickly 
come to mind are:

* The signal handlers are similar in many processes and redundant.  It's not 
easy to see how the processes respond to a particular signal and what signal 
can be used for new things such as dumping the stack trace in the server log.  
Maybe we can have an array of process attributes that specify the signal 
handlers, startup/shutdown order, etc.  Then the common process management code 
handles processes based on the information in the array.

* postmaster.c is very large (more than 6,000 lines) and hard to read.

* It may be easy to forget adding initialization code in the Windows-specific 
path (SubPostmasterMaain).

* It's confusing to find out which process counts toward max_connections, 
MaxBackends, the count of auxiliary processes.  As a user, I'm sometimes 
confused about the relationship between max_connections, 
autovacuum_max_workers, max_wal_senders, max_worker_processes.

Regards
Takayuki Tsunakawa
 




-- 
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] Clarification in pg10's pgupgrade.html step 10 (upgrading standby servers)

[Michael Paquier]

On Fri, Sep 15, 2017 at 8:23 AM, Andreas Joseph Krogh
 wrote:
> I tested upgrading from 9.6 to 10 now, using pg_upgrade, and pg_upgrade
> creates the new data-dir with pg_wal "in it" (just like regular initdb), so
> pg_upgrade seems not to care about where the old version's pg_xlog was. You
> have to move (by symlinking) pg_wal to a separate location manually *after*
> running pg_upgrade on the master.

That's true, this should definitely be mentioned in the documentation.
An improvement could be done as well here for pg_upgrade: when using
--link, the new PGDATA created could consider as well the source
pg_wal and create a link to it, and then clean up its contents. I am
not completely sure if this would be worth doing as people are likely
used to the current flow though. The documentation needs to outline
the matter at least.
-- 
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] Partition-wise join for join between (declaratively) partitioned tables

[Amit Langote]

On 2017/09/15 4:43, Robert Haas wrote:
> On Thu, Sep 14, 2017 at 8:06 AM, Ashutosh Bapat
>  wrote:
>> I have few changes to multi-level expansion patch as per discussion in
>> earlier mails
> 
> OK, I have committed
> 0002-Multi-level-partitioned-table-expansion.patch with a few cosmetic
> changes.
> 
> Phew, getting that sorted out has been an astonishing amount of work.

Yeah, thanks to both of you.  Now on to other complicated stuff. :)

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] Small code improvement for btree

[Doug Doole]

The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:   tested, passed
Spec compliant:   tested, passed
Documentation:not tested

Looks good to me.

The new status of this patch is: Ready for Committer

-- 
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] expanding inheritance in partition bound order

[Amit Langote]

On 2017/09/15 1:37, Robert Haas wrote:
> On Thu, Sep 14, 2017 at 7:56 AM, Amit Khandekar  
> wrote:
>> On 14 September 2017 at 06:43, Amit Langote
>>> langote_amit...@lab.ntt.co.jp> wrote:
>>> Attached updated patch.
>>
>> @@ -1222,151 +1209,130 @@ PartitionDispatch *
>>  RelationGetPartitionDispatchInfo(Relation rel,
>>  int
>> *num_parted, List **leaf_part_oids)
>>  {
>> +   List   *pdlist;
>> PartitionDispatchData **pd;
>>
>> +   get_partition_dispatch_recurse(rel, NULL, , leaf_part_oids);
>>
>> Above, pdlist is passed uninitialized. And then inside
>> get_partition_dispatch_recurse(), it is used here :
>> *pds = lappend(*pds, pd);
>>
>> 
>>
>> pg_indent says more alignments needed. For e.g. gettext_noop() call
>> below needs to be aligned:
>> pd->tupmap = convert_tuples_by_name(RelationGetDescr(parent),
>> tupdesc,
>> gettext_noop("could not convert row type"));
>>
>> 
>>
>> Other than that, the patch looks good to me. I verified that the leaf
>> oids are ordered exaclty in the order of the UPDATE subplans output.
> 
> Committed with fixes for those issues and a few other cosmetic changes.

Thanks Amit for the review and Robert for committing.

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] Process startup infrastructure is a mess

[Andres Freund]

On 2017-09-15 01:06:54 +0100, Simon Riggs wrote:
> On 14 September 2017 at 22:44, Andres Freund  wrote:
> 
> > The way we currently start and initialize individual postgres (sub-)
> > processes is pretty complicated and duplicative.  I've a couple
> > complaints:
> ...
> > I think we should seriously consider doing a larger refactoring of this
> > soon.  I've some ideas about what to do, but I'd welcome some thoughts
> > on whether others consider this a serious problem or not, and what they
> > think we should do about this, first.
> 
> Refactoring without a purpose is a negative for me. It takes time,
> introduces bugs and means the greater code churn over time introduces
> more bugs because fewer people have seen the code. That is arguable,
> but when we compare the priority of that against things people want
> and need there is little contest in my mind.

That's true for code that's not going to be touched anymore, largely bug
free, and can be understood with reasonable effort. Neither is the case
here.


> If we add something to an area then its a good time to refactor it
> since we were going to get bugs anyway.

We've added something to the area on a regular basis. As in last in
v10. The fundamental problem with your argument is that that makes it
impossible for most contributors to get feature in that touch these
parts of the code - many neither have the time nor the knowledge to do
such a refactoring. By your argument we should have rejected logical
replication for v10 because it complicated this further, without
cleaning it up.

Besides that, it also makes it harder to develop new features, not just
to get them in.

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] Trouble with amcheck

[Peter Geoghegan]

On Thu, Sep 14, 2017 at 5:03 PM, Douglas Doole  wrote:
> I just cloned PostgreSQL to a new machine today (Ubuntu 17.04). "make
> install" and "make check-world" run fine but "make installcheck-world" is
> having trouble with amcheck:
>
> In contrib/amcheck/results:
>
> CREATE EXTENSION amcheck;
> ERROR:  could not open extension control file
> "/home/doole/pgCommunity/install/share/postgresql/extension/amcheck.control":
> No such file or directory
>
> I expect I'm missing something in the machine set up, but I'm stumped as to
> what.

I think you need to build and install contrib, so that it is available
to the server that you're running an installcheck against. amcheck is
alphabetically first among contrib modules that have tests, IIRC.

-- 
Peter Geoghegan


-- 
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] Process startup infrastructure is a mess

[Simon Riggs]

On 14 September 2017 at 22:44, Andres Freund  wrote:

> The way we currently start and initialize individual postgres (sub-)
> processes is pretty complicated and duplicative.  I've a couple
> complaints:
...
> I think we should seriously consider doing a larger refactoring of this
> soon.  I've some ideas about what to do, but I'd welcome some thoughts
> on whether others consider this a serious problem or not, and what they
> think we should do about this, first.

Refactoring without a purpose is a negative for me. It takes time,
introduces bugs and means the greater code churn over time introduces
more bugs because fewer people have seen the code. That is arguable,
but when we compare the priority of that against things people want
and need there is little contest in my mind.

If we add something to an area then its a good time to refactor it
since we were going to get bugs anyway.

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


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

[HACKERS] Trouble with amcheck

[Douglas Doole]

I just cloned PostgreSQL to a new machine today (Ubuntu 17.04). "make
install" and "make check-world" run fine but "make installcheck-world" is
having trouble with amcheck:

In contrib/amcheck/results:

CREATE EXTENSION amcheck;
ERROR:  could not open extension control file
"/home/doole/pgCommunity/install/share/postgresql/extension/amcheck.control":
No such file or directory

I expect I'm missing something in the machine set up, but I'm stumped as to
what.

Any suggestions?

Thanks

- Doug

Re: [HACKERS] Log LDAP "diagnostic messages"?

[Michael Paquier]

On Fri, Sep 15, 2017 at 1:38 AM, Robert Haas  wrote:
> On Thu, Sep 14, 2017 at 10:21 AM, Alvaro Herrera
>  wrote:
>> BTW I added --with-ldap and --with-pam to the configure line for the
>> reports in coverage.postgresql.org and the % covered in auth.c went from
>> 24% to 18.9% (from very bad to terribly sad).
>
> Improved code coverage is becoming a fad.

I don't think that this is necessarily bad for authentication. I mean,
you write something and make sure that it actually works, but you also
make sure that the code you have is *compatible* with libraries the
code is linking to. The second part is important to me. For example
with the SSL tests we can know if there is a breakage, and if this
involves our code of OpenSSL's say after a version upgrade.

Now I think as well that it is not completely the role of this patch
to provide more test coverage for LDAP now because infrastructure
lacks. One requirement that is showing up as a remark from Peter E
(from the pg_receivewal --endpos thread
https://www.postgresql.org/message-id/49b529c1-0f44-d905-b33c-005ec0114...@2ndquadrant.com),
is that we should have a simple perl module allowing to find easily if
Postgres is compiled with a given dependency or not so as tests can
decide if they have to be skipped or run.
-- 
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] Warnings "unrecognized node type" for some DDLs with log_statement = 'ddl'

[Michael Paquier]

On Fri, Sep 15, 2017 at 6:19 AM, Robert Haas  wrote:
> On Thu, Sep 14, 2017 at 6:02 AM, Ashutosh Sharma  
> wrote:
>> Hmm...I am also able to reproduce the failures reported by you. Your
>> patch does solve the problem. Just to confirm if we are still missing
>> TAGS for any other Statement nodes, I had a quick look into the list
>> given in 'nodes.h' file but couldn't find any missing nodes. So, yes,
>> your patch does solve the problem completely and looks good to me.
>> Thanks.
>
> Committed and back-patched.

Thanks Ashutosh and Robert.
-- 
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] Clarification in pg10's pgupgrade.html step 10 (upgrading standby servers)

[Andreas Joseph Krogh]

På torsdag 14. september 2017 kl. 21:13:56, skrev Bruce Momjian <
br...@momjian.us >:
On Thu, Sep 14, 2017 at 08:49:24PM +0200, Andreas Joseph Krogh wrote:
 >     I think the tablespace example is clear enough to modify for WAL and we
 >     instruct them right below that example to do WAL.
 >
 >  
 > Well, it's not following the exact same structure as there's no
 > "version-directory" in pg_xlog, so the "rsync the version-dirs into it's 
parent
 > on the target" isn't what's happening.
 >  
 > That's why I think this makes sense to mention for the sake of a complete
 > example:
 >
 > rsync --archive --delete --hard-links --size-only 
/vol1/postgres/9.6/pg_xlog \
 >       /vol1/postgres/10/pg_wal standby.example.com:/vol1/postgres/10/pg_wal

 Well, there is technically no need for version directories in pgdata
 either --- installers just create them.
 
 
I tested upgrading from 9.6 to 10 now, using pg_upgrade, and pg_upgrade 
creates the new data-dir with pg_wal "in it" (just like regular initdb), so 
pg_upgrade seems not to care about where the old version's pg_xlog was. You 
have to move (by symlinking) pg_wal to a separate location manually *after* 
running pg_upgrade on the master. No special handling is needed when rsync'ing 
it over to the standby, so it doesn't need any --hard-links or --size-only, 
correct?
 
Given the path, on the upgraded primary, to pg_wal is /custom/path/to/pg_wal, 
the rsync command will be:
 
rsync --archive --delete /custom/path/to/pg_wal 
standby.example.com:/custom/path/to/pg_wal
 
I think it's useful to mention this to eliminate any doubt.
 
I also think it's worth mentioning that you have to manually move pg_wal to a 
custom location after running pg_upgrade as it will not preserve/use the old 
path.
 
Thanks.

 --
 Andreas Joseph Krogh

Re: [HACKERS] [POC] hash partitioning

[Thom Brown]

On 14 September 2017 at 09:58, amul sul  wrote:
> On Wed, Sep 13, 2017 at 7:43 PM, Jesper Pedersen
>  wrote:
>>
>> Hi Amul,
>>
>> On 09/08/2017 08:40 AM, amul sul wrote:
>>>
>>> Rebased 0002 against this commit & renamed to 0001, PFA.
>>>
>>
>> This patch needs a rebase.
>>
>
> Thanks for your note.
> Attached is the patch rebased on the latest master head.
> Also added error on
> creating
> d
> efault partition
> for the hash partitioned table
> ,
> and updated document &
> test script for the same.

Sorry, but this needs another rebase as it's broken by commit
77b6b5e9ceca04dbd6f0f6cd3fc881519acc8714.

Thom


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

Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

[Michael Paquier]

On Fri, Sep 15, 2017 at 1:58 AM, Peter Eisentraut
 wrote:
> Second thoughts, to make things simpler.  All we need for channel
> binding is a connection flag that says "I require channel binding".  It
> could be modeled after the sslmode parameter, e.g., cbind=disable (maybe
> for debugging), cbind=prefer (default), cbind=require.  If you specify
> "require", then libpq would refuse to proceed unless scram-sha2-256-plus
> (or future similar mechanisms) was offered for authentication.
>
> We don't even need a parameter that specifies which channel binding type
> to use.  If libpq implements tls-unique, it should always use that.  We
> might need a flag for testing other types, but that should not be an
> in-the-user's-face option.

JDBC folks are willing to have end-point, and we should have a way to
enforce it in my opinion for at least testing with an implementation
at client-level in Postgres core. I agree that without the JDBC needs
having a on/off switch would be sufficient, and actually RFC compliant
as tls-unique is mandatory.
-- 
Michael


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

[HACKERS] Process startup infrastructure is a mess

[Andres Freund]

Hi,

The way we currently start and initialize individual postgres (sub-)
processes is pretty complicated and duplicative.  I've a couple
complaints:

1) It's completely non-obvious that bootstrap.c:AuxiliaryProcessMain()
   can get invoked both via postmaster for subprocesses (startup, wal
   writer, bgwriter, checkpointer, wal receiver, "checker"), as well as
   directly via main.c for the bootstrap processes.  Note the autovacuum
   launcher, archiver, logger are *not* started this way.

2) Most of the processes mentioned in 1) and some additional ones
   (autovac launcher and walsender / autovacuum workers to some degree)
   duplicate a lot of logic for startup, error handling and main loop.
   Especially the error handling code is order dependent, complex, and
   has been broken in various subprocesses in the past.

3) There exists yet *another* way of starting processes in the form of
   background workers.  Initially that was "just" for extensions, but is
   now employed for builtin tasks too, like the logical rep launcher /
   workers.  In the course of that special case handling had to be
   sprinkled around, because the bgworker logic isn't quite able to be
   good enough for something builtin.

4) The naming of initialization functions is, uh, not particularly
   clear. How many people can differentiate, without checking,
   BaseInit(), BackendInitialize(), InitPostgres(), InitProcess().

I think the complexity here just grew incrementally with the addition of
more and more subprocesses, without sufficient refactoring. I did some
in 31c453165b5a6, but that's not even remotely enough.

I think we should seriously consider doing a larger refactoring of this
soon.  I've some ideas about what to do, but I'd welcome some thoughts
on whether others consider this a serious problem or not, and what they
think we should do about this, first.

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] Warnings "unrecognized node type" for some DDLs with log_statement = 'ddl'

[Robert Haas]

On Thu, Sep 14, 2017 at 6:02 AM, Ashutosh Sharma  wrote:
> Hmm...I am also able to reproduce the failures reported by you. Your
> patch does solve the problem. Just to confirm if we are still missing
> TAGS for any other Statement nodes, I had a quick look into the list
> given in 'nodes.h' file but couldn't find any missing nodes. So, yes,
> your patch does solve the problem completely and looks good to me.
> Thanks.

Committed and back-patched.

-- 
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] Patches that don't apply or don't compile: 2017-09-12

[Thomas Munro]

On Fri, Sep 15, 2017 at 8:13 AM, Robert Haas  wrote:
> On Wed, Sep 13, 2017 at 5:18 AM, Alvaro Herrera  
> wrote:
>> Thinking further, I think changing patch status automatically may never
>> be a good idea; there's always going to be some amount of common sense
>> applied beforehand (such as if a conflict is just an Oid catalog
>> collision, or a typo fix in a recent commit, etc).  Such conflicts will
>> always raise errors with a tool, but would never cause a (decent) human
>> being from changing the patch status to WoA.
>
> Well it would perhaps be fine if sending an updated patch bounced it
> right back to Needs Review.  But if things are only being auto-flipped
> in one direction that's going to get tedious.
>
> Or one could imagine having the CF app show the CI status alongside
> the existing status column.

FWIW that last thing is the idea that I've been discussing with
Magnus.  That web page I made wouldn't exist, and the "apply" and
"build" badges would appear directly on the CF app and you'd be able
to sort and filter by them etc.  The main submission status would
still be managed by humans and the new apply and build statuses would
be managed by faithful robot servants.

How exactly that would work, I don't know.  One idea is that a future
cfbot, rewritten in better Python and adopted into the pginfra
universe, pokes CF via an HTTPS endpoint so that the CF app finishes
up with the information in its database.

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


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

Re: [HACKERS] [bug fix] PG10: libpq doesn't connect to alternative hosts when some errors occur

[David G. Johnston]

On Thursday, September 14, 2017, Robert Haas  wrote:

> On Thu, Sep 14, 2017 at 3:23 AM, Tsunakawa, Takayuki
> > wrote:
> > Sorry again, but how can we handle this?  A non-PG-developer, Tels (and
> possibly someone else, IIRC) claimed that the behavior be changed during
> the beta period.  Why should we do nothing?
>
> Because we do not have consensus on changing it.  I've decided that
> you're right, but several other people are saying "no".  I can't just
> go change it in the face of objections.
>
>
Add my +1 for changing this to the official tally.

David J.

Re: [HACKERS] [PATCH] Improve geometric types

[Robert Haas]

On Thu, Sep 14, 2017 at 3:33 AM, Kyotaro HORIGUCHI
 wrote:
> I recall a bit about the double-evaluation hazards. I think the
> functions needs a comment describing the reasons so that anyone
> kind won't try to merge them into a macro again.

I think we can count on PostgreSQL developers to understand the
advantages of an inline function over a macro.  Even if they don't,
the solution can't be to put a comment in every place where an inline
function is used explaining it.  That would be very repetitive.

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

[HACKERS] Re: [bug fix] PG10: libpq doesn't connect to alternative hosts when some errors occur

[Robert Haas]

On Thu, Sep 14, 2017 at 3:23 AM, Tsunakawa, Takayuki
 wrote:
> Sorry again, but how can we handle this?  A non-PG-developer, Tels (and 
> possibly someone else, IIRC) claimed that the behavior be changed during the 
> beta period.  Why should we do nothing?

Because we do not have consensus on changing it.  I've decided that
you're right, but several other people are saying "no".  I can't just
go change it in the face of objections.

-- 
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] Patches that don't apply or don't compile: 2017-09-12

[Robert Haas]

On Wed, Sep 13, 2017 at 5:18 AM, Alvaro Herrera  wrote:
> Thinking further, I think changing patch status automatically may never
> be a good idea; there's always going to be some amount of common sense
> applied beforehand (such as if a conflict is just an Oid catalog
> collision, or a typo fix in a recent commit, etc).  Such conflicts will
> always raise errors with a tool, but would never cause a (decent) human
> being from changing the patch status to WoA.

Well it would perhaps be fine if sending an updated patch bounced it
right back to Needs Review.  But if things are only being auto-flipped
in one direction that's going to get tedious.

Or one could imagine having the CF app show the CI status alongside
the existing status column.

-- 
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] postgres_fdw super user checks

[Robert Haas]

On Thu, Sep 14, 2017 at 2:33 PM, Jeff Janes  wrote:
> I think that foreign tables ought to behave as views do, where they run as
> the owner rather than the invoker.  No one has talked me out of it, but no
> one has supported me on it either.  But I think it is too late to change
> that now.

That's an interesting point.  I think that you can imagine use cases
for either method.  Obviously, if what you want to do is drill a hole
through the Internet to another server and then expose it to some of
your fellow users, having the FDW run with the owner's permissions
(and credentials) is exactly right.  But there's another use case too,
which is where you have something that looks like a multi-user
sharding cluster.  You want each person's own credentials to carry
over to everything they do remotely.

I feel like the USER MAPPING stuff is a pretty clunky and annoying way
of trying to make this work, no matter which of those use cases you
happen to have.  But I'm not exactly sure what would be better,
either, and like you say, it's a bit late to be breaking compatibility
at this point.

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


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

[HACKERS] Pre-existing bug in trigger.c

[Tom Lane]

While fooling with the transition-tables bug, I noticed a problem
in trigger.c that has been there a very long time.  AfterTriggerEndQuery
correctly notes

 * ... Be careful here: firing a
 * trigger could result in query_stack being repalloc'd, so we can't save
 * its address across afterTriggerInvokeEvents calls.

However, it then proceeds to pass the address of a query_stack item to
afterTriggerInvokeEvents, so that if such a repalloc occurs,
afterTriggerInvokeEvents is already working with an obsolete dangling
pointer while it scans the rest of the events.

This isn't very trivial to fix.  I thought of making a local copy of
the events pointer struct, but that's problematic because data can
pass in a couple of directions here.  Queuing of additional trigger
events would modify the events list from "outside", while
afterTriggerInvokeEvents occasionally wants to do

if (chunk == events->tail)
events->tailfree = chunk->freeptr;

which has to be in sync with the real state of the events list
including any subsequent additions.

I think possibly the best solution is to change the query_stack
data structure enough so that pre-existing entries don't get
moved during an enlargement.  Or maybe we can fix it so the
"active" events list has a static location and the items in the
query_stack are only valid for levels below the top.

Given the lack of reports traceable to this, this doesn't seem
super urgent to fix, so I'm not going to try to address it while
hacking the transition table business.  But we'll need to return
to it later.  Whatever we do about it has to be back-patched
further than v10, anyway.

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

[Robert Haas]

On Thu, Sep 14, 2017 at 2:05 PM, Jesper Pedersen
 wrote:
> However, it is a little bit difficult to follow the dependencies between
> different partition patches, so I may not always provide sane feedback, as
> seen in [1].
>
> [1]
> https://www.postgresql.org/message-id/579077fd-8f07-aff7-39bc-b92c855cdb70%40redhat.com

Yeah, no issues.  I knew about the dependency between those patches,
but I'm pretty sure there wasn't any terribly explicit discussion
about it, even if the issue probably came up parenthetically someplace
or other.  Oops.

-- 
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] DROP SUBSCRIPTION hangs if sub is disabled in the same transaction

[Arseny Sher]

Peter Eisentraut  writes:

> On 9/13/17 07:00, Arseny Sher wrote:
>> On the other hand, forbidding to execute disable sub and drop sub in one
>> transaction makes it impossible e.g. to drop subscription in trigger
>
> Disabling a subscription before dropping it isn't very useful, is it?
> So I'm not sure this is an important use case we need to optimize.  We
> just need to prevent it from hanging.

Not quite so. Currently it is forbidded to set subscription's slot to
NONE on enabled sub, and the only way to drop sub without touching the
slot is to run ALTER SUBSCRIPTION SET (SLOT_NAME = NONE) beforehand.
Practically this means that we have to go through disable sub -> alter
sub -> drop sub pipeline if we want to left the slot alone or just would
like to drop sub in transaction block -- with replication slot set the
latter is impossible.

--
Arseny Sher


-- 
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] Partition-wise join for join between (declaratively) partitioned tables

[Robert Haas]

On Thu, Sep 14, 2017 at 8:06 AM, Ashutosh Bapat
 wrote:
> I have few changes to multi-level expansion patch as per discussion in
> earlier mails

OK, I have committed
0002-Multi-level-partitioned-table-expansion.patch with a few cosmetic
changes.

Phew, getting that sorted out has been an astonishing amount of work.

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

[HACKERS] COMMIT TRIGGERs, take n, implemented with CONSTRAINT TRIGGERS

[Nico Williams]

I've read through several old threads on COMMIT TRIGGERs.  Rather than
write a lengthy post addressing past debates, here's an implementation
and demonstration of [an approximation of] COMMIT TRIGGERs with natural
and _desirable_ semantics:

 - commit triggers run exactly once in any write transaction

 - commit triggers run at the _end_ of any write transaction

 - multiple commit triggers may be declared, and they run in name
   lexical order

 - commit triggers do NOT run in read-only transactions

 - commit trigger procedures can do anything any any other trigger
   procedure can do: DDL, DML, NOTIFY, ...

There is just one undesirable bit of semantics in this implementation:
unprivileged users can break its semantics by executing SET CONSTRAINTS
... IMMEDIATE.  Obviously this is bad, at least for some possible uses
of commit triggers.

Also, this implementation is somewhat inefficient since under the hood
it uses deferred CONSTRAINT TRIGGERs, which have to be FOR EACH ROW
triggers...

To use this:

 - download commit_trigger.sql (reviews welcome!)
   
 - run this in psql:

  -- Load commit trigger functionality:
  \i commit_trigger.sql

 - run this in psql to demo:

  -- CREATE COMMIT TRIGGER egt
  -- EXECUTE PROCEDURE commit_trigger.example_proc();
  INSERT INTO commit_trigger.triggers
  (trig_name, proc_schema, proc_name)
  SELECT 'egt', 'commit_trigger', 'example_proc';

  CREATE SCHEMA eg;
  CREATE TABLE eg.x(a text primary key);
  BEGIN;
  INSERT INTO eg.x (a) VALUES('foo');
  INSERT INTO eg.x (a) VALUES('bar');
  COMMIT;
  INSERT INTO eg.x (a) VALUES('foobar');
  INSERT INTO eg.x (a) VALUES('baz');
  DROP TABLE eg.x CASCADE;

   There should be exactly one NOTICE for the first transaction, and
   exactly one each for the two INSERTs subsequently done in auto-commit
   mode.

I hope this will put to rest all objections to COMMIT TRIGGERS, and that
it will lead to a proper implementation.

Uses of COMMIT TRIGGERs include:

 - update/refresh view materializations
 - consistency checks
 - NOTIFY
 - record history (in particular, record transaction boundaries)
 - and, no doubt, others

https://github.com/twosigma/postgresql-contrib/
https://github.com/twosigma/postgresql-contrib/blob/master/commit_trigger.sql
https://raw.githubusercontent.com/twosigma/postgresql-contrib/master/commit_trigger.sql

Cheers,

Nico
-- 


-- 
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] Call RelationDropStorage() for broader range of object drops.

[Alexander Korotkov]

On Thu, Sep 14, 2017 at 1:05 PM, Alvaro Herrera 
wrote:

> Hadi Moshayedi wrote:
> > To provide more context, in cstore_fdw creating the storage is easy, we
> > only need to hook into CREATE FOREIGN TABLE using event triggers.
> Removing
> > the storage is not that easy, for DROP FOREIGN TABLE we can use event
> > triggers.
>
> This all sounds a little more complicated than it should ... but since
> FDW are not really IMO the right interface to be implementing a
> different storage format, I'm not terribly on board with supporting this
> more completely.  So what you're doing now is probably acceptable.


+1,
FDW looks OK for prototyping pluggable storage, but it doesn't seem
suitable for permanent implementation.
BTW, Hadi, could you visit "Pluggable storage" thread and check how
suitable upcoming pluggable storage API is for cstore?

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

Re: [HACKERS] Clarification in pg10's pgupgrade.html step 10 (upgrading standby servers)

[Bruce Momjian]

On Wed, Sep 13, 2017 at 07:39:31PM +0200, Michael Banck wrote:
> On Tue, Sep 12, 2017 at 07:38:40PM -0400, Stephen Frost wrote:
> > Further, really, I think we should provide a utility to do all of the
> > above instead of using rsync- and that utility should do some additional
> > things, such as:
> > 
> > - Check that the control file on the primary and replica show that they
> >   reached the same point prior to the pg_upgrade.  If they didn't, then
> >   things could go badly as there's unplayed WAL that the primary got
> >   through and the replica didn't.
> > 
> > - Not copy over unlogged data, or any other information that shouldn't
> >   be copied across.
> > 
> > - Allow the directory structures to be more different between the
> >   primary and the replica than rsync allows (wouldn't have to have a
> >   common subdirectory on the replica).
> > 
> > - Perhaps other validation checks or similar.
> > 
> > Unfortunately, this is a bit annoying as it necessairly involves running
> > things on both the primary and the replica from the same tool, without
> > access to PG, meaning we'd have to work through something else (such as
> > SSH, like rsync does, but then what would we do for Windows...?).
> 
> Maybe pg_rewind's mechanism could be partially reused for this as it
> seems to accomplish something vaguely similar AIUI?

pg_rewind works at the WAL level while this is at the file system level.

-- 
  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] Clarification in pg10's pgupgrade.html step 10 (upgrading standby servers)

[Bruce Momjian]

On Thu, Sep 14, 2017 at 08:49:24PM +0200, Andreas Joseph Krogh wrote:
> I think the tablespace example is clear enough to modify for WAL and we
> instruct them right below that example to do WAL.
> 
>  
> Well, it's not following the exact same structure as there's no
> "version-directory" in pg_xlog, so the "rsync the version-dirs into it's 
> parent
> on the target" isn't what's happening.
>  
> That's why I think this makes sense to mention for the sake of a complete
> example:
> 
> rsync --archive --delete --hard-links --size-only /vol1/postgres/9.6/pg_xlog \
>   /vol1/postgres/10/pg_wal standby.example.com:/vol1/postgres/10/pg_wal

Well, there is technically no need for version directories in pgdata
either --- installers just create them.

-- 
  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] Clarification in pg10's pgupgrade.html step 10 (upgrading standby servers)

[Bruce Momjian]

On Wed, Sep 13, 2017 at 12:16:33PM -0400, Stephen Frost wrote:
> Bruce,
> 
> * Bruce Momjian (br...@momjian.us) wrote:
> > I have applied the attached patch to show examples of using rsync on
> > PGDATA and tablespaces, documented that rsync is only useful when in
> > link mode, and explained more clearly how rsync handles links.  You can
> > see the results here:
> > 
> > http://momjian.us/pgsql_docs/pgupgrade.html
> > 
> > Any more improvements?
> 
> First off, I'd strongly suggest that we make "Step 1" in the pg_upgrade
> process be "take a full backup and verify that you're able to restore it
> successfully and without corruption."

I am hesitant to add pg_upgrade-specific nanny language but if we want
to review all upgrade methods and make recommendations, we can do that.
If we need to add more --link-specific warnings, please suggest that. 
Thanks.

> I don't particularly care for how this seems to imply that the Rsync
> method is "the" method to use when --link mode is used with pg_upgrade.

Agreed.  I have added new text in the attached patch to make it clear
that non-rsync is an option and is easier.

> I'd reword the section title to be along these lines:
> 
> If you have streaming replicas or log-shipping standby servers then they
> will also need to be updated.  The simplest way to accomplish this is to
> simply rebuild the replicas from scratch once the primary is back
> online.  Unfortunately, that can take a long time for larger systems as
> the data has to be copied from the primary to each replica in the
> environment.  If --link mode was used with pg_upgrade, the Latest
> checkpoint location matches between the primary and the replica(s) (as
> discussed in Step 8), the rsync utility is available, and the existing
> data directory and new data directory on the replica are able to be in a
> common directory on the same filesystem (as is required on the primary
> for --link mode to be used), then an alternative method may be used to
> update the replicas using rsync which will generally require much less
> time.
> 
> Note that this method will end up needlessly copying across temporary
> files and unlogged tables.  If these make up a large portion of your
> database, then rebuilding the replicas from scratch may be a better
> option.
> 
> With this method, you will not be running pg_upgrade on the standby
> servers, but rather rsync on the primary to sync the replicas to match
> the results of the pg_upgrade on the primary.  Do not start any servers
> yet.  If you did not use link mode, skip the instructions in this
> section and simply recreate the standby servers.
> 
> This method requires that the *old* data directory on the replica be in
> place as rsync will be creating a hard-link tree between the old data
> files on the replica and the new data directory on the replica (as was
> done by pg_upgrade on the primary).

Sorry, I didn't use any of the above text.  It seems to be a step
backward in clarity.

> a. Install the new PostgreSQL binaries on standby servers.
> 
> ...
> 
> b. Make sure the new standby data directories do not exist
> 
> If initdb was run on the replica to create a new data directory, remove
> that new data directory (the rsync will recreate it).  Do *not* remove
> the existing old data directory.

I clarified "new data directory" in the patch.

> c. Install custom shared object files
> 
>  ** I would probably move this up to be step 'b' instead, and make step
>  'b' be step 'c' instead.

Why move it?  The current ordering seems more logical.

> d. Stop standby servers
> 
> ...
> 
> *new*
> e. Verify/re-verify that Latest checkpoint location in pg_controldata
>on the replica matches that of the primary (from before the primary
>was upgraded with pg_upgrade).

I added text in the pg_controldata paragraph to mention which standby
upgrade method is references.  Repeating the pg_controldata check seems
pointless here.

> f. Save configuration files
> 
>   ** this should have a caveat that it's only necessary if the config
>   files are in the data directory.

I clarified "data directory" in the patch.

> g. Run rsync
> 
>   ** I am having a hard time figuring out why --delete makes sense here.
>   There shouldn't be anything in the new data directory, and we don't
>   actually need to delete anything in the old data directory on the
>   replica, so what are we doing suggesting --delete be used?  Strikes me
>   as unnecessairly adding risk, should someone end up doing the wrong
>   command.  Also, again, if I was doing this, I'd absolutely run rsync
>   with --dry-run for starters and review what it is going to do and make
>   sure that's consistent with what I'd expect.

I talked with Stephen about this on IM.  The issue is that if you don't
do --delete, and there are files in the primary that are not in the
standby, they are copied, but files in the standby and not in the
primary are kept.  This could lead to mixed primary/standby log files,
or worse.  Using 

Re: [HACKERS] Pluggable storage

[Alexander Korotkov]

On Thu, Sep 14, 2017 at 8:17 AM, Haribabu Kommi 
wrote:

> Instead of modifying the Bitmap Heap and Sample scan's to avoid referring
> the internal members of the HeapScanDesc, I divided the HeapScanDesc
> into two parts.
>
> 1. StorageScanDesc
> 2. HeapPageScanDesc
>
> The StorageScanDesc contains the minimal information that is required
> outside
> the Storage routine and this must be provided by all storage routines. This
> structure contains minimal information such as relation, snapshot, buffer
> and
> etc.
>
> The HeapPageScanDesc contains other extra information that is required for
> Bitmap Heap and Sample scans to work. This structure contains the
> information
> of blocks, visible offsets and etc. Currently this structure is used only
> in
> Bitmap Heap and Sample scan and it's supported contrib modules, except
> the pgstattuple module. The pgstattuple needs some additional changes.
>
> By adding additional storage API to return HeapPageScanDesc as it required
> by the Bitmap Heap and Sample scan's and this API is called only in these
> two scan's. And also these scan methods are choosen by the planner only
> when the storage routine supports to returning of HeapPageScanDesc API.
> Currently Implemented the planner support only for Bitmap, yet to do it
> for Sample scan.
>
> With the above approach, I removed all the references of HeapScanDesc
> outside the heap. The changes of this approach is available in the
> 0008-Remove-HeapScanDesc-usage-outside-heap.patch
>
> Suggestions/comments with the above approach.
>

For me, that's an interesting idea.  Naturally, the way BitmapHeapScan and
SampleScan work even on very high-level is applicable only for some storage
AMs (i.e. heap-like storage AMs).  For example, index-organized table
wouldn't ever support BitmapHeapScan, because it refers tuples by PK values
not TIDs.  However, in this case, storage AM might have some alternative to
our BitmapHeapScan.  So, index-organized table might have some compressed
representation of ordered PK values set and use it for bulk fetch of PK
index.

Therefore, I think it would be nice to make BitmapHeapScan an
heap-storage-AM-specific scan method while other storage AMs could provide
other storage-AM-specific scan methods.  Probably it would be too much for
this patchset and should be done during one of next work cycles on storage
AM (I'm sure that such huge project as pluggable storage AMs would have
multiple iterations).

Similarly, SampleScans contain storage-AM-specific logic.  For instance,
our SYSTEM sampling method fetches random blocks from heap providing high
performance way to sample heap.  Coming back to the example of
index-organized table, it could provide it's own storage-AM-specific table
sampling methods including sophisticated PK tree traversal with fetching
random small ranges of PK.  Given that tablesample methods are already
pluggable, making them storage-AM-specific would lead to user-visible
changes.  I.e. tablesample method should be created for particular storage
AM or set of storage AMs.  However, I didn't yet figure out what should API
exactly look like...

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

Re: [HACKERS] DROP SUBSCRIPTION hangs if sub is disabled in the same transaction

[Peter Eisentraut]

On 9/12/17 15:51, Tom Lane wrote:
> ISTM the second of those (refuse to drop an in-use subscription) is
> by far the least surprising behavior.  However, I wonder if there aren't
> race conditions involved here.  What if we haven't yet committed a
> DROP SUBSCRIPTION, and some new worker starts up after we look for
> workers?

DROP SUBSCRIPTION takes an AccessExclusiveLock on pg_subscription to
prevent that.

-- 
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] DROP SUBSCRIPTION hangs if sub is disabled in the same transaction

[Peter Eisentraut]

On 9/13/17 07:00, Arseny Sher wrote:
> On the other hand, forbidding to execute disable sub and drop sub in one
> transaction makes it impossible e.g. to drop subscription in trigger

Disabling a subscription before dropping it isn't very useful, is it?
So I'm not sure this is an important use case we need to optimize.  We
just need to prevent it from hanging.

-- 
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] DROP SUBSCRIPTION hangs if sub is disabled in the same transaction

[Peter Eisentraut]

On 9/14/17 08:23, Alvaro Herrera wrote:
> Peter Eisentraut wrote:
>> On 9/13/17 09:56, Alvaro Herrera wrote:
>>> Tom Lane wrote:
 Peter Eisentraut  writes:
>>>
> - Disallow DROP SUBSCRIPTION in a transaction under certain
> circumstances, for example if a transaction has previously manipulated
> the same subscription.
>>>
 ISTM the second of those (refuse to drop an in-use subscription) is
 by far the least surprising behavior.
>>>
>>> +1 for that option.  IIRC this has precedent for other object types such
>>> as tables, where we refuse some action if we have already operated on
>>> the table in the same transaction.
>>
>> What are some examples of such behavior?
> 
> Search for CheckTableNotInUse() callers.

That one uses the relcache refcount, so we can't use that mechanism
here.  I think we need something based on xmin.  The enum code has
something like it, but I don't understand the details.

-- 
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] Clarification in pg10's pgupgrade.html step 10 (upgrading standby servers)

[Andreas Joseph Krogh]

På torsdag 14. september 2017 kl. 20:39:34, skrev Bruce Momjian <
br...@momjian.us >:
On Wed, Sep 13, 2017 at 04:31:09PM +0200, Andreas Joseph Krogh wrote:
 > På onsdag 13. september 2017 kl. 15:26:27, skrev Bruce Momjian <
 > br...@momjian.us>:
 >
 >     On Wed, Sep 13, 2017 at 01:35:17AM +0200, Andreas Joseph Krogh wrote:
 >     [snip]
 >     > I know I'm being a little nitty-gritty here, but if it helps me
 >     understand it
 >     > might help others.
 >
 >     I have applied the attached patch to show examples of using rsync on
 >     PGDATA and tablespaces, documented that rsync is only useful when in
 >     link mode, and explained more clearly how rsync handles links.  You can
 >     see the results here:
 >
 >     http://momjian.us/pgsql_docs/pgupgrade.html
 >
 >     Any more improvements?
 >
 >  
 > Very nice!
 >  
 > For sake of completeness I think an example of running rsync when 
having pg_wal
 > located outside the data directories would be helpful. Especially an example
 > upgrading from 9.6 to 10 because of the name-change of pg_xlog -> pg_wal.

 I think the tablespace example is clear enough to modify for WAL and we
 instruct them right below that example to do WAL.
 
Well, it's not following the exact same structure as there's no 
"version-directory" in pg_xlog, so the "rsync the version-dirs into it's parent 
on the target" isn't what's happening.
 
That's why I think this makes sense to mention for the sake of a complete 
example:
 rsync --archive --delete --hard-links --size-only /vol1/postgres/9.6/pg_xlog 
\ /vol1/postgres/10/pg_wal standby.example.com:/vol1/postgres/10/pg_wal 
 
Thanks.
 
--
 Andreas Joseph Krogh
 

Re: [HACKERS] Clarification in pg10's pgupgrade.html step 10 (upgrading standby servers)

[Bruce Momjian]

On Wed, Sep 13, 2017 at 04:31:09PM +0200, Andreas Joseph Krogh wrote:
> På onsdag 13. september 2017 kl. 15:26:27, skrev Bruce Momjian <
> br...@momjian.us>:
> 
> On Wed, Sep 13, 2017 at 01:35:17AM +0200, Andreas Joseph Krogh wrote:
> [snip]
> > I know I'm being a little nitty-gritty here, but if it helps me
> understand it
> > might help others.
> 
> I have applied the attached patch to show examples of using rsync on
> PGDATA and tablespaces, documented that rsync is only useful when in
> link mode, and explained more clearly how rsync handles links.  You can
> see the results here:
> 
> http://momjian.us/pgsql_docs/pgupgrade.html
> 
> Any more improvements?
> 
>  
> Very nice!
>  
> For sake of completeness I think an example of running rsync when having 
> pg_wal
> located outside the data directories would be helpful. Especially an example
> upgrading from 9.6 to 10 because of the name-change of pg_xlog -> pg_wal.

I think the tablespace example is clear enough to modify for WAL and we
instruct them right below that example to do WAL.

-- 
  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] postgres_fdw super user checks

[Jeff Janes]

On Tue, Sep 12, 2017 at 1:13 AM, Andreas Karlsson  wrote:

> On 07/27/2017 09:45 PM, Jeff Janes wrote:> Here is an updated patch.  This
> version allows you use the password-less
>
>> connection if you either are the super-user directly (which is the
>> existing committed behavior), or if you are using the super-user's mapping
>> because you are querying a super-user-owned view which you have been
>> granted access to.
>>
>
> I have tested the patch and it passes the tests and works, and the code
> looks good (I have a small nitpick below).
>
> The feature seems useful, especially for people who already use views for
> security, so the question is if this is a potential footgun. I am leaning
> towards no since the superuser should be careful when grant access to is
> views anyway.
>
> It would have been nice if there was a more generic way to handle this
> since 1) the security issue is not unique to postgres_fdw and 2) this
> requires you to create a view. But since the patch is simple, an
> improvement in itself and does not prevent any future further improvements
> in this era I see no reason to let perfect be the enemy of good.
>

Thanks for the review.

I think that foreign tables ought to behave as views do, where they run as
the owner rather than the invoker.  No one has talked me out of it, but no
one has supported me on it either.  But I think it is too late to change
that now.  Wrapping it in a view is not hard, but it sure clutters up a
schema.  I don't think this can be made too generic, because each database
has a quite different security model, so the solution will be much
different.

Attached is a new patch which fixes the style issue you mentioned.

Cheers,

Jeff


postgres_fdw_superuser_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] pg_basebackup behavior on non-existent slot

[Magnus Hagander]

On Tue, Sep 12, 2017 at 7:35 PM, Jeff Janes  wrote:

> On Wed, Sep 6, 2017 at 2:50 AM, Alvaro Herrera 
> wrote:
>
>> Magnus Hagander wrote:
>> > On Mon, Sep 4, 2017 at 3:21 PM, Jeff Janes 
>> wrote:
>>
>> > > Should the parent process of pg_basebackup be made to respond to
>> SIGCHLD?
>> > > Or call waitpid(bgchild, , WNOHANG) in some strategic loop?
>> >
>> > I think it's ok to just call waitpid() -- we don't need to react super
>> > quickly, but we should react.
>>
>> Hmm, not sure about that ... in the normal case (slotname is correct)
>> you'd be doing thousands of useless waitpid() system calls during the
>> whole operation, no?  I think it'd be better to have a SIGCHLD handler
>> that sets a flag (just once), which can be quickly checked without
>> accessing kernel space.
>>
>
> If we don't want polling by waitpid, then my next thought would be to move
> the data copy into another process, then have the main process do nothing
> but wait for the first child to exit.  If the first to exit is the WAL
> receiver, then we must have an error and the data receiver can be killed.
> I don't know how to translate that to Windows, however.
>
>
Well, we could do something similar -- run the main process and the
streamer in separate threads on windows and have a main thread wait on
both. The main thread would have to be in charge of cleanup as well of
course. But I think that's likely going to be more complicated than using
non blocking libpq APIs.


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

Re: [HACKERS] [PATCH] Call RelationDropStorage() for broader range of object drops.

[Andres Freund]

On 2017-09-14 12:05:37 +0200, Alvaro Herrera wrote:
> This sounds terrible.

Welcome to the life of writing extensions.


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

[Jesper Pedersen]

On 09/14/2017 01:52 PM, Robert Haas wrote:

On Thu, Sep 14, 2017 at 1:07 PM, Jesper Pedersen
 wrote:

Yeah, it would be nice to have a syntax like

) PARTITION BY HASH (col) WITH (AUTO_CREATE = 64);

But then there also needs to be a way to create the 64 associated indexes
too for everything to be easy.


Well, for that, there's this proposal:

http://postgr.es/m/c8fe4f6b-ff46-aae0-89e3-e936a35f0...@postgrespro.ru

As several people have right pointed out, there's a lot of work to be
done on partitioning it to get it to where we want it to be.  Even in
v10, it's got significant benefits, such as much faster bulk-loading,
but I don't hear anybody disputing the notion that a lot more work is
needed.  The good news is that a lot of that work is already in
progress; the bad news is that a lot of that work is not done yet.

But I think that's OK.  We can't solve every problem at once, and I
think we're moving things along here at a reasonably brisk pace.  That
didn't stop me from complaining bitterly to someone just yesterday
that we aren't moving faster still, but unfortunately EnterpriseDB has
only been able to get 12 developers to do any work at all on
partitioning this release cycle, and 3 of those have so far helped
only with review and benchmarking.  It's a pity we can't do more, but
considering how many community projects are 1-person efforts I think
it's pretty good.

To be clear, I know you're not (or at least I assume you're not)
trying to beat me up about this, just raising a concern, and I'm not
trying to beat you up either, just let you know that it is definitely
on the radar screen but not there yet.



Definitely not a complain about the work being done.

I think the scope of Amul's and others work on hash partition support is 
where it needs to be. Improvements can always follow in future release.


My point was that is easy to script the definition of the partitions and 
their associated indexes, so it is more important to focus on the core 
functionality with the developer / review resources available.


However, it is a little bit difficult to follow the dependencies between 
different partition patches, so I may not always provide sane feedback, 
as seen in [1].


[1] 
https://www.postgresql.org/message-id/579077fd-8f07-aff7-39bc-b92c855cdb70%40redhat.com


Best regards,
 Jesper


--
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 Thu, Sep 14, 2017 at 1:07 PM, Jesper Pedersen
 wrote:
> Yeah, it would be nice to have a syntax like
>
> ) PARTITION BY HASH (col) WITH (AUTO_CREATE = 64);
>
> But then there also needs to be a way to create the 64 associated indexes
> too for everything to be easy.

Well, for that, there's this proposal:

http://postgr.es/m/c8fe4f6b-ff46-aae0-89e3-e936a35f0...@postgrespro.ru

As several people have right pointed out, there's a lot of work to be
done on partitioning it to get it to where we want it to be.  Even in
v10, it's got significant benefits, such as much faster bulk-loading,
but I don't hear anybody disputing the notion that a lot more work is
needed.  The good news is that a lot of that work is already in
progress; the bad news is that a lot of that work is not done yet.

But I think that's OK.  We can't solve every problem at once, and I
think we're moving things along here at a reasonably brisk pace.  That
didn't stop me from complaining bitterly to someone just yesterday
that we aren't moving faster still, but unfortunately EnterpriseDB has
only been able to get 12 developers to do any work at all on
partitioning this release cycle, and 3 of those have so far helped
only with review and benchmarking.  It's a pity we can't do more, but
considering how many community projects are 1-person efforts I think
it's pretty good.

To be clear, I know you're not (or at least I assume you're not)
trying to beat me up about this, just raising a concern, and I'm not
trying to beat you up either, just let you know that it is definitely
on the radar screen but not there yet.

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

[Jesper Pedersen]

On 09/14/2017 12:56 PM, Robert Haas wrote:

On Thu, Sep 14, 2017 at 12:54 PM, David Fetter  wrote:

Should we be pointing the gun away from people's feet by making hash
partitions that cover the space automagically when the partitioning
scheme[1] is specified?  In other words, do we have a good reason to have
only some of the hash partitions so defined by default?


Sure, we can add some convenience syntax for that, but I'd like to get
the basic stuff working before doing that kind of polishing.

If nothing else, I assume Keith Fiske's pg_partman will provide a way
to magically DTRT about an hour after this goes in.  But probably we
can do better in core easily enough.



Yeah, it would be nice to have a syntax like

) PARTITION BY HASH (col) WITH (AUTO_CREATE = 64);

But then there also needs to be a way to create the 64 associated 
indexes too for everything to be easy.


Best regards,
 Jesper


--
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: Use MINVALUE/MAXVALUE instead of UNBOUNDED for range partition b

[David G. Johnston]

On Thu, Sep 14, 2017 at 8:41 AM, Stephen Frost  wrote:

> Robert, all,
>
> * Robert Haas (robertmh...@gmail.com) wrote:
>
> > >
> > > I vote for rejecting it.  DDL compatibility is less valuable than other
> > > compatibility.  The hypothetical affected application can change its
> DDL to
> > > placate PostgreSQL and use that modified DDL for all other databases,
> too.
> >
> > OK.  Any other votes?
>
> I haven't been as close to this as others, so perhaps my vote is only
> 0.5 on this specific case, but that's my feeling on it.
>

I think we are being consistent as a project by enforcing strictness of
input in this situation so I'll toss my +0.5/+1​ here as well.

David J.

Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

[Peter Eisentraut]

On 9/12/17 19:03, Michael Paquier wrote:
> Once channel binding is involved though.. This needs to be extended
> and this needs careful thoughts:
> * "scram-sha-256" means that the version without channel binding is
> accepted. "!scram-sha-256" means that scram without channel binding is
> refused.
> * "scram-sha-256-plus" means that all channel bindings are accepted.
> "!scram-sha-256-plus" means that no channel binding are accepted.
> After that there is some filtering per channel binding name. Do we
> want a separate parameter or just filter with longer names like
> "scram-sha-256-plus-tls-unique" and
> "scram-sha-256-plus-tls-server-end-point"? The last one gets
> particularly long, this does not help users with typos :)

Second thoughts, to make things simpler.  All we need for channel
binding is a connection flag that says "I require channel binding".  It
could be modeled after the sslmode parameter, e.g., cbind=disable (maybe
for debugging), cbind=prefer (default), cbind=require.  If you specify
"require", then libpq would refuse to proceed unless scram-sha2-256-plus
(or future similar mechanisms) was offered for authentication.

We don't even need a parameter that specifies which channel binding type
to use.  If libpq implements tls-unique, it should always use that.  We
might need a flag for testing other types, but that should not be an
in-the-user's-face option.

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

[Robert Haas]

On Thu, Sep 14, 2017 at 12:54 PM, David Fetter  wrote:
> Should we be pointing the gun away from people's feet by making hash
> partitions that cover the space automagically when the partitioning
> scheme[1] is specified?  In other words, do we have a good reason to have
> only some of the hash partitions so defined by default?

Sure, we can add some convenience syntax for that, but I'd like to get
the basic stuff working before doing that kind of polishing.

If nothing else, I assume Keith Fiske's pg_partman will provide a way
to magically DTRT about an hour after this goes in.  But probably we
can do better in core easily enough.

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

[David Fetter]

On Mon, Sep 11, 2017 at 07:43:29AM -0400, Robert Haas wrote:
> On Mon, Sep 11, 2017 at 4:17 AM, Ashutosh Bapat
>  wrote:
> >> Rebased 0002 against this commit & renamed to 0001, PFA.
> >
> > Given that we have default partition support now, I am wondering
> > whether hash partitioned tables also should have default
> > partitions.  The way we have structured hash partitioning syntax,
> > there can be "holes" in partitions. Default partition would help
> > plug those holes.
> 
> Yeah, I was thinking about that, too.  On the one hand, it seems
> like it's solving the problem the wrong way: if you've set up hash
> partitioning properly, you shouldn't have any holes.

Should we be pointing the gun away from people's feet by making hash
partitions that cover the space automagically when the partitioning
scheme[1] is specified?  In other words, do we have a good reason to have
only some of the hash partitions so defined by default?

Best,
David.

[1] For now, that's just the modulus, but the PoC included specifying
hashing functions, so I assume other ways to specify the partitioning
scheme could eventually be proposed.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter  XMPP: david(dot)fetter(at)gmail(dot)com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate


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

[Jesper Pedersen]

Hi,

On 09/14/2017 12:05 PM, Robert Haas wrote:

On Thu, Sep 14, 2017 at 11:39 AM, Jesper Pedersen
 wrote:

When I do

CREATE TABLE mytab (
   a integer NOT NULL,
   b integer NOT NULL,
   c integer,
   d integer
) PARTITION BY HASH (b);

and create 64 partitions;

CREATE TABLE mytab_p00 PARTITION OF mytab FOR VALUES WITH (MODULUS 64,
REMAINDER 0);
...
CREATE TABLE mytab_p63 PARTITION OF mytab FOR VALUES WITH (MODULUS 64,
REMAINDER 63);

and associated indexes

CREATE INDEX idx_p00 ON mytab_p00 USING btree (b, a);
...
CREATE INDEX idx_p63 ON mytab_p63 USING btree (b, a);

Populate the database, and do ANALYZE.

Given

EXPLAIN (ANALYZE, VERBOSE, BUFFERS ON) SELECT a, b, c, d FROM mytab WHERE b
= 42

gives

Append
   -> Index Scan using idx_p00 (cost rows=7) (actual rows=0)
   ...
   -> Index Scan using idx_p63 (cost rows=7) (actual rows=0)

E.g. all partitions are being scanned. Of course one partition will contain
the rows I'm looking for.


Yeah, we need Amit Langote's work in
http://postgr.es/m/098b9c71-1915-1a2a-8d52-1a7a50ce7...@lab.ntt.co.jp
to land and this patch to be adapted to make use of it.  I think
that's the major thing still standing in the way of this. Concerns
were also raised about not having a way to see the hash function, but
we fixed that in 81c5e46c490e2426db243eada186995da5bb0ba7 and
hopefully this patch has been updated to use a seed (I haven't looked
yet).  And there was a concern about hash functions not being
portable, but the conclusion of that was basically that most people
think --load-via-partition-root will be a satisfactory workaround for
cases where that becomes a problem (cf. commit
23d7680d04b958de327be96ffdde8f024140d50e).  So this is the major
remaining issue that I know about.



Thanks for the information, Robert !

Best regards,
 Jesper


--
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] Log LDAP "diagnostic messages"?

[Robert Haas]

On Thu, Sep 14, 2017 at 10:21 AM, Alvaro Herrera
 wrote:
> BTW I added --with-ldap and --with-pam to the configure line for the
> reports in coverage.postgresql.org and the % covered in auth.c went from
> 24% to 18.9% (from very bad to terribly sad).

Improved code coverage is becoming a fad.

-- 
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] expanding inheritance in partition bound order

[Robert Haas]

On Thu, Sep 14, 2017 at 7:56 AM, Amit Khandekar  wrote:
> On 14 September 2017 at 06:43, Amit Langote
>> langote_amit...@lab.ntt.co.jp> wrote:
>> Attached updated patch.
>
> @@ -1222,151 +1209,130 @@ PartitionDispatch *
>  RelationGetPartitionDispatchInfo(Relation rel,
>  int
> *num_parted, List **leaf_part_oids)
>  {
> +   List   *pdlist;
> PartitionDispatchData **pd;
>
> +   get_partition_dispatch_recurse(rel, NULL, , leaf_part_oids);
>
> Above, pdlist is passed uninitialized. And then inside
> get_partition_dispatch_recurse(), it is used here :
> *pds = lappend(*pds, pd);
>
> 
>
> pg_indent says more alignments needed. For e.g. gettext_noop() call
> below needs to be aligned:
> pd->tupmap = convert_tuples_by_name(RelationGetDescr(parent),
> tupdesc,
> gettext_noop("could not convert row type"));
>
> 
>
> Other than that, the patch looks good to me. I verified that the leaf
> oids are ordered exaclty in the order of the UPDATE subplans output.

Committed with fixes for those issues and a few other cosmetic changes.

-- 
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] Parallel Append implementation

[Robert Haas]

On Mon, Sep 11, 2017 at 9:25 AM, Amit Kapila  wrote:
> I think the patch stores only non-partial paths in decreasing order,
> what if partial paths having more costs follows those paths?

The general picture here is that we don't want the leader to get stuck
inside some long-running operation because then it won't be available
to read tuples from the workers.  On the other hand, we don't want to
just have the leader do no work because that might be slow.  And in
most cast cases, the leader will be the first participant to arrive at
the Append node, because of the worker startup time.  So the idea is
that the workers should pick expensive things first, and the leader
should pick cheap things first.  This may not always work out
perfectly and certainly the details of the algorithm may need some
refinement, but I think the basic concept is good.  Of course, that
may be because I proposed it...

Note that there's a big difference between the leader picking a
partial path and the leader picking a non-partial path.  If the leader
picks a partial path, it isn't committed to executing that path to
completion.  Other workers can help.  If the leader picks a
non-partial path, though, the workers are locked out of that path,
because a single process must run it all the way through.  So the
leader should avoid choosing a non-partial path unless there are no
partial paths remaining.

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

[Robert Haas]

On Thu, Sep 14, 2017 at 11:39 AM, Jesper Pedersen
 wrote:
> When I do
>
> CREATE TABLE mytab (
>   a integer NOT NULL,
>   b integer NOT NULL,
>   c integer,
>   d integer
> ) PARTITION BY HASH (b);
>
> and create 64 partitions;
>
> CREATE TABLE mytab_p00 PARTITION OF mytab FOR VALUES WITH (MODULUS 64,
> REMAINDER 0);
> ...
> CREATE TABLE mytab_p63 PARTITION OF mytab FOR VALUES WITH (MODULUS 64,
> REMAINDER 63);
>
> and associated indexes
>
> CREATE INDEX idx_p00 ON mytab_p00 USING btree (b, a);
> ...
> CREATE INDEX idx_p63 ON mytab_p63 USING btree (b, a);
>
> Populate the database, and do ANALYZE.
>
> Given
>
> EXPLAIN (ANALYZE, VERBOSE, BUFFERS ON) SELECT a, b, c, d FROM mytab WHERE b
> = 42
>
> gives
>
> Append
>   -> Index Scan using idx_p00 (cost rows=7) (actual rows=0)
>   ...
>   -> Index Scan using idx_p63 (cost rows=7) (actual rows=0)
>
> E.g. all partitions are being scanned. Of course one partition will contain
> the rows I'm looking for.

Yeah, we need Amit Langote's work in
http://postgr.es/m/098b9c71-1915-1a2a-8d52-1a7a50ce7...@lab.ntt.co.jp
to land and this patch to be adapted to make use of it.  I think
that's the major thing still standing in the way of this. Concerns
were also raised about not having a way to see the hash function, but
we fixed that in 81c5e46c490e2426db243eada186995da5bb0ba7 and
hopefully this patch has been updated to use a seed (I haven't looked
yet).  And there was a concern about hash functions not being
portable, but the conclusion of that was basically that most people
think --load-via-partition-root will be a satisfactory workaround for
cases where that becomes a problem (cf. commit
23d7680d04b958de327be96ffdde8f024140d50e).  So this is the major
remaining issue that I know about.

-- 
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] Optimise default partition scanning while adding new partition

[Robert Haas]

On Thu, Sep 14, 2017 at 4:03 AM, Jeevan Ladhe
 wrote:
> Thanks Amit for reviewing.
>> Patch looks fine to me.  By the way, why don't we just say "Can we skip
>> scanning part_rel?" in the comment before the newly added call to
>> PartConstraintImpliedByRelConstraint()?  We don't need to repeat the
>> explanation of what it does at the every place we call it.
>
> I have changed the comment.
> PFA.

I'm probably missing something stupid, but why does this happen?

 ALTER TABLE list_parted2 ATTACH PARTITION part_7 FOR VALUES IN (7);
-INFO:  partition constraint for table "list_parted2_def" is implied
by existing constraints
 ERROR:  partition constraint is violated by some row

Based on the regression test changes made up higher in the file, I'd
expect that line to be replaced by two lines, one for
list_parted2_def_p1 and another for list_parted2_def_p2, because at
this point, list_parted2_def is a partitioned table with those two
children, and they seem to have appropriate constraints.

list_parted2_def_p1 has
 Check constraints:
 "check_a" CHECK (a = ANY (ARRAY[21, 22]))

list_parted2_def_p2 has
 Check constraints:
 "check_a" CHECK (a = ANY (ARRAY[31, 32]))

Well, if a is 21 or 22 for the first, or 31 or 32 for the second, then
it's not 7.  Or so I would think.

-- 
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] Surjective functional indexes

[Simon Riggs]

On 14 September 2017 at 16:37, Konstantin Knizhnik
 wrote:
>
>
> On 14.09.2017 13:19, Simon Riggs wrote:

>> This works by looking at overall stats, and only looks at the overall
>> HOT %, so its too heavyweight and coarse.
>>
>> I suggested storing stat info on the relcache and was expecting you
>> would look at how often the expression evaluates to new == old. If we
>> evaluate new against old many times, then if the success rate is low
>> we should stop attempting the comparison. (<10%?)
>>
>> Another idea:
>> If we don't make a check when we should have done then we will get a
>> non-HOT update, so we waste time extra time difference between a HOT
>> and non-HOT update. If we check and fail we waste time take to perform
>> check. So the question is how expensive the check is against how
>> expensive a non-HOT update is. Could we simply say we don't bother to
>> check functions that have a cost higher than 1? So if the user
>> doesn't want to perform the check they can just increase the cost of
>> the function above the check threshold?
>>
> Attached pleased find one more patch which calculates hot update check hit
> rate more precisely: I have to extended PgStat_StatTabEntry with two new
> fields:
> hot_update_hits and hot_update_misses.

It's not going to work, as already mentioned above. Those stats are at
table level and very little to do with this particular index.

But you've not commented on the design I mention that can work: index relcache.

> Concerning your idea to check cost of index function: it certainly makes
> sense.
> The only problems: I do not understand now how to calculate this cost.
> It can be easily calculated by optimizer when it is building query execution
> plan.
> But inside BuildIndexInfo I have just reference to Relation and have no idea
> how
> I can propagate here information about index expression cost from optimizer.

We could copy at create index, if we took that route. Or we can look
up the cost for the index expression and cache it.


Anyway, this is just jumping around because we still have a parameter
and the idea was to remove the parameter entirely by autotuning, which
I think is both useful and possible, just as HOT itself is autotuned.

-- 
Simon Riggshttp://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: Use MINVALUE/MAXVALUE instead of UNBOUNDED for range partition b

[Stephen Frost]

Robert, all,

* Robert Haas (robertmh...@gmail.com) wrote:
> On Wed, Sep 13, 2017 at 10:49 PM, Noah Misch  wrote:
> >> > Both Oracle and MySQL allow finite values after MAXVALUE (usually
> >> > listed as "0" in code examples, e.g. see [1]). Oracle explicitly
> >> > documents the fact that values after MAXVALUE are irrelevant in [1].
> >> > I'm not sure if MySQL explicitly documents that, but it does behave
> >> > the same.
> >> >
> >> > Also, both Oracle and MySQL store what the user entered (they do not
> >> > canonicalise), as can be seen by looking at ALL_TAB_PARTITIONS in
> >> > Oracle, or "show create table" in MySQL.
> >>
> >> OK, thanks.  I still don't really like allowing this, but I can see
> >> that compatibility with other systems has some value here, and if
> >> nobody else is rejecting these cases, maybe we shouldn't either.  So
> >> I'll hold my nose and change my vote to canonicalizing rather than
> >> rejecting outright.
> >
> > I vote for rejecting it.  DDL compatibility is less valuable than other
> > compatibility.  The hypothetical affected application can change its DDL to
> > placate PostgreSQL and use that modified DDL for all other databases, too.
> 
> OK.  Any other votes?

I think I side with Noah on this one.  I agree that DDL compatibility is
less valuable than other compatibility.  I had also been thinking that
perhaps it would be good to still be compatible for the sake of DBAs not
being confused if they got an error, but this seems straight-forward
enough that it wouldn't take much for a DBA who is building such
partitions to understand their mistake and to fix it.

I haven't been as close to this as others, so perhaps my vote is only
0.5 on this specific case, but that's my feeling on it.

Also, I don't think we should be adding compatibility for the sake of
compatibility alone.  If there's more than one way to do something and
they're all correct and reasonable, then I could see us choosing the
route that matches what others in the industry do, but I don't see
simply ignoring user input in this specific case as really correct and
therefore it's better to reject it.

Basically, for my 2c, the reason Oracle does this is something more of a
historical artifact than because it was deemed sensible, and everyone
else just followed suit, but I don't believe we need to or should.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] [POC] hash partitioning

[Jesper Pedersen]

Hi Amul,

On 09/14/2017 04:58 AM, amul sul wrote:

On Wed, Sep 13, 2017 at 7:43 PM, Jesper Pedersen  Index Scan using idx_p00 (cost rows=7) (actual rows=0)
  ...
  -> Index Scan using idx_p63 (cost rows=7) (actual rows=0)

E.g. all partitions are being scanned. Of course one partition will 
contain the rows I'm looking for.


Best regards,
 Jesper


--
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] Surjective functional indexes

[Konstantin Knizhnik]

On 14.09.2017 13:19, Simon Riggs wrote:

On 14 September 2017 at 10:42, Konstantin Knizhnik
 wrote:


On 13.09.2017 14:00, Simon Riggs wrote:

On 13 September 2017 at 11:30, Konstantin Knizhnik
 wrote:


The only reason of all this discussion about terms is that I need to
choose
name for correspondent index option.
Simon think that we do not need this option at all. In this case we
should
not worry about right term.
  From my point of view, "projection" is quite clear notion and not only
for
mathematics. It is also widely used in IT and especially in DBMSes.

If we do have an option it won't be using fancy mathematical
terminology at all, it would be described in terms of its function,
e.g. recheck_on_update

Yes, I'd rather not have an option at all, just some simple code with
useful effect, like we have in many other places.


Attached please find new version of projection functional index optimization
patch.
I have implemented very simple autotune strategy: now I use table statistic
to compare total number of updates with number of hot updates.
If fraction of hot updates is relatively small, then there is no sense to
spend time performing extra evaluation of index expression and comparing its
old and new values.
Right now the formula is the following:

#define MIN_UPDATES_THRESHOLD 10
#define HOT_RATIO_THRESHOLD   2

 if (stat->tuples_updated > MIN_UPDATES_THRESHOLD
 && stat->tuples_updated >
stat->tuples_hot_updated*HOT_RATIO_THRESHOLD)
 {
 /* If percent of hot updates is small, then disable projection
index function
  * optimization to eliminate overhead of extra index expression
evaluations.
  */
 ii->ii_Projection = false;
 }

This threshold values are pulled out of a hat: I am not sure if this
heuristic is right.
I will be please to get feedback if such approach to autotune is promising.

Hmm, not really, but thanks for trying.

This works by looking at overall stats, and only looks at the overall
HOT %, so its too heavyweight and coarse.

I suggested storing stat info on the relcache and was expecting you
would look at how often the expression evaluates to new == old. If we
evaluate new against old many times, then if the success rate is low
we should stop attempting the comparison. (<10%?)

Another idea:
If we don't make a check when we should have done then we will get a
non-HOT update, so we waste time extra time difference between a HOT
and non-HOT update. If we check and fail we waste time take to perform
check. So the question is how expensive the check is against how
expensive a non-HOT update is. Could we simply say we don't bother to
check functions that have a cost higher than 1? So if the user
doesn't want to perform the check they can just increase the cost of
the function above the check threshold?

Attached pleased find one more patch which calculates hot update check 
hit rate more precisely: I have to extended PgStat_StatTabEntry with two 
new fields:

hot_update_hits and hot_update_misses.

Concerning your idea to check cost of index function: it certainly makes 
sense.

The only problems: I do not understand now how to calculate this cost.
It can be easily calculated by optimizer when it is building query 
execution plan.
But inside BuildIndexInfo I have just reference to Relation and have no 
idea how

I can propagate here information about index expression cost from optimizer.


--
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

diff --git a/doc/src/sgml/ref/create_index.sgml b/doc/src/sgml/ref/create_index.sgml
index 83ee7d3..52189ac 100644
--- a/doc/src/sgml/ref/create_index.sgml
+++ b/doc/src/sgml/ref/create_index.sgml
@@ -294,8 +294,33 @@ CREATE [ UNIQUE ] INDEX [ CONCURRENTLY ] [ [ IF NOT EXISTS ] 
 The optional WITH clause specifies storage
 parameters for the index.  Each index method has its own set of allowed
-storage parameters.  The B-tree, hash, GiST and SP-GiST index methods all
-accept this parameter:
+storage parameters. All indexes accept the following parameter:
+   
+
+   
+   
+projection
+
+ 
+   Functional index is based on on projection function: function which extract subset of its argument.
+   In mathematic such functions are called non-injective. For injective function if any attribute used in the indexed
+   expression is changed, then value of index expression is also changed. So to check that index is affected by the
+   update, it is enough to check the set of changed fields. By default this parameters is assigned true value and function is considered
+   as non-injective.
+   In this case change of any of indexed key doesn't mean that value of the function is changed. For example, for
+   the expression expression(bookinfo-'isbn') defined
+   for column of JSON type is changed only when 

Re: [HACKERS] Is it time to kill support for very old servers?

[Robert Haas]

On Wed, Sep 13, 2017 at 11:39 PM, Tom Lane  wrote:
>>> One small problem with cutting libpq's V2 support is that the server's
>>> report_fork_failure_to_client() function still sends a V2-style message.
>
>> We should really fix that so it reports the error as a v3 message,
>> independent of ripping out libpq-fe support for v2.
>
> It might be reasonable to do that, but libpq would have to be prepared
> for the other case for many years to come :-(

Well, let's get that much done anyway.  I'm not 100% sure whether the
time has come to kill v2 with fire, but doing the things that have
been done first has got to be a good idea either way.

-- 
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] A bug in mapping attributes in ATExecAttachPartition()

[Robert Haas]

On Thu, Sep 14, 2017 at 12:59 AM, Amit Langote
 wrote:
> Since Jeevan Ladhe mentioned this patch [1] earlier this week, sending the
> rebased patches here for consideration.  Actually there are only 2 patches
> now, because 0002 above is rendered unnecessary by ecfe59e50fb [2].

Committed 0001 and back-patched to v10.

Your 0002 and the patch from Jeevan Ladhe to which you refer seem to
be covering closely related subjects.  When I apply either patch by
itself, the regression tests pass; when I apply both together, they
fail.  Could you and Jeevan sort that out?

-- 
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] Create replication slot in pg_basebackup if requested and not yet present

[Peter Eisentraut]

On 9/12/17 16:39, Michael Banck wrote:
> We could split up the logic here and create the optional physical
> replication slot in the main connection and the temporary one in the WAL
> streamer connection, but this would keep any fragility around for
> (likely more frequently used) temporary replication slots. It would make
> the patch much smaller though if I revert touching temporary slots at
> all.

That's what I was thinking.

But:

If the race condition concern that Jeff was describing is indeed
correct, then the current use of temporary replication slots would be
equally affected.  So I think either we already have a problem, or we
don't and then this patch wouldn't introduce a new one.

I don't know the details of this well enough.

Thoughts from others?

-- 
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] SCRAM in the PG 10 release notes

[Alvaro Hernandez]

On 14/09/17 18:06, Dave Cramer wrote:


On 14 September 2017 at 02:21, Alvaro Hernandez > wrote:




On 14/09/17 08:57, Heikki Linnakangas wrote:

On 09/12/2017 04:09 AM, Noah Misch wrote:

On Wed, May 10, 2017 at 10:50:51PM -0400, Bruce Momjian wrote:

On Mon, May  1, 2017 at 08:12:51AM -0400, Robert Haas
wrote:

On Tue, Apr 25, 2017 at 10:16 PM, Bruce Momjian
> wrote:

Well, we could add "MD5 users are encouraged
to switch to
SCRAM-SHA-256".  Now whether we want to list
this as something on the
SCRAM-SHA-256 description, or mention it as an
incompatibility, or
under Migration.  I am not clear that MD5 is
in such terrible shape that
this is warranted.


I think it's warranted.  The continuing use of MD5
has been a headache
for some EnterpriseDB customers who have
compliance requirements which
they must meet.  It's not that they themselves
necessarily know or
care whether MD5 is secure, although in some cases
they do; it's that
if they use it, they will be breaking laws or
regulations to which
their business or agency is subject.  I imagine
customers of other
PostgreSQL companies have similar issues.  But
leaving that aside, the
advantage of SCRAM isn't merely that it uses a
better algorithm to
hash the password.  It has other advantages also,
like not being
vulnerable to replay attacks.  If you're doing
password
authentication, you should really be using SCRAM,
and encouraging
people to move to SCRAM after upgrading is a good
idea.

That having been said, SCRAM is a wire protocol
break.  You will not
be able to upgrade to SCRAM unless and until the
drivers you use to
connect to the database add support for it. The
only such driver
that's part of libpq; other drivers that have
reimplemented the
PostgreSQL wire protocol will have to be updated
with SCRAM support
before it will be possible to use SCRAM with those
drivers. I think
this should be mentioned in the release notes,
too.  I also think it
would be great if somebody would put together a
wiki page listing all
the popular drivers and (1) whether they use libpq
or reimplement the
wire protocol, and (2) if the latter, the status
of any efforts to
implement SCRAM, and (3) if those efforts have
been completed, the
version from which they support SCRAM.  Then, I
think we should reach
out to all of the maintainers of those driver
authors who aren't
moving to support SCRAM and encourage them to do so.


I have added this as an open item because we will have
to wait to see
where we are with driver support as the release gets
closer.


With the release near, I'm promoting this to the regular
open issues section.


Thanks.

I updated the list of drivers on the wiki
(https://wiki.postgresql.org/wiki/List_of_drivers
), adding a
column for whether the driver supports SCRAM authentication.
Currently, the only non-libpq driver that has implemented
SCRAM is the JDBC driver. I submitted a patch for the Go
driver, but it hasn't been committed yet.


On the JDBC driver, strictly speaking, code has not been
released yet. It is scheduled for v 42.2.0, and maybe the wiki
should also mention from what version of the driver it is
supported (I guess for all cases, unless their versioning would be
synced with PostgreSQL's).


We won't by syncing our version numbers with Postgres


Of course, I wanted to mean with that for other drivers that are 
not JDBC 

[HACKERS] Re: [COMMITTERS] pgsql: Use MINVALUE/MAXVALUE instead of UNBOUNDED for range partition b

[Robert Haas]

On Wed, Sep 13, 2017 at 10:49 PM, Noah Misch  wrote:
>> > Both Oracle and MySQL allow finite values after MAXVALUE (usually
>> > listed as "0" in code examples, e.g. see [1]). Oracle explicitly
>> > documents the fact that values after MAXVALUE are irrelevant in [1].
>> > I'm not sure if MySQL explicitly documents that, but it does behave
>> > the same.
>> >
>> > Also, both Oracle and MySQL store what the user entered (they do not
>> > canonicalise), as can be seen by looking at ALL_TAB_PARTITIONS in
>> > Oracle, or "show create table" in MySQL.
>>
>> OK, thanks.  I still don't really like allowing this, but I can see
>> that compatibility with other systems has some value here, and if
>> nobody else is rejecting these cases, maybe we shouldn't either.  So
>> I'll hold my nose and change my vote to canonicalizing rather than
>> rejecting outright.
>
> I vote for rejecting it.  DDL compatibility is less valuable than other
> compatibility.  The hypothetical affected application can change its DDL to
> placate PostgreSQL and use that modified DDL for all other databases, too.

OK.  Any other votes?

-- 
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] Partition-wise join for join between (declaratively) partitioned tables

[Robert Haas]

On Wed, Sep 13, 2017 at 10:57 PM, Amit Langote
 wrote:
> I very much like pcinfo-for-subquery.patch, although I'm not sure if we
> need to create PartitionedChildRelInfo for the sub-query parent RTE as the
> patch teaches add_paths_to_append_rel() to do.  ISTM, nested UNION ALL
> subqueries are flattened way before we get to add_paths_to_append_rel();
> if it could not be flattened, there wouldn't be a call to
> add_paths_to_append_rel() in the first place, because no AppendRelInfos
> would be generated.  See what happens when is_simple_union_all_recurse()
> returns false to flatten_simple_union_all() -- no AppendRelInfos will be
> generated and added to root->append_rel_list in that case.
>
> IOW, there won't be nested AppendRelInfos for nested UNION ALL sub-queries
> like we're setting out to build for multi-level partitioned tables.
>
> So, as things stand today, there can at most be one recursive call of
> add_path_to_append_rel() for a sub-query parent RTE, that is, if its child
> sub-queries contain partitioned tables, but not more.  The other patch
> (multi-level expansion of partitioned tables) will change that, but even
> then we won't need sub-query's own PartitioendChildRelInfo.

OK, let's assume you're correct unless some contrary evidence emerges.
Committed without that part; thanks for the review.

-- 
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] SCRAM in the PG 10 release notes

[Dave Cramer]

On 14 September 2017 at 02:21, Alvaro Hernandez  wrote:

>
>
> On 14/09/17 08:57, Heikki Linnakangas wrote:
>
>> On 09/12/2017 04:09 AM, Noah Misch wrote:
>>
>>> On Wed, May 10, 2017 at 10:50:51PM -0400, Bruce Momjian wrote:
>>>
 On Mon, May  1, 2017 at 08:12:51AM -0400, Robert Haas wrote:

> On Tue, Apr 25, 2017 at 10:16 PM, Bruce Momjian 
> wrote:
>
>> Well, we could add "MD5 users are encouraged to switch to
>> SCRAM-SHA-256".  Now whether we want to list this as something on the
>> SCRAM-SHA-256 description, or mention it as an incompatibility, or
>> under Migration.  I am not clear that MD5 is in such terrible shape
>> that
>> this is warranted.
>>
>
> I think it's warranted.  The continuing use of MD5 has been a headache
> for some EnterpriseDB customers who have compliance requirements which
> they must meet.  It's not that they themselves necessarily know or
> care whether MD5 is secure, although in some cases they do; it's that
> if they use it, they will be breaking laws or regulations to which
> their business or agency is subject.  I imagine customers of other
> PostgreSQL companies have similar issues.  But leaving that aside, the
> advantage of SCRAM isn't merely that it uses a better algorithm to
> hash the password.  It has other advantages also, like not being
> vulnerable to replay attacks.  If you're doing password
> authentication, you should really be using SCRAM, and encouraging
> people to move to SCRAM after upgrading is a good idea.
>
> That having been said, SCRAM is a wire protocol break.  You will not
> be able to upgrade to SCRAM unless and until the drivers you use to
> connect to the database add support for it.  The only such driver
> that's part of libpq; other drivers that have reimplemented the
> PostgreSQL wire protocol will have to be updated with SCRAM support
> before it will be possible to use SCRAM with those drivers. I think
> this should be mentioned in the release notes, too.  I also think it
> would be great if somebody would put together a wiki page listing all
> the popular drivers and (1) whether they use libpq or reimplement the
> wire protocol, and (2) if the latter, the status of any efforts to
> implement SCRAM, and (3) if those efforts have been completed, the
> version from which they support SCRAM.  Then, I think we should reach
> out to all of the maintainers of those driver authors who aren't
> moving to support SCRAM and encourage them to do so.
>

 I have added this as an open item because we will have to wait to see
 where we are with driver support as the release gets closer.

>>>
>>> With the release near, I'm promoting this to the regular open issues
>>> section.
>>>
>>
>> Thanks.
>>
>> I updated the list of drivers on the wiki (https://wiki.postgresql.org/w
>> iki/List_of_drivers), adding a column for whether the driver supports
>> SCRAM authentication. Currently, the only non-libpq driver that has
>> implemented SCRAM is the JDBC driver. I submitted a patch for the Go
>> driver, but it hasn't been committed yet.
>>
>
> On the JDBC driver, strictly speaking, code has not been released yet.
> It is scheduled for v 42.2.0, and maybe the wiki should also mention from
> what version of the driver it is supported (I guess for all cases, unless
> their versioning would be synced with PostgreSQL's).


We won't by syncing our version numbers with Postgres



Dave Cramer

da...@postgresintl.com
www.postgresintl.com

Re: [HACKERS] Parallel Append implementation

[Amit Khandekar]

On 11 September 2017 at 18:55, Amit Kapila  wrote:
>>> Do you think non-parallel-aware Append
>>> will be better in any case when there is a parallel-aware append?  I
>>> mean to say let's try to create non-parallel-aware append only when
>>> parallel-aware append is not possible.
>>
>> By non-parallel-aware append, I am assuming you meant  partial
>> non-parallel-aware Append. Yes, if the parallel-aware Append path has
>> *all* partial subpaths chosen, then we do omit a partial non-parallel
>> Append path, as seen in this code in the patch :
>>
>> /*
>> * Consider non-parallel partial append path. But if the parallel append
>> * path is made out of all partial subpaths, don't create another partial
>> * path; we will keep only the parallel append path in that case.
>> */
>> if (partial_subpaths_valid && !pa_all_partial_subpaths)
>> {
>> ..
>> }
>>
>> But if the parallel-Append path has a mix of partial and non-partial
>> subpaths, then we can't really tell which of the two could be cheapest
>> until we calculate the cost. It can be that the non-parallel-aware
>> partial Append can be cheaper as well.
>>
>
> How?  See, if you have four partial subpaths and two non-partial
> subpaths, then for parallel-aware append it considers all six paths in
> parallel path whereas for non-parallel-aware append it will consider
> just four paths and that too with sub-optimal strategy.  Can you
> please try to give me some example so that it will be clear.

Suppose 4 appendrel children have costs for their cheapest partial (p)
and non-partial paths (np)  as shown below :

p1=5000  np1=100
p2=200   np2=1000
p3=80   np3=2000
p4=3000  np4=50

Here, following two Append paths will be generated :

1. a parallel-aware Append path with subpaths :
np1, p2, p3, np4

2. Partial (i.e. non-parallel-aware) Append path with all partial subpaths:
p1,p2,p3,p4

Now, one thing we can do above is : Make the path#2 parallel-aware as
well; so both Append paths would be parallel-aware. Are you suggesting
exactly this ?

So above, what I am saying is, we can't tell which of the paths #1 and
#2 are cheaper until we calculate total cost. I didn't understand what
did you mean by "non-parallel-aware append will consider only the
partial subpaths and that too with sub-optimal strategy" in the above
example. I guess, you were considering a different scenario than the
above one.

Whereas, if one or more subpaths of Append do not have partial subpath
in the first place, then non-parallel-aware partial Append is out of
question, which we both agree.
And the other case where we skip non-parallel-aware partial Append is
when all the cheapest subpaths of the parallel-aware Append path are
partial paths: we do not want parallel-aware and non-parallel-aware
Append paths both having exactly the same partial subpaths.

-

I will be addressing your other comments separately.

Thanks
-Amit Khandekar


-- 
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] additional contrib test suites

[David Steele]

On 9/8/17 1:32 PM, Peter Eisentraut wrote:
> 
> Yes, some of the error messages had changed.  Fixed patches attached.

Patches apply and all tests pass.  A few comments:

* [PATCH v2 1/7] adminpack: Add test suite

There are no regular tests for pg_logdir_ls().  It looks like TAP tests
would be required but I'm not sure it's worth it.  The fact that the
"default" log name format is hard-coded in is, um, interesting.

Maybe add:

+ SELECT pg_logdir_ls();
+ ERROR:  could not read directory "log": No such file or directory

to get a little more coverage?  It would be good to at least have a note
on why it is not tested.

* [PATCH v2 4/7] chkpass: Add test suite

Well, this is kind of scary:

+CREATE EXTENSION chkpass;
+WARNING:  type input function chkpass_in should not be volatile

I guess the only side effect is that this column cannot be indexed?  The
docs say that, so OK, but is there anything else a user should be
worried about?

The rest looks good.  I'll mark this "Ready for Committer" since I'm the
only reviewer.  I don't think anything you might add based on my
comments above requires a re-review.

As for testing on more platforms, send it to the build farm?

-- 
-David
da...@pgmasters.net


-- 
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] psql: new help related to variables are not too readable

[Pavel Stehule]

2017-09-14 15:17 GMT+02:00 Alvaro Herrera :

> Tom Lane wrote:
> > "David G. Johnston"  writes:
> > >​If I was going to try and read it like a book I'd want the extra
> > > white-space to make doing so easier (white-space gives the eye a
> breather
> > > when done with a particular concept) - and the length wouldn't really
> > > matter since I'd just make a single pass and be done with it.  But the
> > > planned usage is for quick lookup of options that you know (or at least
> > > suspect) exist and which you probably have an approximate idea of how
> they
> > > are spelled.  The all-caps and left-justified block headers are
> distinct
> > > enough to scan down - though I'd consider indenting 4 spaces instead
> of 2
> > > to make that even easier (less effort to ignore the indented lines
> since
> > > ignoring nothing is easier than ignoring something).​  Having more fit
> on
> > > one screen makes that vertical skimming considerably easier as well (no
> > > break and re-acquire when scrolling in a new page).
> >
> > Hmm, indenting the descriptions a couple more spaces might be a workable
> > compromise.  Anyone want to try that and see what it looks like?
> > Preferably somebody who's not happy with the current layout ;-)
>
> I have to admit that adding two spaces makes it look a lot more
> acceptable to me.
>

I did some tests now, and when the name of variable is a upper case string,
then are acceptable (although with empty line space it is better).

for pset variables (is lower case), then reading is not too friendly still.

Sure - four spaces is better than two - but readability is not good.

There can be another reason of feeling vertical spaces - the size of chars.
I am using probably small fonts - I am using X windows and my typical
terminal windows is half of screen (I have T520 Lenovo) about 60 rows and
120 columns.

Please, look to document https://github.com/darold/ora2pg README and try to
remove empty lines.

Regards

Pavel


>
> (I'd tweak the description of PSQL_EDITOR_LINENUMBER_ARG by replacing
> "how to" with "mechanism to" while at it, by the way.  It took me a
> while to understand what it was and I first thought the description was
> completely bogus.)
>
> --
> Álvaro Herrerahttps://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>

Re: [HACKERS] Log LDAP "diagnostic messages"?

[Alvaro Herrera]

BTW I added --with-ldap and --with-pam to the configure line for the
reports in coverage.postgresql.org and the % covered in auth.c went from
24% to 18.9% (from very bad to terribly sad).

-- 
Á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] PATCH : Generational memory allocator (was PATCH: two slab-like memory allocators)

[Simon Riggs]

On 14 August 2017 at 01:35, Tomas Vondra  wrote:
> Hi,
>
> Attached is a rebased version of the Generational context, originally
> submitted with SlabContext (which was already committed into Pg 10).
>
> The main change is that I've abandoned the pattern of defining a Data
> structure and then a pointer typedef, i.e.
>
> typedef struct GenerationContextData { ... } GenerationContextData;
> typedef struct GenerationContextData *GenerationContext;
>
> Now it's just
>
> typedef struct GenerationContext { ... } GenerationContext;
>
> mostly because SlabContext was committed like that, and because Andres was
> complaining about this code pattern ;-)
>
> Otherwise the design is the same as repeatedly discussed before.
>
> To show that this is still valuable change (even after SlabContext and
> adding doubly-linked list to AllocSet), I've repeated the test done by
> Andres in [1] using the test case described in [2], that is
>
>   -- generate data
>   SELECT COUNT(*) FROM (SELECT test1()
>   FROM generate_series(1, 5)) foo;
>
>   -- benchmark (measure time and VmPeak)
>   SELECT COUNT(*) FROM (SELECT *
>   FROM pg_logical_slot_get_changes('test', NULL,
> NULL, 'include-xids', '0')) foo;
>
> with different values passed to the first step (instead of the 5). The
> VmPeak numbers look like this:
>
>  N   masterpatched
> --
> 10   1155220 kB  361604 kB
> 20   2020668 kB  434060 kB
> 30   2890236 kB  502452 kB
> 40   3751592 kB  570816 kB
> 50   4621124 kB  639168 kB
>
> and the timing (on assert-enabled build):
>
>  N   masterpatched
> --
> 10  1103.182 ms 412.734 ms
> 20  2216.711 ms 820.438 ms
> 30  3320.095 ms1223.576 ms
> 40  4584.919 ms1621.261 ms
> 50  5590.444 ms2113.820 ms
>
> So it seems it's still a significant improvement, both in terms of memory
> usage and timing. Admittedly, this is a single test, so ideas of other
> useful test cases are welcome.

This all looks good.

What I think this needs is changes to
   src/backend/utils/mmgr/README
which decribe the various options that now exist (normal?, slab) and
will exist (generational)

Don't really care about the name, as long as its clear when to use it
and when not to use it.

This description of generational seems wrong: "When the allocated
chunks have similar lifespan, this works very well and is extremely
cheap."
They don't need the same lifespan they just need to be freed in groups
and in the order they were allocated.

For this patch specifically, we need additional comments in
reorderbuffer.c to describe the memory allocation pattern in that
module so that it is clear that the choice of allocator is useful and
appropriate, possibly with details of how that testing was performed,
so it can be re-tested later or tested on a variety of platforms.

Particularly in reorderbuffer, surely we will almost immediately reuse
chunks again, so is it worth issuing free() and then malloc() again
soon after? Does that cause additional overhead we could also avoid?
Could we possibly keep the last/few free'd chunks around rather than
re-malloc?

Seems like we should commit this soon.

-- 
Simon Riggshttp://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] Log LDAP "diagnostic messages"?

[Alvaro Herrera]

I think the ldap_unbind() changes should be in a separate preliminary
patch to be committed separately and backpatched.

The other bits looks fine, with nitpicks

1. please move the new support function to the bottom of the section
dedicated to LDAP, and include a prototype

2. please wrap lines longer than 80 chars, other than error message
strings.  (I don't know why this file plays fast & loose with
project-wide line length rules, but I also see no reason to continue
doing it.)

-- 
Á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] Parallel Hash take II

[Thomas Munro]

On Thu, Sep 14, 2017 at 11:57 AM, Thomas Munro
 wrote:
> On Thu, Sep 14, 2017 at 12:51 AM, Prabhat Sahu
>  wrote:
>> Setting with lower "shared_buffers" and "work_mem" as below,  query getting 
>> crash but able to see explain plan.
>
> Thanks Prabhat.  A small thinko in the batch reset code means that it
> sometimes thinks the shared skew hash table is present and tries to
> probe it after batch 1.  I have a fix for that and I will post a new
> patch set just as soon as I have a good regression test figured out.

Fixed in the attached version, by adding a missing
"hashtable->shared->num_skew_buckets = 0;" to ExecHashFreeSkewTable().
I did some incidental tidying of the regression tests, but didn't
manage to find a version of your example small enough to put in a
regression tests.  I also discovered some other things:

1.  Multi-batch Parallel Hash Join could occasionally produce a
resowner warning about a leaked temporary File associated with
SharedTupleStore objects.  Fixed by making sure we call routines that
close all files handles in ExecHashTableDetach().

2.  Since last time I tested, a lot fewer TPCH queries choose a
Parallel Hash plan.  Not sure why yet.  Possibly because Gather Merge
and other things got better.  Will investigate.

3.  Gather Merge and Parallel Hash Join may have a deadlock problem.
Since Gather Merge needs to block waiting for tuples, but workers wait
for all participants (including the leader) to reach barriers.  TPCH
Q18 (with a certain set of indexes and settings, YMMV) has Gather
Merge over Sort over Parallel Hash Join, and although it usually runs
successfully I have observed one deadlock.  Ouch.  This seems to be a
more fundamental problem than the blocked TupleQueue scenario.  Not
sure what to do about that.

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


parallel-hash-v20.patchset.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