Re: [HACKERS] contrib/bloom wal-check not run by default

[Michael Paquier]

On Tue, Apr 11, 2017 at 12:14 AM, Peter Eisentraut
 wrote:
> Why $subject?
>
> Does it just need to be wired into the makefiles a bit better?

Looks like an oversight to me. I would suggest changing the Makefile like that:
diff --git a/contrib/bloom/Makefile b/contrib/bloom/Makefile
index 13bd397b70..834ab18bdc 100644
--- a/contrib/bloom/Makefile
+++ b/contrib/bloom/Makefile
@@ -20,5 +20,7 @@ include $(top_builddir)/src/Makefile.global
 include $(top_srcdir)/contrib/contrib-global.mk
 endif

-wal-check: temp-install
+check: prove-check
+
+prove-check:
$(prove_check)
-- 
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] Implementation of SASLprep for SCRAM-SHA-256

[Michael Paquier]

On Mon, Apr 10, 2017 at 5:11 PM, Heikki Linnakangas  wrote:
> Here are some characters that seem plausible to be misinterpreted and
> ignored by SASLprep:
> EUC-JP and EUC-JISX0213:
>
> U+00AD (C2 AD): 足 (meaning "foot", per Unihan database)
> U+FE00-FE0F (EF B8 8X): 鏝 (meaning "trowel", per Unihan database)

Those are common words, but I still have to see those characters used
in passwords. For user names, I have seen games or apps using Kanjis,
so they could be used in such cases. But any application can be
careful in controlling the encoding used by the client, so I would not
worry much about them.

> EUC-CN:
>
> U+00AD (C2 AD): 颅 (meaning "skull", per Unihan database)
> U+FE00-FE0FF (EF B8 8X): 锔 (meaning "curium", per Unihan database)
> U+FEFF (EF BB BF): 锘 (meaning "nobelium", per Wikipedia)
>
> EUC-KR:
>
> U+FE00-FE0F (EF BB BF): 截 (meanings "cut off, stop, obstruct, intersect",
> per Unihan database
> U+FEFF (EF BB BF): 癤 (meanings "pimple, sore, boil", per Unihan database)
>
> EUC-TW:
> U+FE00-FE0F: 踫 (meanings "collide, bump into", per Unihan database)
> U+FEFF: 踢  (meaning "kick", per Unihan database)

Not completely sure about those ones. At least I think that the two
set of characters of Chinese Taipei are pretty common there.

> CP866:
> U+1806: саЖ
> U+180B: саЛ
> U+180C: саМ
> U+180D: саН
> U+200B: тАЛ
> U+200C: тАМ
> U+200D: тАН
> ---
>
> The CP866 cases seem most likely to cause confusion.

No idea about those.

> Those are all common
> words in Russian. I don't know how common those Chinese/Japanese characters 
> are.
>
> Overall, I think this is OK. Even though there are those characters that can
> be misinterpreted, for it to be problem all of the following have to be
> true:
>
> 1. The client is using one of those encodings.
> 2. The password string as whole has to look like valid UTF-8.
> 3. Ignoring those characters/words from the password would lead to a
> significantly weaker password, i.e. it was not very long to begin with, or
> it consisted almost entirely of those characters/words.
>
> Thoughts? Attached is the full results of running iconv with each encoding,
> from which I picked the above cases.

I am not much worrying about such things either. Still I am wondering
though if it would be useful to give users the possibility to block
connection attempts from clients that do not use UTF-8 in this case.
Some people use open instances.
-- 
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] SCRAM authentication, take three

[Heikki Linnakangas]

On 04/11/2017 04:52 AM, Peter Eisentraut wrote:

On 4/10/17 04:27, Heikki Linnakangas wrote:

One thing to consider is that we just made the decision that "md5"
actually means "md5 or scram-sha-256". Extrapolating from that, I think
we'll want "scram-sha-256" to mean "scram-sha-256 or scram-sha-256-plus"
(i.e. the channel-bonding variant) in the future. And if we get support
for scram-sha-512, "scram-sha-256" would presumably allow that too.


But how would you choose between scram-sha-256-plus and scram-sha-512?


Good question. We would need to decide the order of preference for those.

That question won't arise in practice. Firstly, if the server can do 
scram-sha-256-plus, it presumably can also do scram-sha-512-plus. Unless 
there's a change in the way the channel binding works, such that the 
scram-sha-512-plus variant needs a newer version of OpenSSL or 
something. Secondly, the user's pg_authid row will contain a 
SCRAM-SHA-256 or SCRAM-SHA-512 verifier, not both, so that will dictate 
which one to use.


- Heikki



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

[HACKERS] Host variables corresponding bytea type in ecpg

[Kato, Sho]

Hi,

I tried to use the bytea type with ecpg, but the corresponding host variable 
was not described in the manual.

https://www.postgresql.org/docs/devel/static/ecpg-variables.html#ecpg-variables-type-mapping

According to the test code, the bytea type seems to correspond to char *.

  src/interfaces/ecpg/test/sql/binary.pgc 

I think that it is better to describe that the host variable corresponding to 
the bytea type is char * in the manual, but what about it?
I attach modified patch.

regards, Sho Kato.
--
Kato Sho



fix_ecpg_host_variables_in_doc_v1.patch
Description: fix_ecpg_host_variables_in_doc_v1.patch

-- 
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] pgbench --progress-timestamp no longer works correctly

[Noah Misch]

On Fri, Apr 07, 2017 at 09:58:07AM -0700, Jeff Janes wrote:
> --progress-timestamp is supposed to make -P report a Unix Epoch time stamp,
> for easy correlation with the entries in other log files (like the postgres
> server log file using %n).
> 
> But that broke in this commit:
> 
> commit 1d63f7d2d180c8708bc12710254eb7b45823440f
> Author: Tom Lane 
> Date:   Mon Jan 2 13:41:51 2017 -0500
> 
> Use clock_gettime(), if available, in instr_time measurements.
> 
> 
> The commit before that one changed pgbench to make it tolerate the change
> in clock, but it overlooked --progress-timestamp.

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

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

[1] 
https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com


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

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

[Michael Paquier]

On Tue, Apr 11, 2017 at 4:41 AM, Heikki Linnakangas  wrote:
> On 04/10/2017 09:33 PM, Álvaro Hernández Tortosa wrote:
>> - If the username used is the one sent in the startup message, rather
>> than leaving it empty in the client-first-message, I would force it to
>> be the same as the used in the startuo message.
>
> The problem with that is that the SCRAM spec dictates that the username must
> be encoded in UTF-8, but PostgreSQL supports non-UTF-8 usernames.
>
> Or did you mean that, if the username is sent, it must match the one in the
> startup packet, but an empty string would always be allowed? That would be
> reasonable.

That sounds sensible from here.

>> Otherwise we may confuse
>> some client implementations which would probably consider that as an
>> error; for one, my implementation would currently throw an error if
>> username is empty, and I think that's correct.
>
> I'm not sure I follow. The username is sent from client to server, and
> currently, the server will ignore it. If you're writing a client library, it
> can send whatever it wants. (Although again I would recommend an empty
> string, to avoid confusion. Sending the same username as in the startup
> packet, as long as it's in UTF-8, seems reasonable too.)
>
> Thanks for reviewing this! I'll start hacking on code changes to go with
> these docs.

Sounds good to me, thanks! I looked at the doc patch for now, and here
are some nits.

+  
+SCRAM-SHA-256 is the only implemented SASL mechanism, at the moment. It is
+described in detail in [RFC7677] and [RFC5741]. (called just
SCRAM from now on)
+  
Perhaps those should be links to the RFCs.

+
+  Client sends a SASLResponse messsage, with SCRAM
+  client-final-message as the content.
+
Typo. Message needs two 's'.

+  server-final-message in the "additional data" field.
+
+
+
+
I think that you are missing a  markup here.

+
+
+Authentication method specific "additional data". If this
+message contains no additional data, this field is omitted.
+
+
So that's for the first message generated by the client in the
exchange. Better than my previous idea. Shouldn't double quotes be
replaced by proper markups?
-- 
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] Re: pgsql: Use SASLprep to normalize passwords for SCRAM authentication.

[Noah Misch]

On Fri, Apr 07, 2017 at 11:58:10AM +, Heikki Linnakangas wrote:
> No documentation changes included, because there is no details on the
> SCRAM mechanism in the docs anyway. An overview on that in the protocol
> specification would probably be good, even though SCRAM is documented in
> detail in RFC5802. I'll write that as a separate patch. An important thing
> to mention there is that we apply SASLprep even on invalid UTF-8 strings,
> to support other encodings.

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

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

[1] 
https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com


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

Re: [HACKERS] Ongoing issues with representation of empty arrays

[Andrew Gierth]

> "Tom" == Tom Lane  writes:

 >> First is contrib/intarray, _AGAIN_ (see past bugs such as #7730):
 >> ...
 >> I plan to fix this one properly, unless anyone has any objections.

 Tom> Just to clarify, what do you think is "properly"?

I would say, that any time an intarray function returns an empty result
it should be the standard 0-dimensional representation that every other
array operation uses.  The intarray functions all seem already able to
take such values as inputs.  Also there should be regression tests for
this (none of intarray's existing tests have any empty arrays at all).

 >> Second is aclitem[], past bug #8395 which was not really resolved; empty
 >> ACLs are actually 1-dim arrays of length 0, and all the ACL functions
 >> insist on that, which means that you can't call aclexplode('{}') for
 >> example:
 >> It's much less clear what to do about this one. Thoughts? 

 Tom> My instinct is to be conservative in what you send and liberal in
 Tom> what you accept.  In this context that would probably mean fixing
 Tom> aclitem-related functions to accept both 0-dimensional and
 Tom> 1-dimensional-0-length inputs.
 
 Tom> (Actually, is there a reason to be picky about the input
 Tom> dimensionality at all, rather than just iterating over whatever
 Tom> the array contents are?)

Currently there is this:

#define ACL_NUM(ACL)(ARR_DIMS(ACL)[0])

which is obviously wrong for dimensions other than exactly 1.  I don't
_think_ there's any great obstacle to fixing this; the only code that
would care about number of dimensions would be allocacl, and since
there'd be no obvious reason to preserve the shape of an aclitem[]
anywhere, that could just do 0 or 1 dimensions.

-- 
Andrew (irc:RhodiumToad)


-- 
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] Ongoing issues with representation of empty arrays

[Tom Lane]

Andrew Gierth  writes:
> The distinction between the standard representation of '{}' as an array
> with zero dimensions and nonstandard representations as a 1-dimensional
> array with zero elements has come up in a couple of contexts on the IRC
> channel recently.

> First is contrib/intarray, _AGAIN_ (see past bugs such as #7730):
> ...
> I plan to fix this one properly, unless anyone has any objections.

Just to clarify, what do you think is "properly"?

> Second is aclitem[], past bug #8395 which was not really resolved; empty
> ACLs are actually 1-dim arrays of length 0, and all the ACL functions
> insist on that, which means that you can't call aclexplode('{}') for
> example:
> It's much less clear what to do about this one. Thoughts? 

My instinct is to be conservative in what you send and liberal in what you
accept.  In this context that would probably mean fixing aclitem-related
functions to accept both 0-dimensional and 1-dimensional-0-length inputs.
(Actually, is there a reason to be picky about the input dimensionality at
all, rather than just iterating over whatever the array contents are?)
Also, if we've got any functions that create aclitem[] values, fix them to
generate the standard representation of empty arrays.

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

[HACKERS] PostGIS Raster - Loading MrSID format

[Osahon Oduware]

Hi,

I am trying to use the raster2pgsql tool to import raster images in MrSID
format to PostGIS database on a Windows system. However, I get an error
indicating that the format is not supported which I could confirm by
running:
raster2pgsql -G

Please, could someone help with the basic steps to follow to load raster
data in MrSID format using the raster2pgsql tool.

Re: [HACKERS] error handling in RegisterBackgroundWorker

[Tom Lane]

Noah Misch  writes:
> On Mon, Apr 10, 2017 at 09:36:59PM -0400, Peter Eisentraut wrote:
>> If history had been different, we could have implemented, say,
>> autovacuum or walreceiver on the background worker framework.  I think
>> unifying some of that might actually be a future project.  Would it be
>> OK if these processes just logged a warning and didn't start if there
>> was a misconfiguration?

> No.  I can't think of any background worker so unimportant that I'd want the
> warning only.  Certainly, then, the ones you list are far too important.

Well, I dunno.  We allow the system to start without a functioning stats
collector (cf what happens if we fail to create a working loopback
socket).  But lack of stats will effectively disable autovacuum.  So at
the very least we have some inconsistent decisions here.

Personally I'd err on the side of "starting up degraded is better than
not starting at all".  Or maybe we should invent a GUC to let DBAs
express their preference on that?

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: [pgsql-www] [HACKERS] Small issue in online devel documentation build

[Peter Eisentraut]

On 4/8/17 12:50, Robert Haas wrote:
> On Sat, Apr 8, 2017 at 6:39 AM, Bruce Momjian  wrote:
>> What other problems do we have with pgweb that I can work on?
> 
> Well, the 10devel documentation doesn't believe in orange.  Compare:
> 
> https://www.postgresql.org/docs/devel/static/sql-createtable.html
> https://www.postgresql.org/docs/9.6/static/sql-createtable.html
> 
> I think that needs to be fixed.

Patch for that attached.

Also patch for improved note/warning/etc. formatting.

Another issue I saw is that tables no longer get the fancy style, e.g.,
https://www.postgresql.org/docs/devel/static/sql-syntax-lexical.html.
The style selector is table.table, which is what the build produces, but
somehow the class attribute no longer shows up on the web site.

Along with the nested  issue, which is already being discussed,
these are all the issues I'm aware of.

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


0001-Make-h2-orange-in-the-documentation.patch
Description: invalid/octet-stream


0002-Improve-admonitions-style.patch
Description: invalid/octet-stream

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

[HACKERS] PostGIS Out-DB Raster Not Behaving As Expected

[Osahon Oduware]

Hi All,

I created an out-db raster using the following syntax:

raster2pgsql -s {srid} -c -R -I -C -F -t auto {absolute_file_path}
public.{table} | psql -h {host} -p {port} -d {database} -U {user}

The table was created successfully. I wanted to confirm that the actual
file is being read from the location in the filesystem by performing the
following steps:
1) I moved the raster file to a different location.
2) I opened QGIS and attempted to load the raster from PostGIS table.

I was surprised that QGIS could load the file. *How is this possible when
the actual raster data is not stored in the database table?*

Re: [HACKERS] src/interfaces/libpq shipping nmake-related Makefiles

[Michael Paquier]

On Tue, Apr 11, 2017 at 1:45 AM, Tom Lane  wrote:
> Magnus Hagander  writes:
> Are these votes for getting rid of both win32.mak and bcc32.mak?
>
>> PFA a patch that does this. Did I miss something? :)
>
> Perhaps we should get rid of the WIN32_ONLY_COMPILER symbol altogether;
> given this patch, "#ifdef WIN32_ONLY_COMPILER" could be replaced by
> "#ifdef _MSC_VER".

That's here in the patch for people wondering:
-#if defined(_MSC_VER) || defined(__BORLANDC__)
+#if defined(_MSC_VER)
 #define WIN32_ONLY_COMPILER
 #endif
+1 for the renaming.

> Or maybe rename it to something else --- I'm not sure what, but
> I've always found that symbol rather confusing.

No complains here as well. Thanks for the patch.
-- 
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] error handling in RegisterBackgroundWorker

[Noah Misch]

On Mon, Apr 10, 2017 at 09:36:59PM -0400, Peter Eisentraut wrote:
> On 4/9/17 22:40, Noah Misch wrote:
> > Agreed.  There are times when starting up degraded beats failing to start,
> > particularly when the failing component has complicated dependencies.  In 
> > this
> > case, as detailed upthread, this can only fail in response to basic
> > misconfiguration.  It's not the kind of thing that will spontaneously fail
> > after a minor upgrade, for example.
> 
> If history had been different, we could have implemented, say,
> autovacuum or walreceiver on the background worker framework.  I think
> unifying some of that might actually be a future project.  Would it be
> OK if these processes just logged a warning and didn't start if there
> was a misconfiguration?

No.  I can't think of any background worker so unimportant that I'd want the
warning only.  Certainly, then, the ones you list are far too important.


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

Re: [HACKERS] SUBSCRIPTIONS and pg_upgrade

[Stephen Frost]

Peter,

* Peter Eisentraut (peter.eisentr...@2ndquadrant.com) wrote:
> On 4/10/17 20:55, Stephen Frost wrote:
> > * Peter Eisentraut (peter.eisentr...@2ndquadrant.com) wrote:
> >> Problem 1: pg_subscription.subconninfo can only be read by superuser.
> >> So non-superusers cannot dump subscriptions.
> > 
> > I'm not particularly happy with this.
> 
> Why?  How?  Alternatives?

Generally speaking, we should be trying to move away from superuser-only
anything, not introducing more of it.  If the connection string can have
sensitive data in it, I'd be happier if we could pull that sensitive
information out into its own column to allow the rest to be included,
but if we have to exclude the connection string then we have to, but the
rest should be available to individuals with appropriate rights.  That
doesn't necessairly mean "public" should be allowed to see it, but we
should have a role who can who isn't superuser.

> >> Precedent is pg_user_mapping.  In that case, we just omit the
> >> user-mapping options if we're not a superuser.  Pretty dubious, but in
> >> any case that won't work here, because you cannot create a subscription
> >> without a CONNECTION clause.
> > 
> > Given that you can create a disabled subscription, why is a connection
> > clause required..?  I agree that simply excluding pg_user_mapping isn't
> > great but I don't really think we want to use something which we agree
> > isn't ideal as the best approach for this.
> 
> Well, I suppose you could somehow make it work so that a connection
> clause is not required.  But then why dump the subscription at all?  You
> start stripping off information and it becomes less useful.

Isn't there other information associated with the subscription beyond
just the connection string?  Even if there isn't today, I certainly
expect there will be in the future (at a minimum, mapping between the
foreign table and a local table with a different name really should be
supported...).

Consider the recently added option to not include passwords for roles
being dumped through pg_dumpall.  That's useful, for multiple reasons,
even if the roles don't have any objects which depend on them.
Including this information is required to rebuild the system as closely
as possible to the original state.

> >> Proposal: Dump subscriptions if running as superuser.  If not, check if
> >> there are subscriptions and warn about that.  Remove current pg_dump
> >> --include-subscriptions option.
> > 
> > That option was added, iirc, in part because pg_dump was including
> > subscriptions in things like explicit single-table dumps.
> 
> No, you are thinking of publications.

Even so, the other comments apply equally here, from what I can tell.

> The option was added because at some point during the review process of
> the early patches, pg_dump for a non-superuser would just fail outright
> as it was trying to read pg_subscription.

That's certainly an issue in general.

> > The question becomes if it's useful to include subscriptions when only
> > dumping a subset of objects or if they should *only* be included when
> > doing a full dump.
> 
> I think we'd handle that similar to publications.

Which is to say that they'd only be included in a 'full' dump?  I'd like
a way to do better than that in general, but having them only included
in full dumps would also be an option, at least for PG10.

> >> Proposal: Dump all subscriptions in DISABLED state.  Remove current
> >> pg_dump --no-subscription-connect option.
> > 
> > I like this idea in general, I'm just not sure if it's the right answer
> > when we're talking about pg_upgrade.  At a minimum, if we go with this
> > approach, we should produce a warning when subscriptions exist during
> > the pg_upgrade that the user will need to go re-enable them.
> 
> Sure, that's doable.  It's the way of pg_upgrade in general to give you
> a bunch of notes and scripts afterwards, so it wouldn't be too strange
> to add that in.

Right, that's why I was suggesting it.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] SCRAM authentication, take three

[Peter Eisentraut]

On 4/10/17 04:27, Heikki Linnakangas wrote:
> One thing to consider is that we just made the decision that "md5" 
> actually means "md5 or scram-sha-256". Extrapolating from that, I think 
> we'll want "scram-sha-256" to mean "scram-sha-256 or scram-sha-256-plus" 
> (i.e. the channel-bonding variant) in the future. And if we get support 
> for scram-sha-512, "scram-sha-256" would presumably allow that too.

But how would you choose between scram-sha-256-plus and scram-sha-512?

-- 
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] Ongoing issues with representation of empty arrays

[David G. Johnston]

On Mon, Apr 10, 2017 at 5:57 PM, Andrew Gierth 
wrote:

> Second is aclitem[], past bug #8395 which was not really resolved; empty
> ACLs are actually 1-dim arrays of length 0, and all the ACL functions
> insist on that, which means that you can't call aclexplode('{}') for
> example:
>
> https://www.postgresql.org/message-id/flat/CA%
> 2BTgmoZdDpTJDUVsgzRhoCctidUqLDyO8bdYwgLD5p8DwHtMcQ%40mail.gmail.com
>
> It's much less clear what to do about this one. Thoughts?
>

​After a quick Google search to get a feel for the landscape...​

At a high level the Access Control System is a high-risk area - it seems
like the above anomaly doesn't outweigh the risk of slogging through and
changing the mechanics.  But maybe I'm being overly paranoid in my
inexperience...

The question I'd have is whether people are doing '{}'::aclitem[] out of
habit or because there is no other way, in SQL, to generate an empty ACL
array?  If its the later we probably should supply one even if its not
documented for external use; none of these other functions are.

I.e., a no-arg "newacl" function where:  array_length(newacl(), 1) => 0

IOW - the response to Bug # 8395 is to make #3 workable with the correct
syntax.  That #2 would continue to return false is maybe annoying but
unless we are prepared (and capable) of making '{}'::aclitem[] an error I
don't see that we should worry about it.

David J.

p.s. A side question is whether a handful of reports and a couple of PGXN
extensions suggest that we should consider user-facing documenting some of
these things.

Re: [HACKERS] SCRAM authentication, take three

[Peter Eisentraut]

On 4/9/17 19:19, Noah Misch wrote:
> These are the two chief approaches I'm seeing:
> 
> 1. scram-sha-256, scram-sha-256-plus, and successors will be their own
>pg_hba.conf authentication methods.  Until and unless someone implements an
>ability to name multiple methods per HBA line, you must choose exactly one
>SASL method.  The concrete work for v10 would be merely renaming "scram" to
>"scram-sha-256".

I like that.

> 2. Create a multiplexed authentication method like "sasl" or "scram" (not to
>be confused with today's "scram" method, which denotes SCRAM-SHA-256
>precisely).  The DBA permits concrete methods like scram-sha-256 via HBA
>option.  Absent that option, the system could default to a reasonable list.

The problem with that approach is that you would then eventually need
yet another place like pg_hba.conf to configure which SASL mechanisms to
use under which circumstances.  pg_hba.conf is already that place for
the Legacy Authentication and Security Layer, so it could be that place
for SASL as well.

-- 
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] Repetitive code in RI triggers

[Peter Eisentraut]

On 4/10/17 11:55, Ildar Musin wrote:
> I was looking through the RI triggers code recently and noticed a few 
> almost identical functions, e.g. ri_restrict_upd() and 
> ri_restrict_del(). The following patch is an attempt to reduce some of 
> repetitive code.

That looks like something worth pursuing.  Please add it to the next
commit fest.

-- 
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] error handling in RegisterBackgroundWorker

[Peter Eisentraut]

On 4/9/17 22:40, Noah Misch wrote:
> Agreed.  There are times when starting up degraded beats failing to start,
> particularly when the failing component has complicated dependencies.  In this
> case, as detailed upthread, this can only fail in response to basic
> misconfiguration.  It's not the kind of thing that will spontaneously fail
> after a minor upgrade, for example.

If history had been different, we could have implemented, say,
autovacuum or walreceiver on the background worker framework.  I think
unifying some of that might actually be a future project.  Would it be
OK if these processes just logged a warning and didn't start if there
was a misconfiguration?

-- 
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] [PATCH v1] Add and report the new "in_hot_standby" GUC pseudo-variable.

[Michael Paquier]

On Mon, Apr 10, 2017 at 5:47 PM, Magnus Hagander  wrote:
> Based on that we seem to agree here, should we add this as an open item?
> Clearly if we want to change this, we should do so before 10.

This really is a new feature, so as the focus is to stabilize things I
think that we should not make the code more complicated because...

> I also came up with another case where the current one won't work but it
> could be really useful -- if we make a replication connection (with say
> pg_receivewal) it would be good to be able to say "i want the master" (or "i
> want a standby") the same way. And that will fail today if it needs SHOW to
> work, right?
>
> So having it send that information across in the startup package when
> talking to a 10 server, but falling back to using SHOW if talking to an
> earlier server, would make a lot of sense I think.

Of this reason, as libpq needs to be compliant with past server
versions as well we are never going to save a set of version-dependent
if/else code to handle target_session_attrs properly using either a
SHOW or a new mechanism.
-- 
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] SUBSCRIPTIONS and pg_upgrade

[Peter Eisentraut]

On 4/10/17 20:55, Stephen Frost wrote:
> Peter,
> 
> * Peter Eisentraut (peter.eisentr...@2ndquadrant.com) wrote:
>> Problem 1: pg_subscription.subconninfo can only be read by superuser.
>> So non-superusers cannot dump subscriptions.
> 
> I'm not particularly happy with this.

Why?  How?  Alternatives?

>> Precedent is pg_user_mapping.  In that case, we just omit the
>> user-mapping options if we're not a superuser.  Pretty dubious, but in
>> any case that won't work here, because you cannot create a subscription
>> without a CONNECTION clause.
> 
> Given that you can create a disabled subscription, why is a connection
> clause required..?  I agree that simply excluding pg_user_mapping isn't
> great but I don't really think we want to use something which we agree
> isn't ideal as the best approach for this.

Well, I suppose you could somehow make it work so that a connection
clause is not required.  But then why dump the subscription at all?  You
start stripping off information and it becomes less useful.

>> Proposal: Dump subscriptions if running as superuser.  If not, check if
>> there are subscriptions and warn about that.  Remove current pg_dump
>> --include-subscriptions option.
> 
> That option was added, iirc, in part because pg_dump was including
> subscriptions in things like explicit single-table dumps.

No, you are thinking of publications.

The option was added because at some point during the review process of
the early patches, pg_dump for a non-superuser would just fail outright
as it was trying to read pg_subscription.

> The question becomes if it's useful to include subscriptions when only
> dumping a subset of objects or if they should *only* be included when
> doing a full dump.

I think we'd handle that similar to publications.

>> Proposal: Dump all subscriptions in DISABLED state.  Remove current
>> pg_dump --no-subscription-connect option.
> 
> I like this idea in general, I'm just not sure if it's the right answer
> when we're talking about pg_upgrade.  At a minimum, if we go with this
> approach, we should produce a warning when subscriptions exist during
> the pg_upgrade that the user will need to go re-enable them.

Sure, that's doable.  It's the way of pg_upgrade in general to give you
a bunch of notes and scripts afterwards, so it wouldn't be too strange
to add that in.

-- 
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] Ongoing issues with representation of empty arrays

[Andrew Gierth]

The distinction between the standard representation of '{}' as an array
with zero dimensions and nonstandard representations as a 1-dimensional
array with zero elements has come up in a couple of contexts on the IRC
channel recently.

First is contrib/intarray, _AGAIN_ (see past bugs such as #7730):

select array_dims(('{1,2}'::integer[] & '{3}'));
 array_dims 

 [1:0]
(1 row)

regression=# select ('{1,2}'::integer[] & '{3}') = '{}';
 ?column? 
--
 f
(1 row)

Worse, the fact that the fix for #7730 (commit c155f654) only did a
very partial job means that it's now inconsistent:

regression=# select (a - b), (a & c), (a - b) = (a & c)
 from (values (array[1,2],array[1,2],array[3])) v(a,b,c);
 ?column? | ?column? | ?column? 
--+--+--
 {}   | {}   | f
(1 row)

I plan to fix this one properly, unless anyone has any objections.


Second is aclitem[], past bug #8395 which was not really resolved; empty
ACLs are actually 1-dim arrays of length 0, and all the ACL functions
insist on that, which means that you can't call aclexplode('{}') for
example:

https://www.postgresql.org/message-id/flat/CA%2BTgmoZdDpTJDUVsgzRhoCctidUqLDyO8bdYwgLD5p8DwHtMcQ%40mail.gmail.com

It's much less clear what to do about this one. Thoughts? 

-- 
Andrew (irc:RhodiumToad)


-- 
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] WAL logging problem in 9.4.3?

[Kyotaro HORIGUCHI]

Hello, thank you for looking this.

At Fri, 07 Apr 2017 20:38:35 -0400, Tom Lane  wrote in 
<27309.1491611...@sss.pgh.pa.us>
> Alvaro Herrera  writes:
> > Interesting.  I wonder if it's possible that a relcache invalidation
> > would cause these values to get lost for some reason, because that would
> > be dangerous.
> 
> > I suppose the rationale is that this shouldn't happen because any
> > operation that does things this way must hold an exclusive lock on the
> > relation.  But that doesn't guarantee that the relcache entry is
> > completely stable,
> 
> It ABSOLUTELY is not safe.  Relcache flushes can happen regardless of
> how strong a lock you hold.
> 
>   regards, tom lane

Ugh. Yes, relcache invalidation happens anytime and it resets the
added values. pg_stat_info deceived me that it can store
transient values. But I  came up with another thought.

The reason I proposed it was I thought that hash_search for every
buffer is not good. Instead, like pg_stat_info, we can link the
pending-sync hash entry to Relation. This greately reduces the
frequency of hash-searching.

I'll post new patch in this way soon.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center



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

Re: [HACKERS] SUBSCRIPTIONS and pg_upgrade

[Stephen Frost]

Peter,

* Peter Eisentraut (peter.eisentr...@2ndquadrant.com) wrote:
> Problem 1: pg_subscription.subconninfo can only be read by superuser.
> So non-superusers cannot dump subscriptions.

I'm not particularly happy with this.

> Precedent is pg_user_mapping.  In that case, we just omit the
> user-mapping options if we're not a superuser.  Pretty dubious, but in
> any case that won't work here, because you cannot create a subscription
> without a CONNECTION clause.

Given that you can create a disabled subscription, why is a connection
clause required..?  I agree that simply excluding pg_user_mapping isn't
great but I don't really think we want to use something which we agree
isn't ideal as the best approach for this.

> Proposal: Dump subscriptions if running as superuser.  If not, check if
> there are subscriptions and warn about that.  Remove current pg_dump
> --include-subscriptions option.

That option was added, iirc, in part because pg_dump was including
subscriptions in things like explicit single-table dumps.  I certainly
don't think we should start including all subscriptions in all dumps.

The question becomes if it's useful to include subscriptions when only
dumping a subset of objects or if they should *only* be included when
doing a full dump.  My gut feeling is that if we care enough about blobs
to have an explicit option to include them, even when we aren't dumping
out everything, then having something similar for subscriptions makes
sense.

> Problem 2: Restoring a subscription immediately starts replication.
> Maybe you want that or maybe you don't.  We could dump all subscriptions
> in DISABLED state.  But then after restoring you have to go and manually
> enable all subscriptions.  We don't have a command to turn all
> subscriptions back on at once.  Maybe that is not a terrible issue,
> since one wouldn't normally have many subscriptions.

Having a way to turn them all on would be nice.

> Proposal: Dump all subscriptions in DISABLED state.  Remove current
> pg_dump --no-subscription-connect option.

I like this idea in general, I'm just not sure if it's the right answer
when we're talking about pg_upgrade.  At a minimum, if we go with this
approach, we should produce a warning when subscriptions exist during
the pg_upgrade that the user will need to go re-enable them.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] ExecPrepareExprList and per-query context

[Amit Langote]

On 2017/04/08 1:49, Tom Lane wrote:
> Amit Langote  writes:
>> Should ExecPrepareExprList also switch to estate->es_query_cxt?
> 
> Good point; I'm surprised we haven't noted any failures from that.
> We surely want the entire result data structure to be in the same
> memory context.  There are not very many callers right now, and
> I guess they are all in the right context already (or we aren't
> testing them :-().

Thanks for the fix.

>> Or maybe
>> ExecPrepareExpr could itself detect that passed-in node is a List and
>> create the list of ExprState nodes by itself.
> 
> -1.  That's just breaking the API of ExecPrepareExpr.

I guess you're right.  I was just thinking that passing a List through
ExecPrepareExpr() used to work and now it doesn't.

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] Compiler warning in costsize.c

[Michael Paquier]

On Tue, Apr 11, 2017 at 4:02 AM, Robert Haas  wrote:
> On Mon, Apr 10, 2017 at 2:09 PM, Tom Lane  wrote:
>> ilm...@ilmari.org (Dagfinn Ilmari =?utf-8?Q?Manns=C3=A5ker?=) writes:
>>> Why bother with the 'rte' variable at all if it's only used for the
>>> Assert()ing the rtekind?
>>
>> That was proposed a few messages back.  I don't like it because it makes
>> these functions look different from the other scan-cost-estimation
>> functions, and we'd just have to undo the "optimization" if they ever
>> grow a need to reference the rte for another purpose.
>
> I think that's sort of silly, though.  It's a trivial difference,
> neither likely to confuse anyone nor difficult to undo.

+1. I would just do that and call it a day. There is no point to do a
mandatory list lookup as that's just for an assertion, and fixing this
warning does not seem worth the addition of fancier facilities. If the
function declarations were doubly-nested in the code, I would
personally consider the use of a variable, but not here.
-- 
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] [sqlsmith] ERROR: badly formatted node string "RESTRICTINFO...

[Tom Lane]

I wrote:
> (BTW, I've not yet looked to see if this needs to be back-ported.)

postgres_fdw will definitely include RestrictInfos in its fdw_private
list in 9.6.  However, I've been unable to provoke a visible failure.
After some rooting around, the reason seems to be that:

1. postgres_fdw doesn't mark its plans as parallel-safe --- it doesn't
even have a IsForeignScanParallelSafe method.  So you'd think that one
of its plans could never be transmitted to a parallel worker ... but:

2. Andreas' test query doesn't have the foreign scan in the main query
tree, it's in an InitPlan.  9.6 did not transmit any subplans or initplans
to parallel workers, but HEAD does.  So HEAD sends the illegal structure
to the worker which spits up on trying to read a RestrictInfo.

I think the fact that we see this problem at all may indicate an
oversight in commit 5e6d8d2bbbcace304450b309e79366c0da4063e4 ("Allow
parallel workers to execute subplans").  If the worker were to actually
run the initplan, bad things would happen (the worker would create its
own remote connection, which we don't want).  Now, in the problem plan
the InitPlan is actually attached to the topmost Gather, which I think
is safe because it'll be run by the master, but I wonder if we're being
careful enough about non-parallel-safe plans for initplans/subplans.
I do not think the planner tracks the locations of references to initplan
outputs carefully enough to be very sure about what it's doing here.

Also, even if the worker never actually executes the plan node, just
doing ExecInitNode on it in a parallel worker might be more than a
non-parallel-safe FDW is prepared to cope with.

Anyway, at present it doesn't look like we need this patch in 9.6.

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_stats_ext view does not seem all that useful

[Alvaro Herrera]

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

+1.  I have other immediate commitments but I can push David's patch on
Thursday.

-- 
Á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: Write Amplification Reduction Method (WARM)

[Andres Freund]

Hi,


On 2017-04-08 23:36:13 +0530, Pavan Deolasee wrote:
> On Wed, Apr 5, 2017 at 11:57 PM, Andres Freund  wrote:
> 
> > On 2017-04-05 09:36:47 -0400, Robert Haas wrote:
> > > By the way, the "Converting WARM chains back to HOT chains" section of
> > > README.WARM seems to be out of date.  Any chance you could update that
> > > to reflect the current state and thinking of the patch?
> >
> > I propose we move this patch to the next CF.  That shouldn't prevent you
> > working on it, although focusing on review of patches that still might
> > make it wouldn't hurt either.
> >
> >
> Thank you all for the  reviews, feedback, tests, criticism. And apologies
> for keep pushing it till the last minute even though it was clear to me
> quite some time back the patch is not going to make it.

What confuses me about that position is that people were advocating to
actually commit till literally hours before the CF closed.


> But if I'd given
> up, it would have never received whatever little attention it got. The only
> thing that disappoints me is that the patch was held back on no strong
> technical grounds -  at least none were clear to me. There were concerns
> about on-disk changes etc, but most on-disk changes were known for 7 months
> now. Reminds me of HOT development, when it would not receive adequate
> feedback for quite many months, probably for very similar reasons - complex
> patch, changes on-disk format, risky, even though performance gains were
> quite substantial. I was much more hopeful this time because we have many
> more experts now as compared to then, but we probably have equally more
> amount of complex patches to review/commit.

I don't think it's realistic to expect isolated in-depth review of
on-disk changes, when the rest of the patch isn't in a close-to-ready
shape. The likelihood that further work on the patch invalidates such
in-depth review is significant. It's not like only minor details changed
in the last few months.

I do agree that it's hard to get qualified reviewers on bigger patches.
But I think part of the reaction to that has to be active work on that
front: If your patch needs reviews by committers or other topical
experts, you need to explicitly reach out.  There's a lot of active
threads, and nobody has time to follow all of them in sufficient detail
to know that certain core parts of an actively developed patch are ready
for review.  Offer tit-for-tat reviews.  Announce that your patch is
ready, that you're only waiting for review.  Post a summary of open
questions...


> Finally, my apologies for not spending enough time reviewing other
> patches.  I know its critical, and I'll try to improve on that.

I do find it a more than a bit ironic to lament early lack of attention
to your patch, while also being aware of not having done much review.
This can only scale if everyone reviews each others patches, not if
there's a few individuals that have to review everyones patches.

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] pg_stats_ext view does not seem all that useful

[Tomas Vondra]

On 04/10/2017 12:12 PM, David Rowley wrote:

During my review and time spent working on the functional dependencies
part of extended statistics I wondered what was the use for the
pg_stats_ext view. I was unsure why the length of the serialised
dependencies was useful.

Perhaps we could improve the view, but I'm not all that sure what
value it would add. Maybe we need to discuss this, but in the
meantime, I've attached a patch which just removes the view completely



Yeah, let's get rid of the view. It was quite useful before introducing 
the custom data types (and implicit casts to text), because 
pg_statistic_ext would simply return the whole bytea value. But since 
then the view kinda lost the purpose and no one really noticed.


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] Patch: Write Amplification Reduction Method (WARM)

[Bruce Momjian]

On Sat, Apr  8, 2017 at 11:36:13PM +0530, Pavan Deolasee wrote:
> Thank you all for the  reviews, feedback, tests, criticism. And apologies for
> keep pushing it till the last minute even though it was clear to me quite some
> time back the patch is not going to make it. But if I'd given up, it would 
> have
> never received whatever little attention it got. The only thing that
> disappoints me is that the patch was held back on no strong technical grounds 
> -
>  at least none were clear to me. There were concerns about on-disk changes 
> etc,
> but most on-disk changes were known for 7 months now. Reminds me of HOT
> development, when it would not receive adequate feedback for quite many 
> months,
> probably for very similar reasons - complex patch, changes on-disk format,
> risky, even though performance gains were quite substantial. I was much more
> hopeful this time because we have many more experts now as compared to then,
> but we probably have equally more amount of complex patches to review/commit.

I am sad to see WARM didn't make it into Postgres 10, but I agree
deferment was the right decision, as painful as that is.  We now have
something to look forward to in Postgres 11.  :-)

-- 
  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] strange parallel query behavior after OOM crashes

[Tomas Vondra]

On 04/10/2017 01:39 PM, Kuntal Ghosh wrote:

On Thu, Apr 6, 2017 at 6:50 AM, Robert Haas  wrote:

On Wed, Apr 5, 2017 at 8:17 PM, Neha Khatri  wrote:

The problem here seem to be the change in the max_parallel_workers value
while the parallel workers are still under execution. So this poses two
questions:

1. From usecase point of view, why could there be a need to tweak the
max_parallel_workers exactly at the time when the parallel workers are at
play.
2. Could there be a restriction on tweaking of max_parallel_workers while
the parallel workers are at play? At least do not allow setting the
max_parallel_workers less than the current # of active parallel workers.


Well, that would be letting the tail wag the dog.  The maximum value
of max_parallel_workers is only 1024, and what we're really worried
about here is seeing a value near PG_UINT32_MAX, which leaves a lot of
daylight.  How about just creating a #define that's used by guc.c as
the maximum for the GUC, and here we assert that we're <= that value?


I've added a GUC parameter named MAX_PARALLEL_WORKER_LIMIT and
asserted that the absolute difference between the two counts <= that
value, i.e.,
abs((int)(register_count - terminate_count)) <= MAX_PARALLEL_WORKER_LIMIT;

Because, it's possible that register count has overflowed but
terminate count has not yet overflowed. As a result, the difference in
unsigned integer will be near UINT32_MAX. Hence, we need the absolute
difference after typecasting the same to integer. This value should be
less than the max_parallel_workers upper limit.

I've attached both the patches for better accessibility. PFA.



At first I was like 'WTF? Why do we need a new GUC just becase of an 
assert?' but you're actually not adding a new GUC parameter, you're 
adding a constant which is then used as a max value for max for the two 
existing parallel GUCs.


I think this is fine.

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] [sqlsmith] ERROR: badly formatted node string "RESTRICTINFO...

[Tom Lane]

I wrote:
> Apparently, postgres_fdw is trying to store RestrictInfos in the
> fdw_private field of a ForeignScan node.  That won't do; those aren't
> supposed to be present in a finished plan tree, so there's no readfuncs.c
> support for them (and we're not adding it).

> Don't know if this is a new bug, or ancient but not previously reachable.
> It seems to be nearly the inverse of the problem you found yesterday,
> in which postgres_fdw was stripping RestrictInfos sooner than it really
> ought to.  Apparently that whole business needs a fresh look.

Attached is a proposed patch that cleans up the mess here --- and it was
a mess.  The comments for PgFdwRelationInfo claimed that the remote_conds
and local_conds fields contained bare clauses, but actually they could
contain either bare clauses or RestrictInfos or both, depending on where
the clauses had come from.  And there was some seriously obscure and
undocumented logic around how the fdw_recheck_quals got set up for a
foreign join node.  (BTW, said logic is assuming that no EPQ recheck is
needed for a foreign join.  I commented it to that effect, but I'm not
very sure that I believe it.  If there's a bug there, though, it's
independent of the immediate problem.)

Anybody want to review this, or shall I just push it?

(BTW, I've not yet looked to see if this needs to be back-ported.)

regards, tom lane

diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c
index c5149a0..1d5aa83 100644
*** a/contrib/postgres_fdw/deparse.c
--- b/contrib/postgres_fdw/deparse.c
*** classifyConditions(PlannerInfo *root,
*** 210,216 
  
  	foreach(lc, input_conds)
  	{
! 		RestrictInfo *ri = (RestrictInfo *) lfirst(lc);
  
  		if (is_foreign_expr(root, baserel, ri->clause))
  			*remote_conds = lappend(*remote_conds, ri);
--- 210,216 
  
  	foreach(lc, input_conds)
  	{
! 		RestrictInfo *ri = lfirst_node(RestrictInfo, lc);
  
  		if (is_foreign_expr(root, baserel, ri->clause))
  			*remote_conds = lappend(*remote_conds, ri);
*** build_tlist_to_deparse(RelOptInfo *forei
*** 869,874 
--- 869,875 
  {
  	List	   *tlist = NIL;
  	PgFdwRelationInfo *fpinfo = (PgFdwRelationInfo *) foreignrel->fdw_private;
+ 	ListCell   *lc;
  
  	/*
  	 * For an upper relation, we have already built the target list while
*** build_tlist_to_deparse(RelOptInfo *forei
*** 884,892 
  	tlist = add_to_flat_tlist(tlist,
  	   pull_var_clause((Node *) foreignrel->reltarget->exprs,
  	   PVC_RECURSE_PLACEHOLDERS));
! 	tlist = add_to_flat_tlist(tlist,
! 			  pull_var_clause((Node *) fpinfo->local_conds,
! 			  PVC_RECURSE_PLACEHOLDERS));
  
  	return tlist;
  }
--- 885,898 
  	tlist = add_to_flat_tlist(tlist,
  	   pull_var_clause((Node *) foreignrel->reltarget->exprs,
  	   PVC_RECURSE_PLACEHOLDERS));
! 	foreach(lc, fpinfo->local_conds)
! 	{
! 		RestrictInfo *rinfo = lfirst_node(RestrictInfo, lc);
! 
! 		tlist = add_to_flat_tlist(tlist,
!   pull_var_clause((Node *) rinfo->clause,
!   PVC_RECURSE_PLACEHOLDERS));
! 	}
  
  	return tlist;
  }
*** deparseSelectSql(List *tlist, bool is_su
*** 1049,1054 
--- 1055,1061 
   * "buf".
   *
   * quals is the list of clauses to be included in the WHERE clause.
+  * (These may or may not include RestrictInfo decoration.)
   */
  static void
  deparseFromExpr(List *quals, deparse_expr_cxt *context)
*** deparseLockingClause(deparse_expr_cxt *c
*** 1262,1267 
--- 1269,1277 
   *
   * The conditions in the list are assumed to be ANDed. This function is used to
   * deparse WHERE clauses, JOIN .. ON clauses and HAVING clauses.
+  *
+  * Depending on the caller, the list elements might be either RestrictInfos
+  * or bare clauses.
   */
  static void
  appendConditions(List *exprs, deparse_expr_cxt *context)
*** appendConditions(List *exprs, deparse_ex
*** 1278,1293 
  	{
  		Expr	   *expr = (Expr *) lfirst(lc);
  
! 		/*
! 		 * Extract clause from RestrictInfo, if required. See comments in
! 		 * declaration of PgFdwRelationInfo for details.
! 		 */
  		if (IsA(expr, RestrictInfo))
! 		{
! 			RestrictInfo *ri = (RestrictInfo *) expr;
! 
! 			expr = ri->clause;
! 		}
  
  		/* Connect expressions with "AND" and parenthesize each condition. */
  		if (!is_first)
--- 1288,1296 
  	{
  		Expr	   *expr = (Expr *) lfirst(lc);
  
! 		/* Extract clause from RestrictInfo, if required */
  		if (IsA(expr, RestrictInfo))
! 			expr = ((RestrictInfo *) expr)->clause;
  
  		/* Connect expressions with "AND" and parenthesize each condition. */
  		if (!is_first)
diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index 6670df5..28a945f 100644
*** a/contrib/postgres_fdw/postgres_fdw.c
--- b/contrib/postgres_fdw/postgres_fdw.c
*** enum FdwScanPrivateIndex
*** 64,70 
  {
  	/* SQL statement to execute remotely (as a 

Re: [HACKERS] "left shift of negative value" warnings

[Markus Nullmeier]

On 10/04/17 22:19, Andres Freund wrote:
> I guess the motivation is that it's not entirely clear what happens with
> the sign bit, when shifting.

Indeed, certain one's complement CPUs even "outlived" C99 by a small
margin, as it were:
http://mainframe.typepad.com/blog/2012/10/sad-day-unisys-abandons-its-mainframe-processors.html

And the respective ABI will be around for some time to come, apparently:
https://www.theregister.co.uk/2016/03/31/free_x86_mainframes_for_all_virtual_mainframes_that_is

:-)

-- 
Markus Nullmeierhttp://www.g-vo.org
German Astrophysical Virtual Observatory (GAVO)



-- 
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] Some thoughts about SCRAM implementation

[Robert Haas]

On Mon, Apr 10, 2017 at 2:32 PM, Andres Freund  wrote:
> That can equally be said about about a lot of features.  If we don't
> stop at some point... Also, we're not late in the CF cycle, the CF cycle
> for v10 is over.  It's not like the non-existance of channel binding
> removes previously existing features, or makes SCRAM useless.

+1.

-- 
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] strange parallel query behavior after OOM crashes

[Robert Haas]

On Mon, Apr 10, 2017 at 2:32 PM, Neha Khatri  wrote:
> On Tue, Apr 11, 2017 at 1:16 AM, Robert Haas  wrote:
>> 1. Forget BGW_NEVER_RESTART workers in
>> ResetBackgroundWorkerCrashTimes() rather than leaving them around to
>> be cleaned up after the conclusion of the restart, so that they go
>> away before rather than after shared memory is reset.
>
> Now with this, would it still be required to forget BGW_NEVER_RESTART
> workers in maybe_start_bgworker():
>
> if (rw->rw_crashed_at != 0)
> {
> if (rw->rw_worker.bgw_restart_time == BGW_NEVER_RESTART)
> {
> ForgetBackgroundWorker();
> continue;
> }
>  ..
> }

Well, as noted before, not every background worker failure leads to a
crash-and-restart; for example, a non-shmem-connected worker or one
that exits with status 1 will set rw_crashed_at but will not cause a
crash-and-restart cycle.   It's not 100% clear to me whether the code
you're talking about can be reached in those cases, but I wouldn't bet
against it.

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


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

Re: [HACKERS] "left shift of negative value" warnings

[Andres Freund]

On 2017-04-10 15:25:57 -0400, Tom Lane wrote:
> Andres Freund  writes:
> > On 2017-04-09 19:20:27 -0400, Tom Lane wrote:
> >> As I read that, it's only "undefined" if overflow would occur (ie
> >> the sign bit would change).  Your compiler is being a useless annoying
> >> nanny, but that seems to be the in thing for compiler authors these
> >> days.
> 
> > "The result of E1 << E2 is E1 left-shifted E2 bit positions; vacated bits 
> > are filled with
> > zeros. If E1 has an unsigned type, the value of the result is E1 × 2 E2 , 
> > reduced modulo
> > one more than the maximum value representable in the result type. If E1 has 
> > a signed
> > type and nonnegative value, and E1 × 2 E2 is representable in the result 
> > type, then that is
> > the resulting value; otherwise, the behavior is undefined."
> 
> > As I read this it's defined iff E1 is signed, nonnegative *and* the the
> > result of the shift is representable in the relevant type.  That seems,
> > uh, a bit restrictive, but that seems to be the only reading?
> 
> Oh --- I misread the "nonnegative" as applying to the shift count, but
> you're right, it's talking about the LHS.  That's weird --- the E1 × 2^E2
> definition works fine as long as there's no overflow, so why didn't they
> define it like that?  It seems just arbitrarily broken this way.

I guess the motivation is that it's not entirely clear what happens with
the sign bit, when shifting.  Why they made that UB instead of
implementation defined, is a complete mystery to me, however.

We should do *something* about this?  The warnings are a bit annoying :(

- Andres


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

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

[Álvaro Hernández Tortosa]

On 10/04/17 21:41, Heikki Linnakangas wrote:

On 04/10/2017 09:33 PM, Álvaro Hernández Tortosa wrote:

 Thanks for posting the patched HTML. In my opinion, all looks good
except that:

- I will add an extra String (a CSV) to AuthenticationSASL message for
channel binding names, so that message format can remain without changes
when channel binding is implemented. It can be empty.


Note that SCRAM-SHA-256 with channel binding has a different SASL 
mechanism name, SRAM-SHA-256-PLUS. No need for a separate flag or 
string for channel binding. When support for channel binding is added 
to the server, it will advertise two SASL mechanisms in the 
AuthenticationSASL message, SCRAM-SHA-256 and SCRAM-SHA-256-PLUS. (Or 
just SCRAM-SHA-256-PLUS, if channel-binding is required).


Channel binding needs to specify actually three things:
- The mechanism, which is indeed suffixed "-PLUS".
- The channel binding name, which is described here: 
https://tools.ietf.org/html/rfc5056. Types are also IANA-registered (see 
https://www.iana.org/assignments/channel-binding-types/channel-binding-types.xhtml). 
SCRAM mandates to implement 'tls-unique', but other channel binding 
types could be supported (like 'tls-server-end-point' for example).
- The channel binding data, which is channel binding mechanism 
dependent, and is sent as part of the client last message.


What I'm talking about here is the second one, the channel binding 
type (name).





- If the username used is the one sent in the startup message, rather
than leaving it empty in the client-first-message, I would force it to
be the same as the used in the startuo message.


The problem with that is that the SCRAM spec dictates that the 
username must be encoded in UTF-8, but PostgreSQL supports non-UTF-8 
usernames.


Or did you mean that, if the username is sent, it must match the one 
in the startup packet, but an empty string would always be allowed? 
That would be reasonable.



Otherwise we may confuse
some client implementations which would probably consider that as an
error; for one, my implementation would currently throw an error if
username is empty, and I think that's correct.


I'm not sure I follow. The username is sent from client to server, and 
currently, the server will ignore it. If you're writing a client 
library, it can send whatever it wants. (Although again I would 
recommend an empty string, to avoid confusion. Sending the same 
username as in the startup packet, as long as it's in UTF-8, seems 
reasonable too.)


OK, understood. I will not let then the SCRAM implementation I'm 
writing to allow for empty string as the user name, but in the pgjdbc 
glue code send "ignore" as the user name or something like that ;P


Thanks for reviewing this! I'll start hacking on code changes to go 
with these docs.


Thanks for writing the code :)


Álvaro


--

Álvaro Hernández Tortosa


---
<8K>data



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

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

[Heikki Linnakangas]

On 04/10/2017 09:33 PM, Álvaro Hernández Tortosa wrote:

 Thanks for posting the patched HTML. In my opinion, all looks good
except that:

- I will add an extra String (a CSV) to AuthenticationSASL message for
channel binding names, so that message format can remain without changes
when channel binding is implemented. It can be empty.


Note that SCRAM-SHA-256 with channel binding has a different SASL 
mechanism name, SRAM-SHA-256-PLUS. No need for a separate flag or string 
for channel binding. When support for channel binding is added to the 
server, it will advertise two SASL mechanisms in the AuthenticationSASL 
message, SCRAM-SHA-256 and SCRAM-SHA-256-PLUS. (Or just 
SCRAM-SHA-256-PLUS, if channel-binding is required).



- If the username used is the one sent in the startup message, rather
than leaving it empty in the client-first-message, I would force it to
be the same as the used in the startuo message.


The problem with that is that the SCRAM spec dictates that the username 
must be encoded in UTF-8, but PostgreSQL supports non-UTF-8 usernames.


Or did you mean that, if the username is sent, it must match the one in 
the startup packet, but an empty string would always be allowed? That 
would be reasonable.



Otherwise we may confuse
some client implementations which would probably consider that as an
error; for one, my implementation would currently throw an error if
username is empty, and I think that's correct.


I'm not sure I follow. The username is sent from client to server, and 
currently, the server will ignore it. If you're writing a client 
library, it can send whatever it wants. (Although again I would 
recommend an empty string, to avoid confusion. Sending the same username 
as in the startup packet, as long as it's in UTF-8, seems reasonable too.)


Thanks for reviewing this! I'll start hacking on code changes to go with 
these docs.


- Heikki



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

Re: [HACKERS] "left shift of negative value" warnings

[Tom Lane]

Andres Freund  writes:
> On 2017-04-09 19:20:27 -0400, Tom Lane wrote:
>> As I read that, it's only "undefined" if overflow would occur (ie
>> the sign bit would change).  Your compiler is being a useless annoying
>> nanny, but that seems to be the in thing for compiler authors these
>> days.

> "The result of E1 << E2 is E1 left-shifted E2 bit positions; vacated bits are 
> filled with
> zeros. If E1 has an unsigned type, the value of the result is E1 × 2 E2 , 
> reduced modulo
> one more than the maximum value representable in the result type. If E1 has a 
> signed
> type and nonnegative value, and E1 × 2 E2 is representable in the result 
> type, then that is
> the resulting value; otherwise, the behavior is undefined."

> As I read this it's defined iff E1 is signed, nonnegative *and* the the
> result of the shift is representable in the relevant type.  That seems,
> uh, a bit restrictive, but that seems to be the only reading?

Oh --- I misread the "nonnegative" as applying to the shift count, but
you're right, it's talking about the LHS.  That's weird --- the E1 × 2^E2
definition works fine as long as there's no overflow, so why didn't they
define it like that?  It seems just arbitrarily broken this way.

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] "left shift of negative value" warnings

[Andres Freund]

On 2017-04-09 19:20:27 -0400, Tom Lane wrote:
> Andres Freund  writes:
> > For a while I've been getting warnings like
> > /home/andres/src/postgresql/src/backend/utils/adt/inet_cidr_ntop.c: In 
> > function ‘inet_cidr_ntop_ipv6’:
> > /home/andres/src/postgresql/src/backend/utils/adt/inet_cidr_ntop.c:205:11: 
> > warning: left shift of negative value [-Wshift-negative-value]
> > m = ~0 << (8 - b);
> >^~
> 
> I imagine forcing the LHS to unsigned would silence that, though you'd
> have to be careful that the sign extension (widening) happened before
> you changed the value to unsigned, in the int64 cases.  It's a bit odd
> though that it seems to think ~0 is signed.

Hm, why's that odd? By default integers are signed, and ~ doesn't change
signedness?


> > If I understand C99 correctly, the behaviour of a left-shift of a
> > negative number is undefined (6.5.7 4.).
> 
> As I read that, it's only "undefined" if overflow would occur (ie
> the sign bit would change).  Your compiler is being a useless annoying
> nanny, but that seems to be the in thing for compiler authors these
> days.

"The result of E1 << E2 is E1 left-shifted E2 bit positions; vacated bits are 
filled with
zeros. If E1 has an unsigned type, the value of the result is E1 × 2 E2 , 
reduced modulo
one more than the maximum value representable in the result type. If E1 has a 
signed
type and nonnegative value, and E1 × 2 E2 is representable in the result type, 
then that is
the resulting value; otherwise, the behavior is undefined."

As I read this it's defined iff E1 is signed, nonnegative *and* the the
result of the shift is representable in the relevant type.  That seems,
uh, a bit restrictive, but that seems to be the only reading?  Note that
the otherwise is preceded by a semicolon...

- 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] Somebody has not thought through subscription locking considerations

[Peter Eisentraut]

On 3/31/17 20:25, Petr Jelinek wrote:
> On 01/04/17 01:57, Petr Jelinek wrote:
>> That being said, looking at use-cases for SetSubscriptionRelState that's
>> basically CREATE SUBSCRIPTION, ALTER SUBSCRIPTION REFRESH and tablesync
>> worker. So the DDL thing applies to first ones as well and tablesync
>> should not be running in case the record does not exist so it's fine if
>> it fails. In terms of RemoveSubscriptionRel that's only called from
>> heap_drop_with_catalog and tablesync holds relation lock so there is no
>> way heap_drop_with_catalog will happen on the same relation. This leads
>> me to thinking that RowExclusiveLock is fine for both
>> SetSubscriptionRelState and RemoveSubscriptionRel as long as we document
>> that callers should be aware that SetSubscriptionRelState has
>> concurrency issues and fail on unique index check.
>>
> 
> And a simple patch to do so. Peter do you see any problem with doing this?

committed

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


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

Re: [HACKERS] recent deadlock regression test failures

[Tom Lane]

Thomas Munro  writes:
> On Tue, Apr 11, 2017 at 6:17 AM, Tom Lane  wrote:
>> Maybe it's impossible for a parallel worker to acquire its own
>> snapshot at all, in which case this is moot.  But I'm nervous.

> Parallel workers can't acquire snapshots, and SERIALIZABLE disables
> all parallel query planning anyway.

> I did some early stage POC hacking to lift that restriction[1], and if
> we finish up doing it at all like that then all SERIALIZABLEXACT
> structures would be associated with leader processes and
> pg_safe_snapshot_blocking_pid() would automatically deal only in
> leader PIDs as arguments and results so there would be no problem
> here.  The interlocking I proposed in that WIP patch may need work, to
> be discussed, but I'm fairly sure that sharing the leader's
> SERIALIZABLEXACT like that is the only sensible way forward.

OK, sounds good.  I agree that it would only be sensible for a parallel
worker to be using a snapshot already acquired by the master, so far
as the parallelized query itself is concerned.  What was worrying me
is snapshots acquired by, eg, volatile PL functions called by the query.
But on second thought, in SERIALIZABLE mode no such function would be
taking an actual new snapshot anyhow --- they'd just continue to use
the transaction snap.

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] valgrind errors around dsa.c

[Andres Freund]

On 2017-04-08 14:46:04 +1200, Thomas Munro wrote:
> Fix attached.

Thanks. Pushed!

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] Compiler warning in costsize.c

[Robert Haas]

On Mon, Apr 10, 2017 at 2:09 PM, Tom Lane  wrote:
> ilm...@ilmari.org (Dagfinn Ilmari =?utf-8?Q?Manns=C3=A5ker?=) writes:
>> Why bother with the 'rte' variable at all if it's only used for the
>> Assert()ing the rtekind?
>
> That was proposed a few messages back.  I don't like it because it makes
> these functions look different from the other scan-cost-estimation
> functions, and we'd just have to undo the "optimization" if they ever
> grow a need to reference the rte for another purpose.

I think that's sort of silly, though.  It's a trivial difference,
neither likely to confuse anyone nor difficult to undo.

-- 
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] recent deadlock regression test failures

[Thomas Munro]

On Tue, Apr 11, 2017 at 6:17 AM, Tom Lane  wrote:
> Kevin Grittner  writes:
>> On Mon, Apr 10, 2017 at 9:28 AM, Tom Lane  wrote:
>>> I notice that the safe-snapshot code path is not paying attention to
>>> parallel-query cases, unlike the lock code path.  I'm not sure how
>>> big a deal that is...
>
>> Parallel workers in serializable transactions should be using the
>> transaction number of the "master" process to take any predicate
>> locks, and if parallel workers are doing any DML work against
>> tuples, that should be using the master transaction number for
>> xmin/xmax and serialization failure testing.
>
> Right, but do they record the master's PID rather than their own in
> the SERIALIZABLEXACT data structure?
>
> Maybe it's impossible for a parallel worker to acquire its own
> snapshot at all, in which case this is moot.  But I'm nervous.

Parallel workers can't acquire snapshots, and SERIALIZABLE disables
all parallel query planning anyway.

I did some early stage POC hacking to lift that restriction[1], and if
we finish up doing it at all like that then all SERIALIZABLEXACT
structures would be associated with leader processes and
pg_safe_snapshot_blocking_pid() would automatically deal only in
leader PIDs as arguments and results so there would be no problem
here.  The interlocking I proposed in that WIP patch may need work, to
be discussed, but I'm fairly sure that sharing the leader's
SERIALIZABLEXACT like that is the only sensible way forward.

[1] https://commitfest.postgresql.org/14/1004/

-- 
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] GSOC'17 project introduction: Parallel COPY execution with errors handling

[Alexey Kondratov]

Yes, sure, I don't doubt it. The question was around step 4 in the following 
possible algorithm:

1. Suppose we have to insert N records
2. Start subtransaction with these N records
3. Error is raised on k-th line
4. Then, we know that we can safely insert all lines from the 1st till (k - 1)
5. Report, save to errors table or silently drop k-th line
6. Next, try to insert lines from (k + 1) till Nth with another subtransaction
7. Repeat until the end of file

One can start subtransaction with those (k - 1) safe-lines and repeat it after 
each error line
OR 
iterate till the end of file and start only one subtransaction with all lines 
excepting error lines.


Alexey


> On 10 Apr 2017, at 19:55, Robert Haas  wrote:
> 
> On Mon, Apr 10, 2017 at 11:39 AM, Alex K  wrote:
>> (1) It seems that starting new subtransaction at step 4 is not necessary. We
>> can just gather all error lines in one pass and at the end of input start
>> the only one additional subtransaction with all safe-lines at once: [1, ...,
>> k1 - 1, k1 + 1, ..., k2 - 1, k2 + 1, ...], where ki is an error line number.
> 
> The only way to recover from an error is to abort the subtransaction,
> or to abort the toplevel transaction.  Anything else is unsafe.
> 
> -- 
> 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] recent deadlock regression test failures

[Kevin Grittner]

On Mon, Apr 10, 2017 at 1:17 PM, Tom Lane  wrote:
> Kevin Grittner  writes:
>> On Mon, Apr 10, 2017 at 9:28 AM, Tom Lane  wrote:
>>> I notice that the safe-snapshot code path is not paying attention to
>>> parallel-query cases, unlike the lock code path.  I'm not sure how
>>> big a deal that is...
>
>> Parallel workers in serializable transactions should be using the
>> transaction number of the "master" process to take any predicate
>> locks, and if parallel workers are doing any DML work against
>> tuples, that should be using the master transaction number for
>> xmin/xmax and serialization failure testing.
>
> Right, but do they record the master's PID rather than their own in
> the SERIALIZABLEXACT data structure?

There had better be just a single SERIALIZABLEXACT data structure
for a serializable transaction, or it just doesn't work.  I can't
see how it would have anything but the master's PID in it.  If
parallel execution is possible in a serializable transaction and
things are done any other way, we have a problem.

> Maybe it's impossible for a parallel worker to acquire its own
> snapshot at all, in which case this is moot.  But I'm nervous.

I have trouble seeing what sane semantics we could possibly have if
each worker for a parallel scan used a different snapshot -- under
any transaction isolation level.  I know that under the read
committed transaction isolation level we can follow xmax pointers to
hit tuples on the target relation which are not in the snapshot used
for the scan, but I don't see how that would negate the need to have
the scan itself run on a single snapshot,

--
Kevin Grittner
VMware vCenter Server
https://www.vmware.com/


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

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

[Álvaro Hernández Tortosa]

On 10/04/17 14:57, Heikki Linnakangas wrote:

On 04/07/2017 01:13 AM, Michael Paquier wrote:
On Fri, Apr 7, 2017 at 5:15 AM, Álvaro Hernández Tortosa 
 wrote:
I don't see it. The message AuthenticationSASL.String could 
contain a
CSV of the SCRAM protocols supported. This is specially important to 
support
channel binding (which is just another protocol name for this 
matter), which
is the really enhanced security mechanism of SCRAM. Since this 
message is
sent regardless, and the client replies with PasswordMessage, no 
extra round
trip is required. However, PasswordMessage needs to also include a 
field
with the name of the selected protocol (it is the client who picks). 
Or a
different message would need to be created, but no extra round-trips 
more
than those required by SCRAM itself (4 messages for SCRAM + 1 extra 
for the

server to tell the client it needs to use SCRAM).


Yes, it seems to me that the list of protocols to send should be done
by sendAuthRequest(). Then the client parses the received string, and
sends an extra 'p' message with its choice before sending the first
SCRAM message. So there is no need for any extra round trips.


I started writing down the protocol docs, based on the above idea. See 
attached. The AuthenticationSASL message now contains a list of 
mechanisms.


Does that seem clear to you? If so, I'll change the code to match the 
attached docs.


I added two new message formats to the docs, SASLResponse and 
SASLInitialResponse. Those use the same type byte as PasswordMessage, 
'p', but I decided to describe them as separate messages for 
documentation purposes, since the content is completely different 
depending on whether the message is sent as part of SASL, GSS, md5, or 
password authentication. IOW, this is not a change in the 
implementation, only in the way it's documented.



While working on this, and reading the RFCs more carefully, I noticed 
one detail we should change, to be spec-complicant. The SASL spec 
specifies that a SASL authentication exchange consists of 
challenge-response pairs. There must be a response to each challenge. 
If the last message in the authentication mechanism (SCRAM in this 
case) goes in the server->client direction, then that message must 
sent as "additional data" in the server->client message that tells the 
client that the authentication was successful. That's AuthenticationOK 
in the PostgreSQL protocol. In the current implementation, the 
server-final-message is sent as an AuthenticationSASLContinue message, 
and the client doesn't respond to that.


We should change that, so that the server-final-message is sent as 
"additional data" in the AuthenticationOK message. The attached docs 
patch describes that, rather than what the current implementation does.


(For your convenience, I built the HTML docs with this patch, and put 
them up at http://hlinnaka.iki.fi/temp/scram-wip-docs/protocol.html 
for viewing)


- Heikki




Thanks for posting the patched HTML. In my opinion, all looks good 
except that:


- I will add an extra String (a CSV) to AuthenticationSASL message for 
channel binding names, so that message format can remain without changes 
when channel binding is implemented. It can be empty.


- If the username used is the one sent in the startup message, rather 
than leaving it empty in the client-first-message, I would force it to 
be the same as the used in the startuo message. Otherwise we may confuse 
some client implementations which would probably consider that as an 
error; for one, my implementation would currently throw an error if 
username is empty, and I think that's correct.



Álvaro

--

Álvaro Hernández Tortosa


---
<8K>data

Re: [HACKERS] strange parallel query behavior after OOM crashes

[Neha Khatri]

On Tue, Apr 11, 2017 at 1:16 AM, Robert Haas  wrote:

>
> 1. Forget BGW_NEVER_RESTART workers in
> ResetBackgroundWorkerCrashTimes() rather than leaving them around to
> be cleaned up after the conclusion of the restart, so that they go
> away before rather than after shared memory is reset.


Now with this, would it still be required to forget BGW_NEVER_RESTART
workers in maybe_start_bgworker():

if (rw->rw_crashed_at != 0)
{
if (rw->rw_worker.bgw_restart_time == BGW_NEVER_RESTART)
{
ForgetBackgroundWorker();
continue;
}
 ..
}

Regards,
Neha

Re: [HACKERS] Some thoughts about SCRAM implementation

[Andres Freund]

On 2017-04-10 20:28:27 +0200, Álvaro Hernández Tortosa wrote:
> 
> 
> On 10/04/17 13:02, Heikki Linnakangas wrote:
> > On 04/10/2017 12:39 PM, Álvaro Hernández Tortosa wrote:
> > > - I think channel binding support should be added. SCRAM brings security
> > > improvements over md5 and other simpler digest algorithms. But where it
> > > really shines is together with channel binding. This is the only method
> > > to prevent MITM attacks. Implementing it should not very difficult.
> > > There are several possible channel binding mechanisms, but the mandatory
> > > and probably more typical one is 'tls-unique' which basically means
> > > getting the byte array from the TLSfinish() message and comparing it
> > > with the same data sent by the client. That's more or less all it takes
> > > to implement it. So I would go for it.
> > 
> > We missed the boat for PostgreSQL 10. You're right that it probably
> > wouldn't be difficult to implement, but until there's a concrete patch
> > to discuss, that's a moot point.
> 
> Really? That's a real shame I know we're very late in the CF cycle
> but, again, this would be a real shame.

That can equally be said about about a lot of features.  If we don't
stop at some point... Also, we're not late in the CF cycle, the CF cycle
for v10 is over.  It's not like the non-existance of channel binding
removes previously existing features, or makes SCRAM useless.


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] Some thoughts about SCRAM implementation

[Álvaro Hernández Tortosa]

On 10/04/17 13:02, Heikki Linnakangas wrote:

On 04/10/2017 12:39 PM, Álvaro Hernández Tortosa wrote:

- I think channel binding support should be added. SCRAM brings security
improvements over md5 and other simpler digest algorithms. But where it
really shines is together with channel binding. This is the only method
to prevent MITM attacks. Implementing it should not very difficult.
There are several possible channel binding mechanisms, but the mandatory
and probably more typical one is 'tls-unique' which basically means
getting the byte array from the TLSfinish() message and comparing it
with the same data sent by the client. That's more or less all it takes
to implement it. So I would go for it.


We missed the boat for PostgreSQL 10. You're right that it probably 
wouldn't be difficult to implement, but until there's a concrete patch 
to discuss, that's a moot point.


Really? That's a real shame I know we're very late in the CF 
cycle but, again, this would be a real shame.





- One SCRAM parameter, i, the iteration count, is important, and in my
opinion should be configurable. AFAIK it is currently hard coded at
4096, which is the minimum value accepted by the RFC. But it should be
at least 15K (RFC says), and given that it affects computing time of the
authentication, it should be configurable. It's also now specified per
user, which I think its too much granularity (it should suffice to say
this group of users all require this iteration count).


Yeah, a GUC might be a good idea.


Cool.



The iteration count is a part of the stored SCRAM verifier, so it's 
determined when you set the user's password. That's why it's per-user.


Sure. But I think per-user is too much granularity. And if it is 
not, it should be a parameter exposed to the CREATE USER command, such 
that it can be (effectively) set per-user. I maintain the best approach 
could be the suggested pg_scram.conf with the format (or a similar one) 
that I proposed in the OP.





- The SCRAM RFC states that server should advertise supported
mechanisms. I see discussion going into not advertising them. I think it
should be done, I don't see reasons not to do it, and it will become
compliant with the RFC.


The SASL spec [RFC4422] says that mechanism negotiation is protocol 
specific. The SCRAM spec [RFC5802] prescribes how negotiation of 
channel-binding is done, and the current implementation is compliant 
with that. (Which is very straightforward when neither the client nor 
the server supports channel binding).


Yeah. But what I'm saying is that we might want to add an extra 
String to AuthenticationSASL message for channel binding, so that the 
message format needs not to be changed when channel binding is added. 
But the channel binding method name needs to be negotiated, and so there 
needs to be room for it.




The negotiation of the mechanism is being discussed one the "Letting 
the client choose the protocol to use during a SASL exchange" thread. 
I'm just writing a more concrete proposal based on your suggestion of 
sending a list of SASL mechanisms in the AuthenticationSASL message. 
Stay tuned!


Yepp, I will reply there, thanks :)




- I don't see why proposed scram mechanism names for pg_hba.conf are not
following the IANA Registry standard (
https://www.iana.org/assignments/sasl-mechanisms/sasl-mechanisms.xhtml#scram), 


which is uppercase (like in SCRAM-SHA-256).


All the existing authentication mechanisms are spelled in lowercase. 
And uppercase is ugly. It's a matter of taste, for sure.


And I agree with you. It's ugly. But it's standard. I'd say let's 
favor standardization vs our own taste.





- SCRAM also supports the concept of authzid, which is kind of what
pg_ident does: authenticate the user as the user sent, but login as the
user specified here. It could also be supported.


Yeah, it might be handy for something like pgbouncer, which could then 
have one userid+password to authenticate, but then switch to another 
user. But the SCRAM part of that seems to be just a small part of the 
overall feature. In any case, this is clearly Postgres 11 material.


Again, it should be a tiny change, just an extra attribute sent 
over the message. But I guess time was over... just saying... ;)






* The nonce length is not specified by the RFC. I see typical
implementations use 24 chars for the client and 18 for the server.
Current code uses 10. I think it should not hurt making it at least 16
or 18.


Wouldn't hurt, I guess. IIRC I checked some other implementations, 
when I picked 10, but I don't remember which ones anymore. Got a 
reference for 24/18?


First reference is the RFC example itself (non-mandatory, of 
course). But then I saw many followed this. As a quick example, GNU SASL 
defines:


#define SNONCE_ENTROPY_BYTES 18
https://www.gnu.org/software/gsasl/coverage/lib/scram/server.c.gcov.frameset.html




* It seems to me that the errors defined by the RFC, sent on 

Re: [HACKERS] recent deadlock regression test failures

[Tom Lane]

Kevin Grittner  writes:
> On Mon, Apr 10, 2017 at 9:28 AM, Tom Lane  wrote:
>> I notice that the safe-snapshot code path is not paying attention to
>> parallel-query cases, unlike the lock code path.  I'm not sure how
>> big a deal that is...

> Parallel workers in serializable transactions should be using the
> transaction number of the "master" process to take any predicate
> locks, and if parallel workers are doing any DML work against
> tuples, that should be using the master transaction number for
> xmin/xmax and serialization failure testing.

Right, but do they record the master's PID rather than their own in
the SERIALIZABLEXACT data structure?

Maybe it's impossible for a parallel worker to acquire its own
snapshot at all, in which case this is moot.  But I'm nervous.

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] Compiler warning in costsize.c

[Tom Lane]

ilm...@ilmari.org (Dagfinn Ilmari =?utf-8?Q?Manns=C3=A5ker?=) writes:
> Why bother with the 'rte' variable at all if it's only used for the
> Assert()ing the rtekind?

That was proposed a few messages back.  I don't like it because it makes
these functions look different from the other scan-cost-estimation
functions, and we'd just have to undo the "optimization" if they ever
grow a need to reference the rte for another purpose.

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] Compiler warning in costsize.c

[Tom Lane]

Robert Haas  writes:
> On Mon, Apr 10, 2017 at 8:30 AM, Michael Paquier
>  wrote:
>> On Mon, Apr 10, 2017 at 9:05 PM, Tom Lane  wrote:
>>> I wonder if we shouldn't just do
>>> ...
>>> and eat the "useless" calculation of rte.

> -1 from me.  I'm not a big fan of useless calculation just because it
> happens to be needed in an Assert-enabled build.

Well, those planner_rt_fetch() calls are going to reduce to a simple
array lookup, so it seems rather extreme to insist on contorting the
code just to avoid that.  It's not like these functions are trivially
cheap otherwise.

In fact, I kind of wonder why we're using planner_rt_fetch() at all in
costsize.c, rather than "root->simple_rte_array[rel->relid]".  Maybe at
one time these functions were invokable before reaching query_planner(),
but we don't do that anymore.  (Just to be sure, I stuck
"Assert(root->simple_rte_array)" into each costsize.c function that uses
planner_rt_fetch, and it still passes check-world.)

So now my proposal is

/* Should only be applied to base relations that are subqueries */
Assert(rel->relid > 0);
-#ifdef USE_ASSERT_CHECKING
-   rte = planner_rt_fetch(rel->relid, root);
+   rte = root->simple_rte_array[rel->relid];
Assert(rte->rtekind == RTE_SUBQUERY);
-#endif

and make the rest of costsize.c look like that too.

regards, tom lane


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

Re: [HACKERS] SUBSCRIPTIONS and pg_upgrade

[Peter Eisentraut]

OK, we need to come to a conclusion here.  To summarize:

Problem 1: pg_subscription.subconninfo can only be read by superuser.
So non-superusers cannot dump subscriptions.

Precedent is pg_user_mapping.  In that case, we just omit the
user-mapping options if we're not a superuser.  Pretty dubious, but in
any case that won't work here, because you cannot create a subscription
without a CONNECTION clause.

Proposal: Dump subscriptions if running as superuser.  If not, check if
there are subscriptions and warn about that.  Remove current pg_dump
--include-subscriptions option.


Problem 2: Restoring a subscription immediately starts replication.
Maybe you want that or maybe you don't.  We could dump all subscriptions
in DISABLED state.  But then after restoring you have to go and manually
enable all subscriptions.  We don't have a command to turn all
subscriptions back on at once.  Maybe that is not a terrible issue,
since one wouldn't normally have many subscriptions.

Proposal: Dump all subscriptions in DISABLED state.  Remove current
pg_dump --no-subscription-connect 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] Variable substitution in psql backtick expansion

[Pavel Stehule]

2017-04-10 13:07 GMT+02:00 Greg Stark :

> On 2 April 2017 at 07:53, Fabien COELHO  wrote:
> > Note that this is already available indirectly, as show in the
> > documentation.
> >
> >   SELECT some-boolean-expression AS okay \gset
> >   \if :okay
> > \echo boolean expression was true
> >   \else
> > \echo boolean expression was false
> >   \endif
>
>
> Am I the only one who thinks that even if \if got the ability to
> evaluate arbitrary SQL queries I would probably still always write
> things as above? I think putting arbitrary SQL expressions (let alone
> queries) would make scripts just a total mess and impossible for
> humans to parse.
>

Totally agree.


> Whereas storing the results in psql variables and then using those
> variables in \if makes even fairly complex queries and nested \if
> structures straightforward.  It would also make it far clearer in what
> order the queries will be evaluated and under which set of conditions.
>
> I don't think taking a simple command line execution environment like
> psql and trying to embed a complete complex language parser in it is
> going to result in a sensible programming environment. Having a simple
> \if  is already pushing it. If someone wanted
> anything more complex I would strongly recommend switching to perl or
> python before trying to code up nesting arbitrary sql in nested
> expressions.
>

I think so some local expression evaluation could be - but it should not be
placed in \if statement

\expr issupported :VERSION_NUM >= 1
\if :issuported

maybe \if can support the basic logic predicates NOT, OR, AND - but the
operands can be only evaluated variables

Regards

Pavel





> --
> greg
>
>
> --
> 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] recent deadlock regression test failures

[Kevin Grittner]

On Mon, Apr 10, 2017 at 9:28 AM, Tom Lane  wrote:
> Thomas Munro  writes:
>> Here's a pair of draft patches for review:

Thanks.

> Pushed with cosmetic improvements.

Thanks.

> I notice that the safe-snapshot code path is not paying attention to
> parallel-query cases, unlike the lock code path.  I'm not sure how
> big a deal that is...

Parallel workers in serializable transactions should be using the
transaction number of the "master" process to take any predicate
locks, and if parallel workers are doing any DML work against
tuples, that should be using the master transaction number for
xmin/xmax and serialization failure testing.  If either of those
rules are being violated, parallel processing should probably be
disabled within a serializable transaction.  I don't think
safe-snapshot processing needs to do anything special if the above
rules are followed, nor can I see anything special it *could* do.

--
Kevin Grittner
VMware vCenter Server
https://www.vmware.com/


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

Re: [HACKERS] logical replication and SIGHUP

[Peter Eisentraut]

On 4/9/17 23:20, Michael Paquier wrote:
> After more review, I think that got_SIGTERM should be of type volatile
> sig_atomic_t in launcher.c or that's not signal-safe. I think as well
> that for correctness errno should be saved as SetLatch() is called and
> restored afterwards. Please find attached a patch to address all that.

committed

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


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

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

[Fujii Masao]

Hi,

 src/backend/replication/logical/launcher.c
 * Worker started and attached to our shmem. This check is safe
 * because only launcher ever starts the workers, so nobody can steal
 * the worker slot.

The tablesync patch enabled even worker to start another worker.
So the above assumption is not valid for now.

This issue seems to cause the corner case where the launcher picks up
the same worker slot that previously-started worker has already picked
up to start another worker.

Regards,

-- 
Fujii Masao


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

Re: [HACKERS] Compiler warning in costsize.c

[Dagfinn Ilmari Mannsåker]

Tom Lane  writes:

> I wonder if we shouldn't just do
>
>   RangeTblEntry *rte PG_USED_FOR_ASSERTS_ONLY;
>   ListCell   *lc;
>  
>   /* Should only be applied to base relations that are subqueries */
>   Assert(rel->relid > 0);
> -#ifdef USE_ASSERT_CHECKING
>   rte = planner_rt_fetch(rel->relid, root);
>   Assert(rte->rtekind == RTE_SUBQUERY);
> -#endif
>
> and eat the "useless" calculation of rte.

Why bother with the 'rte' variable at all if it's only used for the
Assert()ing the rtekind?

Assert(planner_rt_fetch(rel->relid, root)->rtekind == RTE_SUBQUERY);

- ilmari

-- 
"The surreality of the universe tends towards a maximum" -- Skud's Law
"Never formulate a law or axiom that you're not prepared to live with
 the consequences of."  -- Skud's Meta-Law


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

Re: [HACKERS] logical replication worker and statistics

[Stas Kelvich]

> On 10 Apr 2017, at 19:50, Peter Eisentraut  
> wrote:
> 
> On 4/10/17 05:49, Stas Kelvich wrote:
>> Here is small patch to call statistics in logical worker. Originally i 
>> thought that stat
>> collection during logical replication should manually account amounts of 
>> changed tuples,
>> but seems that it is already smoothly handled on relation level. So call to 
>> pgstat_report_stat() is enough.
> 
> I wonder whether we need a similar call somewhere in tablesync.c.  It
> seems to work without it, though.

I thought it spins up the same worker from worker.c.


Stas Kelvich
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company




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

Re: [HACKERS] pg_upgrade vs extension upgrades

[Tom Lane]

Magnus Hagander  writes:
> After you've run pg_upgrade, you have to loop through all your databases
> and do an "ALTER EXTENSION abc UPDATE" once for each extension.

> Is there a reason we shouldn't have pg_upgrade emit a script that does
> this, similar to how it emits a script to run ANALYZE?

+1 --- I think this has been discussed before, but nobody got round
to it.

Do we need to worry about the order of the updates when there are
cross-extension dependencies?  I'm thinking that if extension A
requires extension B, it'd be safest to update B first.

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] GSOC'17 project introduction: Parallel COPY execution with errors handling

[Robert Haas]

On Mon, Apr 10, 2017 at 11:39 AM, Alex K  wrote:
> (1) It seems that starting new subtransaction at step 4 is not necessary. We
> can just gather all error lines in one pass and at the end of input start
> the only one additional subtransaction with all safe-lines at once: [1, ...,
> k1 - 1, k1 + 1, ..., k2 - 1, k2 + 1, ...], where ki is an error line number.

The only way to recover from an error is to abort the subtransaction,
or to abort the toplevel transaction.  Anything else is unsafe.

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


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

Re: [HACKERS] pg_upgrade vs extension upgrades

[Robert Haas]

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

I don't know whether it's useful, but I doubt it would hurt anything.

-- 
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] logical replication worker and statistics

[Peter Eisentraut]

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

Discussion is ongoing, patch is proposed, should be resolved this week.

-- 
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] logical replication worker and statistics

[Peter Eisentraut]

On 4/10/17 05:49, Stas Kelvich wrote:
> Here is small patch to call statistics in logical worker. Originally i 
> thought that stat
> collection during logical replication should manually account amounts of 
> changed tuples,
> but seems that it is already smoothly handled on relation level. So call to 
> pgstat_report_stat() is enough.

I wonder whether we need a similar call somewhere in tablesync.c.  It
seems to work without it, though.

-- 
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] Compiler warning in costsize.c

[Robert Haas]

On Mon, Apr 10, 2017 at 8:30 AM, Michael Paquier
 wrote:
> On Mon, Apr 10, 2017 at 9:05 PM, Tom Lane  wrote:
>> I wonder if we shouldn't just do
>>
>> RangeTblEntry *rte PG_USED_FOR_ASSERTS_ONLY;
>> ListCell   *lc;
>>
>> /* Should only be applied to base relations that are subqueries */
>> Assert(rel->relid > 0);
>> -#ifdef USE_ASSERT_CHECKING
>> rte = planner_rt_fetch(rel->relid, root);
>> Assert(rte->rtekind == RTE_SUBQUERY);
>> -#endif
>>
>> and eat the "useless" calculation of rte.
>
> That works as well. Now this code really has been written so as we
> don't want to do this useless computation for non-Assert builds,
> that's why I did not suggest it. But as it does just a list_nth call,
> that's not really costly... And other code paths dealing with the cost
> do it as well.

-1 from me.  I'm not a big fan of useless calculation just because it
happens to be needed in an Assert-enabled build.

-- 
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] tuple-routing and constraint violation error message, revisited

[Robert Haas]

On Mon, Apr 10, 2017 at 8:14 AM, Ashutosh Bapat
 wrote:
> On Fri, Mar 31, 2017 at 7:43 AM, Amit Langote
>  wrote:
>> Summary is: We decided in f1b4c771ea7 [1] that passing the original slot
>> (one containing the tuple formatted per root partitioned table's tupdesc)
>> to ExecConstraints(), but that breaks certain cases.  Imagine what would
>> happen if a BR insert trigger changed the tuple - the original slot would
>> not contain those changes. So, it seems better to convert (if necessary)
>> the tuple formatted per partition tupdesc after tuple-routing back to the
>> root table's format and use the converted tuple to make val_desc shown in
>> the message if an error occurs.
>>
>> Attached rebased version of the patch that I had originally proposed
>> (summary above is the commit message).  Robert thought it would be fine to
>> show the row formatted per partition rowtype, but would look better if we
>> could show the column names as well (remember that we're trying to account
>> for possible differences in the ordering of columns between the root table
>> and leaf partitions to which tuples are routed.)
>>
>> Added this to PostgreSQL 10 open items list.
>
> The changes look good to me. Now, ExecConstraint() has three blocks
> which are almost similar, differing only in the constraints checked
> and the error message. It was manageable without partitioning and may
> be it's still manageable, but it's certainly being pushed to the
> limits. May be we should refactor error reporting code into a separate
> function and call it in those three places.

Yeah, possibly.

Thanks for the review.  I have committed the patch.

-- 
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] src/interfaces/libpq shipping nmake-related Makefiles

[Tom Lane]

Magnus Hagander  writes:
 Are these votes for getting rid of both win32.mak and bcc32.mak?

> PFA a patch that does this. Did I miss something? :)

Perhaps we should get rid of the WIN32_ONLY_COMPILER symbol altogether;
given this patch, "#ifdef WIN32_ONLY_COMPILER" could be replaced by
"#ifdef _MSC_VER".

Or maybe rename it to something else --- I'm not sure what, but
I've always found that symbol rather confusing.

Looks good other than that nitpick.

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] max_sync_workers_per_subscription is missing in postgresql.conf

[Fujii Masao]

On Mon, Apr 10, 2017 at 9:39 PM, Masahiko Sawada  wrote:
> On Mon, Apr 10, 2017 at 9:32 PM, Petr Jelinek
>  wrote:
>> On 10/04/17 07:16, Masahiko Sawada wrote:
>>> Hi all,
>>>
>>> Attached a patch for $subject.
>>>
>>> I added this parameter into "Asynchronous Behavior" section of
>>> "RESOURCE" section. But GUC parameter for subscriber now is written in
>>> this section, in spite of there is "REPLICATION" section. I think that
>>> we can coordinate these parameters to not confuse user. For example in
>>> documentation, these parameters are described in "19.6.4. Subscribers"
>>> section of "19.6. Replication" section. Thought?

Yes, I think that we should not only add the parameter into
postgresql.conf.sample
but also

- add REPLICATION_SUBSCRIBERS into config_group
- mark max_logical_replication_workers and max_sync_workers_per_subscription
  as REPLICATION_SUBSCRIBERS parameters, in guc.c
- move those parameters into "Subscribers" section in postgresql.conf.sample

The attached patch does these.

Regards,

-- 
Fujii Masao


Add_max_sync_workers_per_subscription_into_postgresql_conf_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] GCC 7 warnings

[Tom Lane]

Peter Eisentraut  writes:
> Possible fixes:

> a) Ignore, hoping GCC will change before final release. (unlikely at
> this point)

> b) Add compiler option to disable this particular warning, worry about
> it later.  (Might be an option for backpatching.)

> c) Expand the target buffer sizes until the warning goes away.  (Sample
> patch attached.)

> d) Replace most of the problematic code with psprintf() and dynamically
> sized buffers.

+1 for (c) as you have it.  Later we might think about selectively
doing (d), but it seems like more work for probably not much benefit.

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: Adding lfirst_node (was Re: [HACKERS] [sqlsmith] Planner crash on foreign table join)

[Andres Freund]

On 2017-04-10 12:20:16 -0400, Tom Lane wrote:
> Barring objections, I'll push this shortly.

+1, to just about all of it


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

Adding lfirst_node (was Re: [HACKERS] [sqlsmith] Planner crash on foreign table join)

[Tom Lane]

I wrote:
> Andrew Gierth  writes:
>> In a discussion with Andres on the hash grouping sets review thread, I
>> proposed that we should have something of the form

>> #define lfirst_node(_type_, l) (castNode(_type_,lfirst(l)))

> That seems like a fairly good idea.  A significant fraction of the
> existing castNode() calls are being applied to lfirst(something),
> and this would shorten that idiom a bit.

PFA a patch to do this.  It turns out that just under half of the
castNode() calls in the current tree have List-cell-extraction
functions as arguments, and so can be replaced like this.  So I think
this is a great idea and we should do it; it's a definite notational
improvement.

As with the original addition of castNode, it seems like a good idea
to back-patch the additions to pg_list.h, so that we won't have
back-patching problems for new code using this feature.

> There's another noticeable fraction that are being applied to
> linitial(something), but I'm not sure if defining linitial_node()
> is worth the trouble.

It is, and in fact I ended up providing equivalents for all the
List-cell-extraction functions.  All except lfourth_node() are
actually in use in this patch.

Barring objections, I'll push this shortly.

regards, tom lane

diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index 6b7503d..bf03e67 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -2393,7 +2393,7 @@ JumbleRangeTable(pgssJumbleState *jstate, List *rtable)
 
 	foreach(lc, rtable)
 	{
-		RangeTblEntry *rte = castNode(RangeTblEntry, lfirst(lc));
+		RangeTblEntry *rte = lfirst_node(RangeTblEntry, lc);
 
 		APP_JUMB(rte->rtekind);
 		switch (rte->rtekind)
@@ -2656,7 +2656,7 @@ JumbleExpr(pgssJumbleState *jstate, Node *node)
 JumbleExpr(jstate, (Node *) caseexpr->arg);
 foreach(temp, caseexpr->args)
 {
-	CaseWhen   *when = castNode(CaseWhen, lfirst(temp));
+	CaseWhen   *when = lfirst_node(CaseWhen, temp);
 
 	JumbleExpr(jstate, (Node *) when->expr);
 	JumbleExpr(jstate, (Node *) when->result);
diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c
index 129bdb5..c5149a0 100644
--- a/contrib/postgres_fdw/deparse.c
+++ b/contrib/postgres_fdw/deparse.c
@@ -1350,7 +1350,7 @@ deparseExplicitTargetList(List *tlist, List **retrieved_attrs,
 
 	foreach(lc, tlist)
 	{
-		TargetEntry *tle = castNode(TargetEntry, lfirst(lc));
+		TargetEntry *tle = lfirst_node(TargetEntry, lc);
 
 		if (i > 0)
 			appendStringInfoString(buf, ", ");
diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index 2851869..6670df5 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -1169,7 +1169,7 @@ postgresGetForeignPlan(PlannerInfo *root,
 	 */
 	foreach(lc, scan_clauses)
 	{
-		RestrictInfo *rinfo = castNode(RestrictInfo, lfirst(lc));
+		RestrictInfo *rinfo = lfirst_node(RestrictInfo, lc);
 
 		/* Ignore any pseudoconstants, they're dealt with elsewhere */
 		if (rinfo->pseudoconstant)
@@ -5022,8 +5022,8 @@ conversion_error_callback(void *arg)
 		EState	   *estate = fsstate->ss.ps.state;
 		TargetEntry *tle;
 
-		tle = castNode(TargetEntry, list_nth(fsplan->fdw_scan_tlist,
-			 errpos->cur_attno - 1));
+		tle = list_nth_node(TargetEntry, fsplan->fdw_scan_tlist,
+			errpos->cur_attno - 1);
 
 		/*
 		 * Target list can have Vars and expressions.  For Vars, we can get
diff --git a/src/backend/catalog/objectaddress.c b/src/backend/catalog/objectaddress.c
index 1eb7930..1492722 100644
--- a/src/backend/catalog/objectaddress.c
+++ b/src/backend/catalog/objectaddress.c
@@ -854,7 +854,7 @@ get_object_address(ObjectType objtype, Node *object,
 
 	objlist = castNode(List, object);
 	domaddr = get_object_address_type(OBJECT_DOMAIN,
-	  castNode(TypeName, linitial(objlist)),
+	  linitial_node(TypeName, objlist),
 	  missing_ok);
 	constrname = strVal(lsecond(objlist));
 
@@ -932,8 +932,8 @@ get_object_address(ObjectType objtype, Node *object,
 break;
 			case OBJECT_CAST:
 {
-	TypeName   *sourcetype = castNode(TypeName, linitial(castNode(List, object)));
-	TypeName   *targettype = castNode(TypeName, lsecond(castNode(List, object)));
+	TypeName   *sourcetype = linitial_node(TypeName, castNode(List, object));
+	TypeName   *targettype = lsecond_node(TypeName, castNode(List, object));
 	Oid			sourcetypeid;
 	Oid			targettypeid;
 
@@ -947,7 +947,7 @@ get_object_address(ObjectType objtype, Node *object,
 break;
 			case OBJECT_TRANSFORM:
 {
-	TypeName   *typename = castNode(TypeName, linitial(castNode(List, object)));
+	TypeName   *typename = linitial_node(TypeName, castNode(List, object));
 	char	   *langname = strVal(lsecond(castNode(List, object)));
 	Oid			

Re: [HACKERS] GCC 7 warnings

[Andres Freund]

On 2017-04-10 09:10:07 -0700, Andres Freund wrote:
> Hi,
> 
> On 2017-04-10 11:03:23 -0400, Peter Eisentraut wrote:
> > The release of GCC 7 is approaching [0], and the number of warnings in
> > PostgreSQL has gone up since we last looked [1].  Output attached.  (My
> > version is 7.0.1 20170408.)
> > 
> > Most of the issues have to do with concatenating two or more strings of
> > potential size MAXPGPATH into another buffer of size MAXPGPATH, which
> > could lead to truncation.
> 
> Hm, interesting - I don't see this with
> gcc-7 (Debian 7-20170407-1) 7.0.1 20170407 (experimental) [trunk revision 
> 246759]
> although I do recall having seen them at some point.  Not sure what's up
> there.

Hm. That's with -Wformat-truncation=1 (which is what's added by -Wextra
afaics), I see a good chunk more with -Wformat-truncation=2 - but that's
not enabled by -Wall/extra, so I don't really see a problem right now?

- 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] GCC 7 warnings

[Andres Freund]

Hi,

On 2017-04-10 11:03:23 -0400, Peter Eisentraut wrote:
> The release of GCC 7 is approaching [0], and the number of warnings in
> PostgreSQL has gone up since we last looked [1].  Output attached.  (My
> version is 7.0.1 20170408.)
> 
> Most of the issues have to do with concatenating two or more strings of
> potential size MAXPGPATH into another buffer of size MAXPGPATH, which
> could lead to truncation.

Hm, interesting - I don't see this with
gcc-7 (Debian 7-20170407-1) 7.0.1 20170407 (experimental) [trunk revision 
246759]
although I do recall having seen them at some point.  Not sure what's up
there.

I've to add -Wno-implicit-fallthrough to avoid noisyness, though.

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

[HACKERS] Repetitive code in RI triggers

[Ildar Musin]

Hi all,

I was looking through the RI triggers code recently and noticed a few 
almost identical functions, e.g. ri_restrict_upd() and 
ri_restrict_del(). The following patch is an attempt to reduce some of 
repetitive code. Yet there is still room for improvement.


Thanks,
--
Ildar Musin
i.mu...@postgrespro.ru
diff --git a/src/backend/utils/adt/ri_triggers.c b/src/backend/utils/adt/ri_triggers.c
index 37139f9..259c988 100644
--- a/src/backend/utils/adt/ri_triggers.c
+++ b/src/backend/utils/adt/ri_triggers.c
@@ -199,8 +199,9 @@ static int	ri_constraint_cache_valid_count = 0;
 static bool ri_Check_Pk_Match(Relation pk_rel, Relation fk_rel,
   HeapTuple old_row,
   const RI_ConstraintInfo *riinfo);
-static Datum ri_restrict_del(TriggerData *trigdata, bool is_no_action);
-static Datum ri_restrict_upd(TriggerData *trigdata, bool is_no_action);
+static Datum ri_restrict(TriggerData *trigdata, bool is_no_action, bool is_update);
+static Datum ri_setnull(TriggerData *trigdata, bool is_update);
+static Datum ri_setdefault(FunctionCallInfo fcinfo, bool is_update);
 static void quoteOneName(char *buffer, const char *name);
 static void quoteRelationName(char *buffer, Relation rel);
 static void ri_GenerateQual(StringInfo buf,
@@ -608,7 +609,7 @@ RI_FKey_noaction_del(PG_FUNCTION_ARGS)
 	/*
 	 * Share code with RESTRICT case.
 	 */
-	return ri_restrict_del((TriggerData *) fcinfo->context, true);
+	return ri_restrict((TriggerData *) fcinfo->context, true, false);
 }
 
 /* --
@@ -633,175 +634,10 @@ RI_FKey_restrict_del(PG_FUNCTION_ARGS)
 	/*
 	 * Share code with NO ACTION case.
 	 */
-	return ri_restrict_del((TriggerData *) fcinfo->context, false);
+	return ri_restrict((TriggerData *) fcinfo->context, false, false);
 }
 
 /* --
- * ri_restrict_del -
- *
- *	Common code for ON DELETE RESTRICT and ON DELETE NO ACTION.
- * --
- */
-static Datum
-ri_restrict_del(TriggerData *trigdata, bool is_no_action)
-{
-	const RI_ConstraintInfo *riinfo;
-	Relation	fk_rel;
-	Relation	pk_rel;
-	HeapTuple	old_row;
-	RI_QueryKey qkey;
-	SPIPlanPtr	qplan;
-	int			i;
-
-	/*
-	 * Get arguments.
-	 */
-	riinfo = ri_FetchConstraintInfo(trigdata->tg_trigger,
-	trigdata->tg_relation, true);
-
-	/*
-	 * Get the relation descriptors of the FK and PK tables and the old tuple.
-	 *
-	 * fk_rel is opened in RowShareLock mode since that's what our eventual
-	 * SELECT FOR KEY SHARE will get on it.
-	 */
-	fk_rel = heap_open(riinfo->fk_relid, RowShareLock);
-	pk_rel = trigdata->tg_relation;
-	old_row = trigdata->tg_trigtuple;
-
-	switch (riinfo->confmatchtype)
-	{
-			/* --
-			 * SQL:2008 15.17 
-			 *	General rules 9) a) iv):
-			 *		MATCH SIMPLE/FULL
-			 *			... ON DELETE RESTRICT
-			 * --
-			 */
-		case FKCONSTR_MATCH_SIMPLE:
-		case FKCONSTR_MATCH_FULL:
-			switch (ri_NullCheck(old_row, riinfo, true))
-			{
-case RI_KEYS_ALL_NULL:
-case RI_KEYS_SOME_NULL:
-
-	/*
-	 * No check needed - there cannot be any reference to old
-	 * key if it contains a NULL
-	 */
-	heap_close(fk_rel, RowShareLock);
-	return PointerGetDatum(NULL);
-
-case RI_KEYS_NONE_NULL:
-
-	/*
-	 * Have a full qualified key - continue below
-	 */
-	break;
-			}
-
-			/*
-			 * If another PK row now exists providing the old key values, we
-			 * should not do anything.  However, this check should only be
-			 * made in the NO ACTION case; in RESTRICT cases we don't wish to
-			 * allow another row to be substituted.
-			 */
-			if (is_no_action &&
-ri_Check_Pk_Match(pk_rel, fk_rel, old_row, riinfo))
-			{
-heap_close(fk_rel, RowShareLock);
-return PointerGetDatum(NULL);
-			}
-
-			if (SPI_connect() != SPI_OK_CONNECT)
-elog(ERROR, "SPI_connect failed");
-
-			/*
-			 * Fetch or prepare a saved plan for the restrict delete lookup
-			 */
-			ri_BuildQueryKey(, riinfo, RI_PLAN_RESTRICT_DEL_CHECKREF);
-
-			if ((qplan = ri_FetchPreparedPlan()) == NULL)
-			{
-StringInfoData querybuf;
-char		fkrelname[MAX_QUOTED_REL_NAME_LEN];
-char		attname[MAX_QUOTED_NAME_LEN];
-char		paramname[16];
-const char *querysep;
-Oid			queryoids[RI_MAX_NUMKEYS];
-
-/* --
- * The query string built is
- *	SELECT 1 FROM ONLY  x WHERE $1 = fkatt1 [AND ...]
- *		   FOR KEY SHARE OF x
- * The type id's for the $ parameters are those of the
- * corresponding PK attributes.
- * --
- */
-initStringInfo();
-quoteRelationName(fkrelname, fk_rel);
-appendStringInfo(, "SELECT 1 FROM ONLY %s x",
- fkrelname);
-querysep = "WHERE";
-for (i = 0; i < riinfo->nkeys; i++)
-{
-	Oid			pk_type = RIAttType(pk_rel, riinfo->pk_attnums[i]);
-	Oid			fk_type = RIAttType(fk_rel, riinfo->fk_attnums[i]);
-
-	quoteOneName(attname,
- RIAttName(fk_rel, riinfo->fk_attnums[i]));
-	sprintf(paramname, "$%d", i + 1);
-	ri_GenerateQual(, querysep,
-	paramname, pk_type,
-	

Re: [HACKERS] GSOC'17 project introduction: Parallel COPY execution with errors handling

[Alex K]

Hi Alexander!

I've missed your reply, since proposal submission deadline have passed last
Monday and I didn't check hackers mailing list too frequently.

(1) It seems that starting new subtransaction at step 4 is not necessary.
We can just gather all error lines in one pass and at the end of input
start the only one additional subtransaction with all safe-lines at once:
[1, ..., k1 - 1, k1 + 1, ..., k2 - 1, k2 + 1, ...], where ki is an error
line number.

But assuming that the only livable use-case is when number of errors is
relatively small compared to the total rows number, because if the input is
in totally inconsistent format, then it seems useless to import it into the
db. Thus, it is not 100% clear for me, would it be any real difference in
performance, if one starts new subtransaction at step 4 or not.

(2) Hmm, good question. As far as I know it is impossible to get stdin
input size, thus it is impossible to distribute stdin directly to the
parallel workers. The first approach which comes to the mind is to store
stdin input in any kind of buffer/query and next read it in parallel by
workers. The question is how it will perform in the case of large file, I
guess poor, at least from the memory consumption perspective. But would
parallel execution still be faster is the next question.


Alexey



On Thu, Apr 6, 2017 at 4:47 PM, Alexander Korotkov <
a.korot...@postgrespro.ru> wrote:

> Hi, Alexey!
>
> On Tue, Mar 28, 2017 at 1:54 AM, Alexey Kondratov <
> kondratov.alek...@gmail.com> wrote:
>
>> Thank you for your responses and valuable comments!
>>
>> I have written draft proposal https://docs.google.c
>> om/document/d/1Y4mc_PCvRTjLsae-_fhevYfepv4sxaqwhOo4rlxvK1c/edit
>>
>> It seems that COPY currently is able to return first error line and error
>> type (extra or missing columns, type parse error, etc).
>> Thus, the approach similar to the Stas wrote should work and, being
>> optimised for a small number of error rows, should not
>> affect COPY performance in such case.
>>
>> I will be glad to receive any critical remarks and suggestions.
>>
>
> I've following questions about your proposal.
>
> 1. Suppose we have to insert N records
>> 2. We create subtransaction with these N records
>> 3. Error is raised on k-th line
>> 4. Then, we can safely insert all lines from 1st and till (k - 1)
>>
> 5. Report, save to errors table or silently drop k-th line
>> 6. Next, try to insert lines from (k + 1) till N with another
>> subtransaction
>> 7. Repeat until the end of file
>
>
> Do you assume that we start new subtransaction in 4 since subtransaction
> we started in 2 is rolled back?
>
> I am planning to use background worker processes for parallel COPY
>> execution. Each process will receive equal piece of the input file. Since
>> file is splitted by size not by lines, each worker will start import from
>> the first new line to do not hit a broken line.
>
>
> I think that situation when backend is directly reading file during COPY
> is not typical.  More typical case is \copy psql command.  In that case
> "COPY ... FROM stdin;" is actually executed while psql is streaming the
> data.
> How can we apply parallel COPY in this case?
>
> --
> Alexander Korotkov
> Postgres Professional: http://www.postgrespro.com
> The Russian Postgres Company
>

Re: [HACKERS] GCC 7 warnings

[Aleksander Alekseev]

Hi Peter,

> c) Expand the target buffer sizes until the warning goes away.  (Sample
> patch attached.)

I personally think it's a great patch. Unfortunately I don't have GCC
7.0 right now but at least it doesn't break anything on 6.3.1. Since
there is no rush I would suggest to add an entry to the next commitfest,
so this patch wouldn't accidentally be lost.

As a side node I believe it would be great to replace all sprintf calls
to snprintf just to be on a safe side. IIRC we did something similar to
str* procedures not a long time ago. Unless there are any objections
regarding this idea I'm willing to write such a patch.

-- 
Best regards,
Aleksander Alekseev


signature.asc
Description: PGP signature

Re: [HACKERS] Merge join for GiST

[Robert Haas]

On Mon, Apr 10, 2017 at 7:53 AM, Andrew Borodin  wrote:
> I think this idea is somewhat related to this patch [2], but as for
> now cannot describe how exactly GiST merge and Range Merge features
> relate.

It also seems somewhat related to Peter Moser's work on ALIGN and
NORMALIZE.  It would be nice if the various groups of people
interested in improving PostgreSQL's spatial stuff got together and
reviewed each others' patches.  As a non-spatial guy myself, it's
pretty hard to decide on the relative merits of different proposed
approaches.

-- 
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] RMT: Use Visual Studio 2015 for Compiling and linking the Windows version in PG10

[Robert Haas]

On Mon, Apr 10, 2017 at 11:21 AM, Hans Buschmann  wrote:
> On 03/28/2017 06:46 AM, I wrote in pgsql-general:
>> I use Postgres on Windows 64 bit (8.1,10,WS2012R2,WS2016) for quite a
>> while.
>> I always install and upgrade from the ZIP binary distribution from
>> enterpriseDB which works like a charm.
>>
>> In a recent fresh install I noticed that PostgreSQL 9.6.2 (older
>> versions not tested anymore) requires Visual C++ 2013 Runtime library to
>> be installed.
>>
>> It seems all binaries are linked against msvcr120.dll (verified with
>> ProcExplorer of Sysinternals).
>>
>> I propose to switch to Visual Studio 2015 for the next distribution:
>> - no more install of > 5 years old runtime library
>> - seems to need no extra runtime library on newer systems (there is no
>> msvcr140.dll after installation of VCR 2015)
>> - when needed, VC Runtime Library 2015 is the same requirement as for
>> recent Apache 2.4.xx versions
>> - VS2015 build is already member of the build farm
>> - I could easyly test the beta on recent Windows 64 OSs
>>
>> BTW, nowhere in the current documentation is mentioned the requirement
>> of the VC++ runtime library installation.
>>
>> Thanks in advance
>
> Now after feature freeze of the new version I wanted to address this topic
> to the Release Management Team.
>
> Opinions?

The one response you got on pgsql-general suggests that this isn't a
proposed change to PostgreSQL but rather a proposed change to
EnterpriseDB's packaging of PostgreSQL.  The RMT has no jurisdiction
over EnterpriseDB's packaging of PostgreSQL, and I have no personal
knowledge of that topic.  You could contact Dave Page about that
issue, perhaps.

If there is a change to PostgreSQL itself proposed, the RMT could
consider granting an extension to feature freeze for that change if
the larger PostgreSQL development community felt that the change was
worthwhile, urgent, and low-risk.  However, the RMT would probably not
want to make such a decision about the matter without hearing from
those community members who know something about Windows, or in
advance of the patch having been written.

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

[Magnus Hagander]

After you've run pg_upgrade, you have to loop through all your databases
and do an "ALTER EXTENSION abc UPDATE" once for each extension.

Is there a reason we shouldn't have pg_upgrade emit a script that does
this, similar to how it emits a script to run ANALYZE?

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

Re: [HACKERS] pg_dump emits ALTER TABLE ONLY partitioned_table

[Robert Haas]

On Sun, Apr 9, 2017 at 10:10 PM, Tom Lane  wrote:
> While I admit that I've not been paying close attention to the whole
> table partitioning business, I wonder whether we have any clearly written
> down specification about (a) how much partition member tables are allowed
> to deviate schema-wise from their parent, and (b) how DDL semantics on
> partitioned tables differ from DDL semantics for traditional inheritance.
> Obviously those are closely related questions.  But the fact that this
> bug exists at all shows that there's been some lack of clarity on (b),
> and so I wonder whether we have any clarity on (a) either.

Children can have constraints (including NOT NULL constraints) which
parents lack, and can have a different column order, but must have
exactly the same column names and types.

The point here is, of course, not that there's any real value in the
parent columns being (a, b) and the child columns being (b, a),
although we do allow that, but rather than somebody might have a
parent with (a, b) and a child that has those plus a dropped column.
Explaining to a user - to whom the dropped column is invisible - why
that child couldn't be attached as a partition of that parent would be
difficult, so it seemed best (to me, anyway, and I think to other
people who were paying attention) to rule that the partitioning code
has to cope with the possibility of attribute numbers varying across
partitions.  (Also consider the reverse case, where the parent has a
dropped column and the prospective child doesn't have one with the
same width in the same location.)

In Amit's example from the original post, the child has an implicit
NOT NULL constraint that does not exist in the parent.  p1.b isn't
declared NOT NULL, but the fact that it is range-partitioned on b
requires it to be so, just as we would do if b were declared as the
PRIMARY KEY.  Somehow that's not playing nice with pg_dump, but I'm
still fuzzy on the details.

-- 
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] RMT: Use Visual Studio 2015 for Compiling and linking the Windows version in PG10

[Hans Buschmann]

On 03/28/2017 06:46 AM, I wrote in pgsql-general:
>
> I use Postgres on Windows 64 bit (8.1,10,WS2012R2,WS2016) for quite a while.
> I always install and upgrade from the ZIP binary distribution from
> enterpriseDB which works like a charm.
>
> In a recent fresh install I noticed that PostgreSQL 9.6.2 (older
> versions not tested anymore) requires Visual C++ 2013 Runtime library to
> be installed.
>
> It seems all binaries are linked against msvcr120.dll (verified with
> ProcExplorer of Sysinternals).
>
> I propose to switch to Visual Studio 2015 for the next distribution:
> - no more install of > 5 years old runtime library
> - seems to need no extra runtime library on newer systems (there is no
> msvcr140.dll after installation of VCR 2015)
> - when needed, VC Runtime Library 2015 is the same requirement as for
> recent Apache 2.4.xx versions
> - VS2015 build is already member of the build farm
> - I could easyly test the beta on recent Windows 64 OSs
>
> BTW, nowhere in the current documentation is mentioned the requirement
> of the VC++ runtime library installation.
>
> Thanks in advance
>

Now after feature freeze of the new version I wanted to address this topic to 
the Release Management Team.

Opinions?


 Hans Buschmann

Re: [HACKERS] strange parallel query behavior after OOM crashes

[Robert Haas]

[ Adding Julien, whose patch this was. ]

On Thu, Apr 6, 2017 at 5:34 AM, Kuntal Ghosh  wrote:
> While performing StartupDatabase, both master and standby server
> behave in similar way till postmaster spawns startup process.
> In master, startup process completes its job and dies. As a result,
> reaper is called which in turn calls maybe_start_bgworker().
> In standby, after getting a valid snapshot, startup process sends
> postmaster a signal to enable connections. Signal handler in
> postmaster calls maybe_start_bgworker().
> In maybe_start_bgworker(), if we find a crashed bgworker(crashed_at !=
> 0) with a NEVER RESTART flag, we call ForgetBackgroundWorker().to
> forget the bgworker process.
>
> I've attached the patch for adding an argument in
> ForgetBackgroundWorker() to indicate a crashed situation. Based on
> that, we can take the necessary actions. I've not included the Assert
> statement in this patch.

This doesn't seem right, because this will potentially pass wasCrashed
= true even if there has been no crash-and-restart cycle.  The place
where you're passing "true" only knows that rw->rw_crashed_at != 0 &&
rw->rw_worker.bgw_restart_time == BGW_NEVER_RESTART, but that doesn't
prove much of anything.  Some *other* background worker could have
crashed, rather than the one at issue.  Even there's only one worker
involved, the test for whether to call HandleChildCrash() involves
other factors:

if ((rw->rw_worker.bgw_flags & BGWORKER_SHMEM_ACCESS) != 0)
{
if (!EXIT_STATUS_0(exitstatus) && !EXIT_STATUS_1(exitstatus))
{
HandleChildCrash(pid, exitstatus, namebuf);
return true;
}
}

if (!ReleasePostmasterChildSlot(rw->rw_child_slot) &&
(rw->rw_worker.bgw_flags & BGWORKER_SHMEM_ACCESS) != 0)
{
HandleChildCrash(pid, exitstatus, namebuf);
return true;
}

After some study, I think the solution here is as follows:

1. Forget BGW_NEVER_RESTART workers in
ResetBackgroundWorkerCrashTimes() rather than leaving them around to
be cleaned up after the conclusion of the restart, so that they go
away before rather than after shared memory is reset.

2. Don't allow BGWORKER_CLASS_PARALLEL workers to be anything other
than BGW_NEVER_RESTART, so that (1) suffices to guarantee that, after
recovering from a crash, 0 is the correct starting value for
parallel_register_count.  (Alternatively, we could iterate through the
worker list and compute the correct starting value for
parallel_register_count, but that seems silly since we only ever
expect BGW_NEVER_RESTART here anyway.)

The attached patch seems to fix the problem for me.  I'll commit it
tomorrow unless somebody sees a problem with it; if that happens, I'll
provide some other kind of update tomorrow.  [For clarity, for
official status update purposes, I am setting a next-update date of
tomorrow, 2017-04-11.]

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


forget-before-crash-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

[HACKERS] contrib/bloom wal-check not run by default

[Peter Eisentraut]

Why $subject?

Does it just need to be wired into the makefiles a bit better?

-- 
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] GCC 7 warnings

[Peter Eisentraut]

The release of GCC 7 is approaching [0], and the number of warnings in
PostgreSQL has gone up since we last looked [1].  Output attached.  (My
version is 7.0.1 20170408.)

Most of the issues have to do with concatenating two or more strings of
potential size MAXPGPATH into another buffer of size MAXPGPATH, which
could lead to truncation.

Possible fixes:

a) Ignore, hoping GCC will change before final release. (unlikely at
this point)

b) Add compiler option to disable this particular warning, worry about
it later.  (Might be an option for backpatching.)

c) Expand the target buffer sizes until the warning goes away.  (Sample
patch attached.)

d) Replace most of the problematic code with psprintf() and dynamically
sized buffers.

Comments?


[0]: https://gcc.gnu.org/ml/gcc/2017-03/msg00066.html
[1]:
https://www.postgresql.org/message-id/CAFj8pRA=xV0_-aDF5UtGVY8HGvg+ovA_js_P8z4jLHxvX0Wa=a...@mail.gmail.com

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
In file included from /usr/include/stdio.h:498:0,
 from ../../src/include/c.h:81,
 from ../../src/include/postgres_fe.h:25,
 from file_utils.c:15:
file_utils.c: In function 'walkdir.constprop':
file_utils.c:177:32: warning: '__builtin_snprintf' output may be truncated 
before the last format character [-Wformat-truncation=]
   snprintf(subpath, MAXPGPATH, "%s/%s", path, de->d_name);
^
file_utils.c:177:3: note: '__builtin_snprintf' output 2 or more bytes (assuming 
1025) into a destination of size 1024
   snprintf(subpath, MAXPGPATH, "%s/%s", path, de->d_name);
   ^
In file included from /usr/include/stdio.h:498:0,
 from ../../src/include/c.h:81,
 from ../../src/include/postgres.h:47,
 from pgtz.c:13:
pgtz.c: In function 'pg_tzenumerate_next':
pgtz.c:432:33: warning: '__builtin_snprintf' output may be truncated before the 
last format character [-Wformat-truncation=]
   snprintf(fullname, MAXPGPATH, "%s/%s",
 ^
pgtz.c:432:3: note: '__builtin_snprintf' output 2 or more bytes (assuming 1025) 
into a destination of size 1024
   snprintf(fullname, MAXPGPATH, "%s/%s",
   ^
In file included from /usr/include/stdio.h:498:0,
 from ../../../../src/include/c.h:81,
 from ../../../../src/include/postgres.h:47,
 from rewriteheap.c:103:
rewriteheap.c: In function 'CheckPointLogicalRewriteHeap':
rewriteheap.c:1237:29: warning: '%s' directive output may be truncated writing 
up to 1023 bytes into a region of size 1004 [-Wformat-truncation=]
   snprintf(path, MAXPGPATH, "pg_logical/mappings/%s", mapping_de->d_name);
 ^
rewriteheap.c:1237:3: note: '__builtin_snprintf' output between 21 and 1044 
bytes into a destination of size 1024
   snprintf(path, MAXPGPATH, "pg_logical/mappings/%s", mapping_de->d_name);
   ^
In file included from /usr/include/stdio.h:498:0,
 from ../../../../src/include/c.h:81,
 from ../../../../src/include/postgres.h:47,
 from xlog.c:15:
xlog.c: In function 'do_pg_start_backup':
xlog.c:10403:41: warning: '%s' directive output may be truncated writing up to 
1023 bytes into a region of size 1014 [-Wformat-truncation=]
snprintf(fullpath, sizeof(fullpath), "pg_tblspc/%s", de->d_name);
 ^
xlog.c:10403:4: note: '__builtin_snprintf' output between 11 and 1034 bytes 
into a destination of size 1024
snprintf(fullpath, sizeof(fullpath), "pg_tblspc/%s", de->d_name);
^
xlog.c: In function 'do_pg_stop_backup':
../../../../src/include/access/xlog_internal.h:131:20: warning: '%s' directive 
output may be truncated writing up to 1023 bytes into a region of size 1017 
[-Wformat-truncation=]
 #define XLOGDIR"pg_wal"
^
xlog.c:4123:31: note: in expansion of macro 'XLOGDIR'
 snprintf(path, MAXPGPATH, XLOGDIR "/%s", xlde->d_name);
   ^~~
In file included from xlog.c:35:0:
../../../../src/include/access/xlog_internal.h:203:38: note: format string is 
defined here
  snprintf(path, MAXPGPATH, XLOGDIR "/%08X%08X%08X.%08X.backup", tli, \
  ^~
In file included from /usr/include/stdio.h:498:0,
 from ../../../../src/include/c.h:81,
 from ../../../../src/include/postgres.h:47,
 from xlog.c:15:
xlog.c:4123:5: note: '__builtin_snprintf' output between 8 and 1031 bytes into 
a destination of size 1024
 snprintf(path, MAXPGPATH, XLOGDIR "/%s", xlde->d_name);
 ^
In file included from /usr/include/stdio.h:498:0,
 from ../../../src/include/c.h:81,
 from ../../../src/include/postgres.h:47,
 from pgstat.c:19:
pgstat.c: In function 'pgstat_reset_remove_files':
pgstat.c:639:30: 

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

[Jeevan Ladhe]

Hi Rahila,

I tried to review the code, and here are some of my early comments:

1.
When I configure using "-Werror", I see unused variable in function
DefineRelation:

tablecmds.c: In function ‘DefineRelation’:
tablecmds.c:761:17: error: unused variable ‘partdesc’
[-Werror=unused-variable]
   PartitionDesc partdesc;
 ^

2.
Typo in comment:
+ /*
+ * When adding a list partition after default partition, scan the
+ * default partiton for rows satisfying the new partition
+ * constraint. If found dont allow addition of a new partition.
+ * Otherwise continue with the creation of new  partition.
+ */

partition
don't

3.
I think instead of a period '.', it will be good if you can use semicolon
';'
in following declaration similar to the comment for 'null_index'.

+ int def_index; /* Index of the default list partition. -1 for
+ * range partitioned tables */

4.
You may want to consider 80 column alignment for changes done in function
get_qual_from_partbound, and other places as applicable.

5.
It would be good if the patch has some test coverage that explains what is
being achieved, what kind of error handling is done etc.

6.
There are some places having code like following:

+ Node *value = lfirst(c);
  Const   *val = lfirst(c);
  PartitionListValue *list_value = NULL;

+ if (IsA(value, DefElem))

The additional variable is not needed and you can call IsA on val itself.

7.
Also, in places like below where you are just trying to check for node is a
DefaultElem, you can avoid an extra variable:

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

Can be written as:
+ foreach(cell1, bspec->listdatums)
+ {
+ if (IsA(lfirst(cell1), DefElem))
+ {
+ def_elem = true;
+ *defid = inhrelid;
+ }
+ }


Regards,
Jeevan Ladhe



On Mon, Apr 10, 2017 at 8:12 PM, Jeevan Ladhe  wrote:

> Hi Rahila,
>
>
> With your latest patch:
>
> Consider a case when a table is partitioned on a boolean key.
>
> Even when there are existing separate partitions for 'true' and
>
> 'false', still default partition can be created.
>
>
> I think this should not be allowed.
>
>
> Consider following case:
>
>
> postgres=# CREATE TABLE list_partitioned (
>
> a bool
>
> ) PARTITION BY LIST (a);
>
> CREATE TABLE
>
> postgres=# CREATE TABLE part_1 PARTITION OF list_partitioned FOR VALUES IN
> ('false');
>
> CREATE TABLE
>
> postgres=# CREATE TABLE part_2 PARTITION OF list_partitioned FOR VALUES IN
> ('true');
>
> CREATE TABLE
>
> postgres=# CREATE TABLE part_default PARTITION OF list_partitioned FOR
> VALUES IN (DEFAULT);
>
> CREATE TABLE
>
>
> The creation of table part_default should have failed instead.
>
>
> Thanks,
>
> Jeevan Ladhe
>
>
>
> On Thu, Apr 6, 2017 at 9:37 PM, Keith Fiske  wrote:
>
>>
>> On Thu, Apr 6, 2017 at 7:30 AM, Rahila Syed 
>> wrote:
>>
>>> Hello,
>>>
>>> Thanks a lot for testing and reporting this. Please find attached an
>>> updated patch with the fix. The patch also contains a fix
>>> regarding operator used at the time of creating expression as default
>>> partition constraint. This was notified offlist by Amit Langote.
>>>
>>> Thank you,
>>> Rahila Syed
>>>
>>>
>> Could probably use some more extensive testing, but all examples I had on
>> my previously mentioned blog post are now working.
>>
>> Keith
>>
>>
>

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

[Jeevan Ladhe]

Hi Rahila,


With your latest patch:

Consider a case when a table is partitioned on a boolean key.

Even when there are existing separate partitions for 'true' and

'false', still default partition can be created.


I think this should not be allowed.


Consider following case:


postgres=# CREATE TABLE list_partitioned (

a bool

) PARTITION BY LIST (a);

CREATE TABLE

postgres=# CREATE TABLE part_1 PARTITION OF list_partitioned FOR VALUES IN
('false');

CREATE TABLE

postgres=# CREATE TABLE part_2 PARTITION OF list_partitioned FOR VALUES IN
('true');

CREATE TABLE

postgres=# CREATE TABLE part_default PARTITION OF list_partitioned FOR
VALUES IN (DEFAULT);

CREATE TABLE


The creation of table part_default should have failed instead.


Thanks,

Jeevan Ladhe



On Thu, Apr 6, 2017 at 9:37 PM, Keith Fiske  wrote:

>
> On Thu, Apr 6, 2017 at 7:30 AM, Rahila Syed 
> wrote:
>
>> Hello,
>>
>> Thanks a lot for testing and reporting this. Please find attached an
>> updated patch with the fix. The patch also contains a fix
>> regarding operator used at the time of creating expression as default
>> partition constraint. This was notified offlist by Amit Langote.
>>
>> Thank you,
>> Rahila Syed
>>
>>
> Could probably use some more extensive testing, but all examples I had on
> my previously mentioned blog post are now working.
>
> Keith
>
>

Re: [HACKERS] pg_dump emits ALTER TABLE ONLY partitioned_table

[Tom Lane]

Stephen Frost  writes:
> * Tom Lane (t...@sss.pgh.pa.us) wrote:
>> While I admit that I've not been paying close attention to the whole
>> table partitioning business, I wonder whether we have any clearly written
>> down specification about (a) how much partition member tables are allowed
>> to deviate schema-wise from their parent, and (b) how DDL semantics on
>> partitioned tables differ from DDL semantics for traditional inheritance.

> Right.  I thought I had understood that partition child tables really
> can't deviate from the parent table in terms of columns or constraints
> but might be allowed to differ with regard to indexes (?).

Well, there's an awful lot of logic that's only of interest if partition
children can have different column orders from their parents.  I really
fail to understand what's the point of allowing that.

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] recent deadlock regression test failures

[Tom Lane]

Thomas Munro  writes:
> Here's a pair of draft patches for review:

Pushed with cosmetic improvements.

I notice that the safe-snapshot code path is not paying attention to
parallel-query cases, unlike the lock code path.  I'm not sure how
big a deal that is...

regards, tom lane


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

Re: [HACKERS] pg_dump emits ALTER TABLE ONLY partitioned_table

[Stephen Frost]

Tom, Robert,

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Robert Haas  writes:
> > I would appreciate help from other contributors and committers on this
> > open item; pg_dump is not my strong point.  In the absence of such
> > help, I will do my best with it.  I will set aside time this week to
> > study this and send another update no later than Thursday.
> 
> The proposed patch seems rather ad-hoc, and I think that it is working
> around a backend behavior that might be broken.

Yeah, I'm not a big fan of the proposed patch either.

> While I admit that I've not been paying close attention to the whole
> table partitioning business, I wonder whether we have any clearly written
> down specification about (a) how much partition member tables are allowed
> to deviate schema-wise from their parent, and (b) how DDL semantics on
> partitioned tables differ from DDL semantics for traditional inheritance.
> Obviously those are closely related questions.  But the fact that this
> bug exists at all shows that there's been some lack of clarity on (b),
> and so I wonder whether we have any clarity on (a) either.

Right.  I thought I had understood that partition child tables really
can't deviate from the parent table in terms of columns or constraints
but might be allowed to differ with regard to indexes (?).

I'll try looking into this also, I should be able to spend some time on
it this week.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] logical replication and SIGHUP

[Tom Lane]

Petr Jelinek  writes:
> On 10/04/17 14:40, Tom Lane wrote:
>> Umm ... you're doing what?

> We are doing:
> +   SetConfigOption("synchronous_commit",
> +   MySubscription->synccommit ?
> "local" : "off",
> +   PGC_BACKEND, PGC_S_OVERRIDE);

That looks fine.

> I don't remember from top of my head if above is safe enough against
> config reread or not, hence the note.

Yes, the override will take care of it ...

regards, tom lane


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

Re: [pgsql-www] [HACKERS] Small issue in online devel documentation build

[Bruce Momjian]

On Mon, Apr 10, 2017 at 10:17:26AM +0200, Magnus Hagander wrote:
> didn't that that happened in the old docs, but I can use an href filter
> to do it only for docs of versions starting with '1' or saying 'devel'.
> 
> That said, doing something like this would probably be cleaner. However, I
> think doing it based on href is a bad idea -- we're bound to silently break
> that somehow.
> 
> What if we changed the  tag to be something like  "docContent" class="post-10"> 
> 
> Please suggest a better name -- but the idea being we add a class do it,
> conditionally for the docs of v10 and newer (which would include devel). That
> way that logic stays where it belongs, and the CSS just uses the information 
> to
> style on.

While we could trigger on some special tag, it is best to trigger on
something the build system does anyway so that if someone builds pre-10
docs with the new build system, they get the same output.  The attached
patch uses:

div.refnamediv h2 .refentrytitle

and "div.refnamediv" is only generated in the new build system.

-- 
  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 +
diff --git a/media/css/text.css b/media/css/text.css
new file mode 100644
index 129735c..2714f83
*** a/media/css/text.css
--- b/media/css/text.css
***
*** 4,10 
  
  /* Heading Definitions */
  
! h1 {
color: #EC5800;
  }
  
--- 4,10 
  
  /* Heading Definitions */
  
! h1, div.refnamediv h2 .refentrytitle {
color: #EC5800;
  }
  

-- 
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] src/interfaces/libpq shipping nmake-related Makefiles

[Magnus Hagander]

On Mon, Apr 10, 2017 at 2:07 PM, Michael Paquier 
wrote:

> On Mon, Apr 10, 2017 at 8:35 PM, Tom Lane  wrote:
> > Magnus Hagander  writes:
> >> Are these votes for getting rid of both win32.mak and bcc32.mak?
> >
> > I'm for it.
>
> +1.
>

PFA a patch that does this. Did I miss something? :)

-- 
 Magnus Hagander
 Me: https://www.hagander.net/ 
 Work: https://www.redpill-linpro.com/ 
diff --git a/doc/src/sgml/install-windows.sgml b/doc/src/sgml/install-windows.sgml
index b95b04f..f5dfb91 100644
--- a/doc/src/sgml/install-windows.sgml
+++ b/doc/src/sgml/install-windows.sgml
@@ -35,14 +35,6 @@
  
 
  
-  Finally, the client access library
-  (libpq) can be built using
-  Visual C++ 7.1 or
-  Borland C++ for compatibility with statically
-  linked applications built using these tools.
- 
-
- 
   Building using MinGW or
   Cygwin uses the normal build system, see
and the specific notes in
@@ -539,113 +531,4 @@ $ENV{DOCROOT}='c:\docbook';
  
 
  
-
- 
-  Building libpq with
-  Visual C++ or
-  Borland C++
-
- 
-  Using Visual C++ 7.1-9.0 or
-  Borland C++ to build libpq is only recommended
-  if you need a version with different debug/release flags, or if you need a
-  static library to link into an application. For normal use the
-  MinGW or
-  Visual Studio or
-  Windows SDK method is recommended.
- 
-
- 
-  To build the libpq client library using
-  Visual Studio 7.1 or later, change into the
-  src directory and type the command:
-
-nmake /f win32.mak
-
- 
- 
- To build a 64-bit version of the libpq
- client library using Visual Studio 8.0 or
- later, change into the src
- directory and type in the command:
-
-nmake /f win32.mak CPU=AMD64
-
- See the win32.mak file for further details
- about supported variables.
- 
-
- 
-  To build the libpq client library using
-  Borland C++, change into the
-  src directory and type the command:
-
-make -N -DCFG=Release /f bcc32.mak
-
- 
-
- 
- Generated Files
- 
-  The following files will be built:
-
-  
-   
-interfaces\libpq\Release\libpq.dll
-
- 
-  The dynamically linkable frontend library
- 
-
-   
-
-   
-interfaces\libpq\Release\libpqdll.lib
-
- 
-  Import library to link your programs to libpq.dll
- 
-
-   
-
-   
-interfaces\libpq\Release\libpq.lib
-
- 
-  Static version of the frontend library
- 
-
-   
-
-  
- 
-
- 
-  Normally you do not need to install any of the client files. You should
-  place the libpq.dll file in the same directory
-  as your applications executable file. Do not install
-  libpq.dll into your Windows,
-  System or System32 directory unless
-  absolutely necessary.
-  If this file is installed using a setup program, then it should
-  be installed with version checking using the
-  VERSIONINFO resource included in the file, to
-  ensure that a newer version of the library is not overwritten.
- 
-
- 
-  If you are planning to do development using libpq
-  on this machine, you will have to add the
-  src\include and
-  src\interfaces\libpq subdirectories of the source
-  tree to the include path in your compiler's settings.
- 
-
- 
-  To use the library, you must add the
-  libpqdll.lib file to your project.  (In Visual
-  C++, just right-click on the project and choose to add it.)
- 
- 
- 
 
diff --git a/src/Makefile.shlib b/src/Makefile.shlib
index 35e2dd8..0ce6d2a 100644
--- a/src/Makefile.shlib
+++ b/src/Makefile.shlib
@@ -405,30 +405,22 @@ endif # PORTNAME == cygwin || PORTNAME == win32
 # tarballs.
 
 ifneq (,$(SHLIB_EXPORTS))
-distprep: lib$(NAME)dll.def lib$(NAME)ddll.def blib$(NAME)dll.def
+distprep: lib$(NAME)dll.def lib$(NAME)ddll.def
 
 UC_NAME = $(shell echo $(NAME) | tr 'abcdefghijklmnopqrstuvwxyz' 'ABCDEFGHIJKLMNOPQRSTUVWXYZ')
 
 lib$(NAME)dll.def: $(SHLIB_EXPORTS)
-	echo '; DEF file for win32.mak release build and for Makefile.shlib (MinGW)' >$@
+	echo '; DEF file for Makefile.shlib (MinGW)' >$@
 	echo 'LIBRARY LIB$(UC_NAME).dll' >>$@
 	echo 'EXPORTS' >>$@
 	sed -e '/^#/d' -e 's/^\(.*[ 	]\)\([0-9][0-9]*\)/\1@ \2/' $< >>$@
 
 lib$(NAME)ddll.def: $(SHLIB_EXPORTS)
-	echo '; DEF file for win32.mak debug build' >$@
+	echo '; DEF file for Makefile.shlib (MinGW)' >$@
 	echo 'LIBRARY LIB$(UC_NAME)D.dll' >>$@
 	echo 'EXPORTS' >>$@
 	sed -e '/^#/d' -e 's/^\(.*[ 	]\)\([0-9][0-9]*\)/\1@ \2/' $< >>$@
 
-blib$(NAME)dll.def: $(SHLIB_EXPORTS)
-	echo '; DEF file for bcc32.mak (Borland C++ Builder)' >$@
-	echo 'LIBRARY BLIB$(UC_NAME)' >>$@
-	echo 'EXPORTS' >>$@
-	sed -e '/^#/d' -e 's/^\(.*[ 	]\)\([0-9][0-9]*\)/_\1@ \2/' $< >>$@
-	echo >>$@
-	echo '; Aliases for MS compatible names' >> $@
-	sed -e '/^#/d' -e 's/^\(.*[ 	]\)\([0-9][0-9]*\)/\1= _\1/' $< | sed 's/ *$$//' >>$@
 endif # SHLIB_EXPORTS
 
 
@@ -517,5 +509,5 @@ clean-lib:
 
 ifneq (,$(SHLIB_EXPORTS))
 maintainer-clean-lib:
-	rm -f lib$(NAME)dll.def lib$(NAME)ddll.def 

Re: [HACKERS] [COMMITTERS] pgsql: Sync pg_dump and pg_dumpall output

[Andrew Dunstan]

On 04/10/2017 12:43 AM, Michael Paquier wrote:
> On Mon, Apr 10, 2017 at 1:01 PM, Noah Misch  wrote:
>> This PostgreSQL 10 open item is past due for your status update.  Kindly send
>> a status update within 24 hours, and include a date for your subsequent 
>> status
>> update.  Refer to the policy on open item ownership:
>> https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com
> Note for reviewers and committers: the patch sent upthread in
> https://www.postgresql.org/message-id/CAB7nPqTUOpF792rDOnBkswZ=zghwxdb01oqu2taf1ku4iuu...@mail.gmail.com
> still applies and passes all the tests.


Sorry, previous notification flew by me. I Have looked at the patch. On
the surface it looks fine and I will apply after running a test, should
be today.

cheers

andrew

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



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

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

[Heikki Linnakangas]

On 04/07/2017 01:13 AM, Michael Paquier wrote:

On Fri, Apr 7, 2017 at 5:15 AM, Álvaro Hernández Tortosa  
wrote:

I don't see it. The message AuthenticationSASL.String could contain a
CSV of the SCRAM protocols supported. This is specially important to support
channel binding (which is just another protocol name for this matter), which
is the really enhanced security mechanism of SCRAM. Since this message is
sent regardless, and the client replies with PasswordMessage, no extra round
trip is required. However, PasswordMessage needs to also include a field
with the name of the selected protocol (it is the client who picks). Or a
different message would need to be created, but no extra round-trips more
than those required by SCRAM itself (4 messages for SCRAM + 1 extra for the
server to tell the client it needs to use SCRAM).


Yes, it seems to me that the list of protocols to send should be done
by sendAuthRequest(). Then the client parses the received string, and
sends an extra 'p' message with its choice before sending the first
SCRAM message. So there is no need for any extra round trips.


I started writing down the protocol docs, based on the above idea. See 
attached. The AuthenticationSASL message now contains a list of mechanisms.


Does that seem clear to you? If so, I'll change the code to match the 
attached docs.


I added two new message formats to the docs, SASLResponse and 
SASLInitialResponse. Those use the same type byte as PasswordMessage, 
'p', but I decided to describe them as separate messages for 
documentation purposes, since the content is completely different 
depending on whether the message is sent as part of SASL, GSS, md5, or 
password authentication. IOW, this is not a change in the 
implementation, only in the way it's documented.



While working on this, and reading the RFCs more carefully, I noticed 
one detail we should change, to be spec-complicant. The SASL spec 
specifies that a SASL authentication exchange consists of 
challenge-response pairs. There must be a response to each challenge. If 
the last message in the authentication mechanism (SCRAM in this case) 
goes in the server->client direction, then that message must sent as 
"additional data" in the server->client message that tells the client 
that the authentication was successful. That's AuthenticationOK in the 
PostgreSQL protocol. In the current implementation, the 
server-final-message is sent as an AuthenticationSASLContinue message, 
and the client doesn't respond to that.


We should change that, so that the server-final-message is sent as 
"additional data" in the AuthenticationOK message. The attached docs 
patch describes that, rather than what the current implementation does.


(For your convenience, I built the HTML docs with this patch, and put 
them up at http://hlinnaka.iki.fi/temp/scram-wip-docs/protocol.html for 
viewing)


- Heikki



0001-Add-docs-for-SASL-authentication-in-protocol.patch
Description: invalid/octet-stream

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

Re: [HACKERS] logical replication and SIGHUP

[Petr Jelinek]

On 10/04/17 14:40, Tom Lane wrote:
> Petr Jelinek  writes:
>> Looks good to me. Just as a note, we'll have to handle this newly
>> supported config rereads in the async commit patch where we override
>> synchronous_commit GUC, but the config reread will change it back.
> 
> Umm ... you're doing what?
> 

We are doing:
+   SetConfigOption("synchronous_commit",
+   MySubscription->synccommit ?
"local" : "off",
+   PGC_BACKEND, PGC_S_OVERRIDE);

> There are mechanisms for making local changes to a GUC.  Just stomping
> on the variable is not an approved way to do it.
> 

I don't remember from top of my head if above is safe enough against
config reread or not, hence the note.

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


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