Re: [HACKERS] "pg_xxx" role name restriction not applied to bootstrap superuser?

[David G. Johnston]

On Saturday, May 7, 2016, Tom Lane  wrote:

> Stephen Frost > writes:
> > * Tom Lane (t...@sss.pgh.pa.us ) wrote:
> >> ... but I'm left with a policy question: should initdb disallow
> >> bootstrap superuser names like "pg_xxx"?
>
> > On the whole, I'd vote to treat the bootstrap user as a normal role and
> > therefore have the same restriction in place for that user also.
>
> If we're going to enforce such a restriction, I think it would be
> a good thing for it to be in place in beta1.
>
>
I don't fathom a good reason to treat only the bootstrap user differently.
I'd second guess prohibiting pg_ generally instead of only the specific
system roles in use in a given release.  Having beta1 go out with full
restrictions will at least maximize the chance of getting complaints and
insight into how prevalent the prefix is in the wild.

David J.

Re: [HACKERS] minor message improvement

[Tom Lane]

Euler Taveira  writes:
> While updating translations, I noticed that access/transam/xlog.c:6174
> contains different messages that could be the same.
> "ignoring file \"%s\" because no file \"%s\" exists"
> a few lines above
> "ignoring \"%s\" file because no \"%s\" file exists"
> Attached is a patch that turn it into one.

Pushed, thanks.

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] First-draft release notes for next week's back-branch releases

[David G. Johnston]

On Friday, May 6, 2016, Tom Lane  wrote:

> If you're not tired of reviewing release notes (I'm sure getting a bit
> tired of writing them), see
>
>
> http://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=eb7de00ac2d282263541ece849ec71e2809e9467
>
> guaibasaurus should have 'em up on the web in an hour or so, too, at
>
> http://www.postgresql.org/docs/devel/static/release-9-5-3.html
>
>
"...replacement_sort_tuples, which see for further details." needs
rewording.

For some reason I had trouble comprehending the index only scans on partial
index couple or paragraphs.  Got it after a few reads.  Seems like
it's almost too detailed.

"Partial indexes can be used for index only scans in some circumstances.
See section for details."  If there isn't a section to point to there
should be - people want to know how to get IOS and aren't going to read
release notes to figure it out.

Are the pg_stat_activity changes breaking changes? If so its not clear
from the notes.

I'll +1 the elsewhere mentioned confusion adding a pg_config view vis-a-vis
pg_settings.  Adding (or using) the word "compile" would be advisable.

The guc for the number of standby servers that must acknowledge should be
named in the notes and linked to the main docs.  "An additional syntax has
been added to synchronous_standby_names to accommodate the number of
standby servers that must acknowledge a commit."

Is it worth mentioning the deprecation of exclusive backups in the notes
introducing non-exclusive ones?

Read the rest and nothing stood out - though I guess I'd advise myself or
the next person to read up from the bottom so fresh eyes read the lower
stuff first.

David J.

Re: [HACKERS] Re: "pg_xxx" role name restriction not applied to bootstrap superuser?

[Tom Lane]

Stephen Frost  writes:
> * Tom Lane (t...@sss.pgh.pa.us) wrote:
>> ... but I'm left with a policy question: should initdb disallow
>> bootstrap superuser names like "pg_xxx"?

> On the whole, I'd vote to treat the bootstrap user as a normal role and
> therefore have the same restriction in place for that user also.

If we're going to enforce such a restriction, I think it would be
a good thing for it to be in place in beta1.

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: Disable BLOB test in pg_dump TAP tests

[Stephen Frost]

Tom,

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Stephen Frost  writes:
> > Disable BLOB test in pg_dump TAP tests
> 
> > Buildfarm member jacana appears to have an issue with running this
> > test.  It's not entirely clear to me why, but rather than try to
> > fight with it, just disable it for now.
> 
> BTW, what was your evidence for thinking that that specific test
> needed to be disabled?  It looks to me like it would have been
> subject to the $-in-/m-mode bug we identified today, so I'm wondering
> if it was merely the first to fail.

No, those errors were "match/don't match" errors.  At the bottom of:

http://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=jacana=2016-05-06%2023%3A41%3A15=bin-check

is the error, which is 'No such file or directory'.  I'm pretty sure the
error is from the '\o' in that test which is trying to write a file to
the '$tempdir' directory.  Clearly, on that system at least, $tempdir
isn't a completely fully-qualified path, since it ends up being:

/home/pgrunner/bf/root/HEAD/pgsql.build/src/bin/pg_dump/tmp_check/tmp_test_v8cH

but the data directory is:

c:/mingw/msys/1.0/home/pgrunner/bf/root/HEAD/pgsql.build/src/bin/pg_dump/tmp_check/data_main_NSG0/pgdata

I'm not quite sure how that system works (is a starting '/' actually
referring to a relative path, and psql isn't being run from that root
directory?  Or does the shell handle converting arguments which start
with a '/' to their full path, and that's why calls to pg_ctl and other
things work, but trying to use that same path from inside a program
doesn't?), but apparently referring to $tempdir from inside psql doesn't
give you the same directory that referring to it from the TAP perl
script does.

> I think it's probably best not to monkey with it right now, but after
> beta1 is out, we should try re-enabling it.

I'd definitely like to work on getting that test working everywhere
post-beta1, but the simplest answer will probably be to just load the
large-object in the same way that pg_dump dumps it out, namely using the
'lo_open', 'lo_write', 'lo_close' functions.  The goal of that test is
to make sure that pg_dump does the right thing when it comes to
exporting the large object, not to test that \lo_import of psql works
(though that would certainly be a good thing to test in a set of psql
tests...).

Thanks!

Stephen


signature.asc
Description: Digital signature

[HACKERS] force_parallel_mode uniqueness

[David G. Johnston]

My take below is that of a user reading our documentation and our projected
consistency via that document.

All of the other planner GUCs are basically, {on, off, special} with on or
special the default as appropriate for the feature - since most/all
features default to enabled.  While I get that the expected usage is to set
this to off (which really leaves parallel mode in its default on
behavior) and then reduce the parallel workers to zero to disable that runs
contrary to all of the other switches listed alongside force_parallel_mode.
 constraint_exclusion seems like something to be emulated here.

Also, all of the other geoq options get placed here yet max parallel
degree is in an entirely different section.  I'm a bit torn on this point
though since it does fit nicely in asynchronous behavior.  I think the next
thought finds the happy middle.

If nothing else this option should include a link to max_parallel_degree
and vice-versa.  Noting how to disable the feature in this section, if the
guc semantics are not changed, would be good too.  That note would
likely suffice to establish the linking term to parallel degree.  Something
can be devised, even if just a see also, to link back.

David J.

Re: [HACKERS] Accurate list of Keywords / Datatypes?

[Euler Taveira]

On 07-05-2016 22:53, Robins Tharakan wrote:
> Should I be looking somewhere else? Parse keywords from Git Source file
> (if so where)? Parse PG Documentation?
> 
src/include/parser/kwlist.h


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


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

[HACKERS] Accurate list of Keywords / Datatypes?

[Robins Tharakan]

Hi,

While creating a Syntax Highlighting XML for Notepad++ (something like a
PLSQL one here http://goo.gl/UBbHdt ), I was looking for a list of Keywords
(& separately list of Datatypes) that Postgres uses in a given version (Say
DEVEL branch).

I did find the Reserved Keyword list (
http://www.postgresql.org/docs/devel/static/sql-keywords-appendix.html) but
it doesn't look like what I need, because:

1. The ones marked as 'Reserved' isn't all the words I am interested in
(since these are just 'Reserved')
2. The entire list seems like an overkill (since it has words such as K
and ROUTINE_CATALOG, which aren't really what I'd like highlighted when I
am working on an SQL file in NPP)

Should I be looking somewhere else? Parse keywords from Git Source file (if
so where)? Parse PG Documentation?

I tried ( https://goo.gl/OYeYuE ) parsing HTML Tags (such as VARNAME /
TOKEN / LITERAL / COMMAND) in PG Documentation but is there's a simpler
(more accurate) way for this?

Any pointers would be great!
-
​thanks
robins

[HACKERS] minor message improvement

[Euler Taveira]

Hi,

While updating translations, I noticed that access/transam/xlog.c:6174
contains different messages that could be the same.

"ignoring file \"%s\" because no file \"%s\" exists"

a few lines above

"ignoring \"%s\" file because no \"%s\" file exists"

Attached is a patch that turn it into one.


-- 
   Euler Taveira   Timbira - http://www.timbira.com.br/
   PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index f9644db..b473f19 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -6179,7 +6179,7 @@ StartupXLOG(void)
 			   TABLESPACE_MAP, TABLESPACE_MAP_OLD)));
 			else
 ereport(LOG,
-		(errmsg("ignoring \"%s\" file because no \"%s\" file exists",
+		(errmsg("ignoring file \"%s\" because no file \"%s\" exists",
 TABLESPACE_MAP, BACKUP_LABEL_FILE),
 		 errdetail("Could not rename file \"%s\" to \"%s\": %m.",
    TABLESPACE_MAP, TABLESPACE_MAP_OLD)));

-- 
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: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

[Andres Freund]

On 2016-05-06 20:28:27 -0500, Kevin Grittner wrote:
> On Fri, May 6, 2016 at 7:48 PM, Andres Freund  wrote:
> > On 2016-05-06 19:43:24 -0500, Kevin Grittner wrote:
> >> It's disappointing that I am not getting more consistent numbers,
> >> but NUMA can be hard to manage that way.
> >
> > FWIW, in my experience, unless you disable autovacuum (or rather
> > auto-analyze), the effects from non-predicable analyze runs with
> > long-running snapshots are worse.  I mean the numa effects suck, but in
> > r/w workload effects of analyze are often much worse.
> 
> Hm.  But the benefits of the patch are not there if autovacuum is
> off.  I'm gonna need to ponder the best way to test given all that.

It's sufficient to set the threshhold for analyze very high, as vacuum
itself doesn't have that problem. I basically just set
autovacuum_analyze_threshold to INT_MAX , that alleviates the problem
for normal runs.

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] ALTER TABLE lock downgrades have broken pg_upgrade

[Simon Riggs]

On 7 May 2016 at 16:49, Tom Lane  wrote:

> Simon Riggs  writes:
> > On 3 May 2016 at 18:07, Tom Lane  wrote:
> >> Or at least, it did until Simon decided that ALTER TABLE RESET
> >> doesn't require AccessExclusiveLock.
>
> > On reflection, this still seems like a good idea.
>
> Yes, what pg_upgrade was doing was clearly a hack, and not a very nice one.
>
> > I accept that it is my bug and should fix it.
>
> It's moot at this point, see 1a2c17f8e.
>

OK, sorry for the noise and thanks for the fix.

-- 
Simon Riggshttp://www.2ndQuadrant.com/

PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] [COMMITTERS] pgsql: Add TAP tests for pg_dump

[Stephen Frost]

Peter,

* Peter Eisentraut (peter.eisentr...@2ndquadrant.com) wrote:
> On 5/7/16 9:36 AM, Stephen Frost wrote:
> >Honestly, over the next couple of months between feature-freeze and
> >release, I'd like to add even more tests, and not just to pg_dump but
> >also to other commands that don't have very good testing today (psql, in
> >particular, but pg_dumpall needs more also, and there's more to do with
> >pg_dump too).
> 
> If we're going to do that, there will be no stopping it.  I also
> have a bunch of code in this area lined up, but I was hoping to
> submit it to the commit fest process at the right time and order to
> get feedback and testing.  If we're going to allow new tests being
> developed right up until release, I can just stop doing release
> preparation work right now and get back to coding and reviewing.

I do think that now is a good time for people to be reviewing what's
been committed, which includes writing tests (either automated ones,
which can be included in our test suite, or one-off's for testing
specific things but which don't make sense to include).

That doesn't mean we should be codeing or reviewing new features for
commit.

As for release prep, I certainly applaud everyone involved in that
effort as we do have the beta release and back-branch releases coming
up.

Regarding when we should stop adding new tests, I can't agree with the
notion that they should be treated as new features.  New tests may break
the buildfarm, but they do not impact the stability of committed code,
nor do they introduce new bugs into the code which has been committed
(if anything, they expose committed bugs, as the set of tests we're
discussing did, which should then be fixed).

> Ultimately, tests are code and should be treated as such.

Perhaps in some manners this is accurate, but I'd view our test suite as
an independent code base from PG.  Bugs in the test suite might cause
false test failures or other issues on the buildfarm or during
packaging, but bugs or issues in the test suite won't cause data loss,
data corruption, or generally impact running operations of our users.

I can see some sense in holding off on adding more tests once we hit RC,
as we want anything done with RC to be essentially identical to the
release, unless there is a serious issue, but holding off on adding new
tests which could expose issues in committed code for the months during
beta doesn't seem sensible.

As such, I'd certainly encourage you to propose new tests, even now, but
not changes to the PG code, except for bug fixes, or changes agreed to
by the RMT.  How you prioritise that with the release preparation work
is up to you, of course, though if I were in your shoes, I'd take care
of the release prep first, as we're quite close to doing a set of
releases.  I'm happy to try and help with that effort myself, though
I've not been involved very much in release prep and am not entirely
sure what I can do to help.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] [COMMITTERS] pgsql: Add TAP tests for pg_dump

[Peter Eisentraut]

On 5/7/16 9:36 AM, Stephen Frost wrote:

Honestly, over the next couple of months between feature-freeze and
release, I'd like to add even more tests, and not just to pg_dump but
also to other commands that don't have very good testing today (psql, in
particular, but pg_dumpall needs more also, and there's more to do with
pg_dump too).


If we're going to do that, there will be no stopping it.  I also have a 
bunch of code in this area lined up, but I was hoping to submit it to 
the commit fest process at the right time and order to get feedback and 
testing.  If we're going to allow new tests being developed right up 
until release, I can just stop doing release preparation work right now 
and get back to coding and reviewing.


Ultimately, tests are code and should be treated as such.


When it comes to packaging, if adding tests using the existing test
infrastructure (the TAP system) causes a problem there, then I think we
need to address that issue rather than not add new tests.  Packagers
also always have the option to not enable the tap tests, if there really
is an issue there which can't be addressed.


Well, that's like saying, if the feature I just committed the night 
before the release turns out to be problematic, you can just ifdef it 
out.  I don't think we want that, and I think it simplifies the way the 
world works.  Because new code interacts with old code, and then there 
will be even more new code which interacts with other code, and so it 
becomes harder and harder to isolate issues.  Yeah, if adding more tests 
causes instabilities because of issues not related to the tests 
themselves, we should fix that.  But that doesn't mean we should 
hand-wave past the fact that that potential exists.  That's software 
development.  If everything were perfect, we wouldn't need a beta period 
at all.


The configure flag to disable TAP tests isn't there so that we get 
license to play with them at random, it's because we wanted to make the 
software dependency optional.  The psql tests run under pg_regress, so 
they can't be made optional anyway.


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

[HACKERS] Re: "pg_xxx" role name restriction not applied to bootstrap superuser?

[Stephen Frost]

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> So this seems like another reason why removing those checks was an
> improvement, but I'm left with a policy question: should initdb disallow
> bootstrap superuser names like "pg_xxx"?  This doesn't seem quite
> open-and-shut.  On the one hand, if we leave it as-is, then people might
> be blindsided by future additions of built-in roles.  On the other,
> if we forbid the case, it seems noticeably more likely that we'll break
> existing setups, because "pg_something" doesn't seem like a terribly
> unlikely choice for the name of the Postgres OS user.  (Certainly
> opossum's owner would have to fix it, so that's one example out of a
> not very large sample space of buildfarm users...)  Allowing a potential
> conflict for the bootstrap superuser is a much narrower conflict risk
> than any-old-user, so maybe it's okay to leave it as is.

On the whole, I'd vote to treat the bootstrap user as a normal role and
therefore have the same restriction in place for that user also.  As was
mentioned previously, it's already impossible to create schemas which
start with 'pg_', so you couldn't have a 'pg_buildfarmer' schema.  I
realize that, for the buildfarm, that's not an issue, but that's a bit
of a special case.

> Also, the failure mode if you do get an actual, rather than hypothetical,
> conflict against a built-in role name isn't all that nice:
> 
> $ initdb -U pg_signal_backend
> ...
> running bootstrap script ... FATAL:  could not create unique index 
> "pg_authid_rolname_index"
> DETAIL:  Key (rolname)=(pg_signal_backend) is duplicated.
> ...
> 
> While it's not hard to interpret this if you already know that
> "pg_signal_backend" is a reserved role name, an explicit failure message
> saying that the bootstrap superuser name can't begin with "pg_" would be
> more user-friendly.  So that's a point in favor of having initdb reject
> the case.
> 
> On the whole I lean to adding a restriction, but only weakly.

Agreed.

Thanks!

Stephen


signature.asc
Description: Digital signature

[HACKERS] "pg_xxx" role name restriction not applied to bootstrap superuser?

[Tom Lane]

I noticed that opossum's latest buildfarm run failed, evidently because
it was set up with the user running the buildfarm named "pg_buildfarmer":

http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=opossum=2016-05-03%2018%3A43%3A31

That caused the bootstrap superuser's name to be "pg_buildfarmer".
initdb didn't complain about this, but it was impossible to log in:

016-05-04 02:29:00.820 BST [5729505c.26:1] LOG:  connection received: 
host=[local]
2016-05-04 02:29:00.927 BST [5729505c.26:2] LOG:  connection authorized: 
user=pg_buildfarmer database=postgres
2016-05-04 02:29:00.932 BST [5729505c.26:3] FATAL:  invalid value for parameter 
"session_authorization": "pg_buildfarmer"
2016-05-04 02:29:02.260 BST [5729505e.4396:1] LOG:  connection received: 
host=[local]
2016-05-04 02:29:02.328 BST [5729505e.4396:2] LOG:  connection authorized: 
user=pg_buildfarmer database=postgres
2016-05-04 02:29:02.333 BST [5729505e.4396:3] FATAL:  invalid value for 
parameter "session_authorization": "pg_buildfarmer"
2016-05-04 02:29:03.662 BST [5729505f.57cd:1] LOG:  connection received: 
host=[local]
2016-05-04 02:29:03.731 BST [5729505f.57cd:2] LOG:  connection authorized: 
user=pg_buildfarmer database=postgres
2016-05-04 02:29:03.735 BST [5729505f.57cd:3] FATAL:  invalid value for 
parameter "session_authorization": "pg_buildfarmer"

I tried to duplicate this failure just now, and could not.  Evidently,
the failures opossum shows above come from the "cannot set role to pg_xxx"
checks you had in check_session_authorization, which are gone as of commit
a89505fd2.  So in principle opossum should succeed if it were to try
again today with the same environment.

So this seems like another reason why removing those checks was an
improvement, but I'm left with a policy question: should initdb disallow
bootstrap superuser names like "pg_xxx"?  This doesn't seem quite
open-and-shut.  On the one hand, if we leave it as-is, then people might
be blindsided by future additions of built-in roles.  On the other,
if we forbid the case, it seems noticeably more likely that we'll break
existing setups, because "pg_something" doesn't seem like a terribly
unlikely choice for the name of the Postgres OS user.  (Certainly
opossum's owner would have to fix it, so that's one example out of a
not very large sample space of buildfarm users...)  Allowing a potential
conflict for the bootstrap superuser is a much narrower conflict risk
than any-old-user, so maybe it's okay to leave it as is.

Also, the failure mode if you do get an actual, rather than hypothetical,
conflict against a built-in role name isn't all that nice:

$ initdb -U pg_signal_backend
...
running bootstrap script ... FATAL:  could not create unique index 
"pg_authid_rolname_index"
DETAIL:  Key (rolname)=(pg_signal_backend) is duplicated.
...

While it's not hard to interpret this if you already know that
"pg_signal_backend" is a reserved role name, an explicit failure message
saying that the bootstrap superuser name can't begin with "pg_" would be
more user-friendly.  So that's a point in favor of having initdb reject
the case.

On the whole I lean to adding a restriction, but only weakly.

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] ALTER TABLE lock downgrades have broken pg_upgrade

[Tom Lane]

Simon Riggs  writes:
> On 3 May 2016 at 18:07, Tom Lane  wrote:
>> Or at least, it did until Simon decided that ALTER TABLE RESET
>> doesn't require AccessExclusiveLock.

> On reflection, this still seems like a good idea.

Yes, what pg_upgrade was doing was clearly a hack, and not a very nice one.

> I accept that it is my bug and should fix it.

It's moot at this point, see 1a2c17f8e.

regards, tom lane


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

Re: [HACKERS] pg_dump broken for non-super user

[Stephen Frost]

* Simon Riggs (si...@2ndquadrant.com) wrote:
> On 7 May 2016 at 16:21, Stephen Frost  wrote:
> > * Simon Riggs (si...@2ndquadrant.com) wrote:
> > > On 7 May 2016 at 16:14, Stephen Frost  wrote:
> > > > > If we don't lock it then we will have a inconsistent dump that will
> > fail
> > > > > later, if dumped while an object is being dropped.
> > > > > Do we want an inconsistent dump?
> > > >
> > > > The dump won't be inconsistent, as Tom pointed out.  The catalog tables
> > > > are read using a repeatable read transaction, which will be consistent.
> > >
> > > The scan is consistent, yes, but the results would not be.
> >
> > I'm not following- the results are entirely dependent on the scan, so if
> > the scan is consistent, how could the results not be?
> >
> 
> Objects would no longer exist because of concurrent DROPs.

A concurrent DROP wouldn't actually produce different results than it
did before, except that the DROP would be allowed to proceed, rather
than block behind the dump.  In both cases, the dump would include that
table.

> You agreed before, why did you change?

I realized that Tom was right that we are just reading from the tables
using regular SELECTs in these cases and therefore the repeatable-read
transaction semantics would be used.

There are cases where that doesn't work, as discussed up-thread and in
the comments, but those are cases where we are using server-side
functions that use SysCache and don't respect the repeatable-read
transaction.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] pg_dump broken for non-super user

[Simon Riggs]

On 7 May 2016 at 16:21, Stephen Frost  wrote:

> * Simon Riggs (si...@2ndquadrant.com) wrote:
> > On 7 May 2016 at 16:14, Stephen Frost  wrote:
> > > > If we don't lock it then we will have a inconsistent dump that will
> fail
> > > > later, if dumped while an object is being dropped.
> > > > Do we want an inconsistent dump?
> > >
> > > The dump won't be inconsistent, as Tom pointed out.  The catalog tables
> > > are read using a repeatable read transaction, which will be consistent.
> >
> > The scan is consistent, yes, but the results would not be.
>
> I'm not following- the results are entirely dependent on the scan, so if
> the scan is consistent, how could the results not be?
>

Objects would no longer exist because of concurrent DROPs.

You agreed before, why did you change?

-- 
Simon Riggshttp://www.2ndQuadrant.com/

PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] pg_dump broken for non-super user

[Stephen Frost]

* Simon Riggs (si...@2ndquadrant.com) wrote:
> On 7 May 2016 at 16:14, Stephen Frost  wrote:
> > > If we don't lock it then we will have a inconsistent dump that will fail
> > > later, if dumped while an object is being dropped.
> > > Do we want an inconsistent dump?
> >
> > The dump won't be inconsistent, as Tom pointed out.  The catalog tables
> > are read using a repeatable read transaction, which will be consistent.
> 
> The scan is consistent, yes, but the results would not be.

I'm not following- the results are entirely dependent on the scan, so if
the scan is consistent, how could the results not be?

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] pg_dump broken for non-super user

[Simon Riggs]

On 7 May 2016 at 16:14, Stephen Frost  wrote:


> > If we don't lock it then we will have a inconsistent dump that will fail
> > later, if dumped while an object is being dropped.
> > Do we want an inconsistent dump?
>
> The dump won't be inconsistent, as Tom pointed out.  The catalog tables
> are read using a repeatable read transaction, which will be consistent.


The scan is consistent, yes, but the results would not be.

-- 
Simon Riggshttp://www.2ndQuadrant.com/

PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] pg_dump broken for non-super user

[Stephen Frost]

Simon,

* Simon Riggs (si...@2ndquadrant.com) wrote:
> On 4 May 2016 at 16:45, Tom Lane  wrote:
> > Why is it that we need to lock a table at all if we're just going to dump
> > its ACL?
> 
> We don't, but surely that's the wrong question.

I tend to agree with this, however...

> If we don't lock it then we will have a inconsistent dump that will fail
> later, if dumped while an object is being dropped.
> Do we want an inconsistent dump?

The dump won't be inconsistent, as Tom pointed out.  The catalog tables
are read using a repeatable read transaction, which will be consistent.

> For what reason are we changing existing behaviour? There is no bug here,
> as Stephen explained.
> 
> So this is a behaviour change after freeze with uncertain purpose.

This isn't accurate.  We never locked tables in pg_catalog before, as we
never looked at them, and that's currently the only case where the new
logic will apply.  We may change the behavior for --no-privileges (and
perhaps other options) in the future to also have this logic apply, but
I agree that's 9.7 material.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Reviewing freeze map code

[Masahiko Sawada]

On Sat, May 7, 2016 at 6:00 AM, Joshua D. Drake  wrote:
> On 05/06/2016 01:58 PM, Stephen Frost wrote:
>>
>> * Joshua D. Drake (j...@commandprompt.com) wrote:
>>>
>>> Yeah I thought about that, it is the word "FORCE" that bothers me.
>>> When you use FORCE there is an assumption that no matter what, it
>>> plows through (think rm -f). So if we don't use FROZEN, that's cool
>>> but FORCE doesn't work either.
>>
>>
>> Isn't that exactly what this FORCE option being contemplated would do
>> though?  Plow through the entire relation, regardless of what the VM
>> says is all frozen or not?
>>
>> Seems like FORCE is a good word for that to me.
>
>
> Except that we aren't FORCING a vacuum. That is the part I have contention
> with. To me, FORCE means:
>
> No matter what else is happening, we are vacuuming this relation (think
> locks).
>
> But I am also not going to dig in my heals. If that is truly what -hackers
> come up with, thank you at least considering what I said.
>
> Sincerely,
>
> JD
>

As Joshua mentioned, FORCE word might imply doing VACUUM while plowing
through locks.
I guess that it might confuse the users.
IMO, since this option will be a way for emergency, SCANALL word works for me.

Or other ideas are,
VACUUM IGNOREVM
VACUUM RESCURE

Regards,

--
Masahiko Sawada


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

Re: [HACKERS] pg_dump broken for non-super user

[Simon Riggs]

On 4 May 2016 at 16:45, Tom Lane  wrote:


> Why is it that we need to lock a table at all if we're just going to dump
> its ACL?


We don't, but surely that's the wrong question.

If we don't lock it then we will have a inconsistent dump that will fail
later, if dumped while an object is being dropped.
Do we want an inconsistent dump?

For what reason are we changing existing behaviour? There is no bug here,
as Stephen explained.

So this is a behaviour change after freeze with uncertain purpose.

-- 
Simon Riggshttp://www.2ndQuadrant.com/

PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] [COMMITTERS] pgsql: Add TAP tests for pg_dump

[Stephen Frost]

* Peter Eisentraut (peter.eisentr...@2ndquadrant.com) wrote:
> On 5/6/16 3:11 PM, Stephen Frost wrote:
> >These are just new tests..?
> 
> This is a matter of degree, but I think there is a difference
> between new test cases and a whole new test suite.

To be clear, I've been calling it a 'test suite' because there ended up
being over 1000 tests added, but it's all using the exinsting
infrastructure of the TAP testing system, just as we have for other
commands (pg_ctl, initdb, etc).

> >I assumed that would be welcome during post
> >feature-freeze, and certainly no one raised any concerns about adding
> >these tests during the discussion prior to my commiting them.
> 
> I didn't see that discussion and I still can't find it.

The new tests were discussed on the thread related to the changes to
pg_dump.  The first patch, which included just 300 or so tests, was
here:

http://www.postgresql.org/message-id/20160425043909.gw10...@tamriel.snowman.net

There was a bit of back-and-forth on that thread and new patches were
posted there, as well as indication that I was planning to push them a
couple days before I did.

Honestly, over the next couple of months between feature-freeze and
release, I'd like to add even more tests, and not just to pg_dump but
also to other commands that don't have very good testing today (psql, in
particular, but pg_dumpall needs more also, and there's more to do with
pg_dump too).

I certainly understand the concern raised about making the buildfarm go
red right before a release, though I believe that's now been dealt with.
When it comes to packaging, if adding tests using the existing test
infrastructure (the TAP system) causes a problem there, then I think we
need to address that issue rather than not add new tests.  Packagers
also always have the option to not enable the tap tests, if there really
is an issue there which can't be addressed.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] [sqlsmith] Failed assertion in parallel worker (ExecInitSubPlan)

[Amit Kapila]

On Fri, May 6, 2016 at 8:45 AM, Tom Lane  wrote:
>
> Andreas Seltenreich  writes:
> > when fuzz testing master as of c1543a8, parallel workers trigger the
> > following assertion in ExecInitSubPlan every couple hours.
> > TRAP: FailedAssertion("!(list != ((List *) ((void *)0)))", File:
"list.c", Line: 390)
> > Sample backtraces of a worker and leader below, plan of leader attached.
> > The collected queries don't seem to reproduce it.
>
> Odd.  My understanding of the restrictions on parallel query is that
> anything involving a SubPlan ought not be parallelized;
>

Subplan references are considered parallel-restricted, so parallel plan can
be generated if there are subplans in a query, but they shouldn't be pushed
to workers.  I have tried a somewhat simpler example to see if we pushdown
anything parallel restricted to worker in case of joins and it turned out
there are cases when that can happen.  Consider below example:

create or replace function parallel_func_select() returns integer
as $$
declare
ret_val int;
begin
 ret_val := 1000;
 return ret_val;
end;
$$ language plpgsql Parallel Restricted;

CREATE TABLE t1(c1, c2) AS SELECT g, repeat('x', 5) FROM
generate_series(1, 1000) g;

CREATE TABLE t2(c1, c2) AS SELECT g, repeat('x', 5) FROM
generate_series(1, 100) g;

Explain Verbose SELECT t1.c1 + parallel_func_select(), t2.c1 FROM t1 JOIN
t2 ON t1.c1 = t2.c1;

   QUERY PLAN



 Gather  (cost=32813.00..537284.53 rows=100 width=8)
   Output: ((t1.c1 + parallel_func_select())), t2.c1
   Workers Planned: 2
   ->  Hash Join  (cost=31813.00..436284.53 rows=100 width=8)
 Output: (t1.c1 + parallel_func_select()), t2.c1
 Hash Cond: (t1.c1 = t2.c1)
 ->  Parallel Seq Scan on public.t1  (cost=0.00..95721.08
rows=4166608 w
idth=4)
   Output: t1.c1, t1.c2
 ->  Hash  (cost=15406.00..15406.00 rows=100 width=4)
   Output: t2.c1
   ->  Seq Scan on public.t2  (cost=0.00..15406.00 rows=100
widt
h=4)
 Output: t2.c1
(12 rows)


>From the above output it is clear that parallel restricted function is
pushed down below gather node.  I found that though we have have care fully
avoided to push pathtarget below GatherPath in apply_projection_to_path()
if pathtarget contains any parallel unsafe or parallel restricted clause,
but we are separately also trying to apply pathtarget to partialpath list
which doesn't seem to be the correct way even if it is required.  I think
this has been added during parallel aggregate patch and it seems to me this
is not required after the changes related to GatherPath in
apply_projection_to_path().

After applying the attached patch, it avoids to add parallel restricted
clauses below gather path.

Now back to the original bug, if you notice in plan file attached in
original bug report, the subplan is pushed below Gather node in target
list, but not to immediate join, rather at one more level down to SeqScan
path.  I am still not sure how it has manage to push the restricted clauses
to that down the level.

Thoughts?

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


avoid_restricted_clause_below_gather_v1.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] pg9.6 segfault using simple query (related to use fk for join estimates)

[Simon Riggs]

On 6 May 2016 at 01:00, Tomas Vondra  wrote:


> I think we can further limit the impact by ignoring foreign keys on a
> single column, because the primary goal of the patch is improving estimates
> with multi-column FKs (and matching joins). I'd argue that 99% of the FKs
> in practice is single-column ones, but sure - if you have a database with
> many multi-column FKs this won't help.
>
> I think it's also important to point out that whatever solution we choose,
> it should probably allow us to relax some of the restrictions in the
> future. For example, with a FK on 3 columns, the current patch only handles
> a "full match" joins, with conditions on all three columns. But it may be
> possible to also improve estimates on just two columns - the current patch
> does not do that, as that needs definitely further more thought and
> discussion.
>

The context of the patch is important. If we can save this for 9.6, we
should.

Multi-column joins are currently badly estimated. Multi-variate States
would be useful here also, but I chose not to commit that patch since it
was much larger. The current patch is small, isolated and yet very
effective in solving the estimation problem in some cases. Committing and
possibly reverting the feature was/is possible without collateral damage.

The current patch works only for multi-column joins, matching them against
multi-column FKs. I see no reason why that should give a massively negative
performance because that is a small subset of cases; I did test that to
check for regressions, but its possible I missed something that does cause
additional run-time in real world cases.

Clearly, an optimizer test suite would be helpful in analyzing the effect
of new patches.

I'll continue to look at this and comment further.

-- 
Simon Riggshttp://www.2ndQuadrant.com/

PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] pg9.6 segfault using simple query (related to use fk for join estimates)

[Simon Riggs]

On 3 May 2016 at 16:10, Tomas Vondra  wrote:


> While this in itself is about a two-line fix, after reviewing
>> 137805f89acb3611 I'm pretty unhappy that it got committed at all.
>> I think this obvious bug is a good sign that it wasn't ready.
>> Other unfinished aspects like invention of an undocumented GUC
>> don't leave a good impression either.
>>
>
> The GUC is meant to make testing during development easier. I haven't
> realized it got committed, but I assume Simon plans to remove it before the
> final release.


Yes, the GUC was for testing, as mentioned on the commit message.


> Can't check right now with Simon, though, as he's is out of office this
> week.
>

Am back and reading.

-- 
Simon Riggshttp://www.2ndQuadrant.com/

PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] ALTER TABLE lock downgrades have broken pg_upgrade

[Simon Riggs]

On 3 May 2016 at 18:07, Tom Lane  wrote:


> Or at least, it did until Simon decided that ALTER TABLE RESET
> doesn't require AccessExclusiveLock.


On reflection, this still seems like a good idea.


> Now you get a failure.
>

Failure condition as an exception to that.


> I haven't tried to construct a pre-9.1 database that would trigger
> this, but you can make it happen by applying the attached patch
> to create a toast-table-less table in the regression tests,
> and then doing "make check" in src/bin/pg_upgrade.  You get this:
>
> ...
> Restoring database schemas in the new cluster
> ok
> Creating newly-required TOAST tablesSQL command
> failed
> ALTER TABLE "public"."i_once_had_a_toast_table" RESET
> (binary_upgrade_dummy_option);
> ERROR:  AccessExclusiveLock required to add toast table.
>
> Failure, exiting
>

It appears that pg_upgrade is depending upon an undocumented side-effect of
ALTER TABLE RESET.

I would say this side-effect should not exist, which IIUC is the same
conclusion on your latest post.

If pg_upgrade needs this, we should implement a specific function that does
what pg_upgrade needs. That way we can isolate the requirement for an
AccessExclusiveLock to the place that needs it: pg_upgrade. That will also
make it less fragile in the future. I don't think that needs a specific
command, just a function.

I accept that it is my bug and should fix it.

-- 
Simon Riggshttp://www.2ndQuadrant.com/

PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services