Re: [HACKERS] tap tests on older branches fail if concurrency is used

[Michael Paquier]

On Thu, Jun 1, 2017 at 10:48 PM, Tom Lane  wrote:
> Andres Freund  writes:
>> when using
>> $ cat ~/.proverc
>> -j9
>> some tests fail for me in 9.4 and 9.5.
>
> Weren't there fixes specifically intended to make that safe, awhile ago?

60f826c has not been back-patched. While this would fix parallel runs
with make's --jobs, PROVE_FLAGS="-j X" would still fail.
-- 
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] tap tests on older branches fail if concurrency is used

[Michael Paquier]

On Wed, May 31, 2017 at 8:45 PM, Craig Ringer  wrote:
> On 1 June 2017 at 08:15, Andres Freund  wrote:
>> Hi,
>>
>> when using
>> $ cat ~/.proverc
>> -j9
>>
>> some tests fail for me in 9.4 and 9.5.  E.g. src/bin/script's tests
>> yields a lot of fun like:
>> $ (cd ~/build/postgres/9.5-assert/vpath/src/bin/scripts/ && make check)
>> ...
>> # LOG:  received immediate shutdown request
>> # WARNING:  terminating connection because of crash of another server process
>> # DETAIL:  The postmaster has commanded this server process to roll back the 
>> current transaction and exit, because another server process exited 
>> abnormally and possibly corrupted shared memory.
>> # HINT:  In a moment you should be able to reconnect to the database and 
>> repeat your command.
>> ...
>>
>> it appears as if various tests are trampling over each other.

They are. The problem can be easily reproduced on my side with that:
PROVE_FLAGS="-j 9" make check
It would be nice to get a minimum of stability for those tests in
back-branches even if PostgresNode.pm is not back-patched.

> The immediate problem appears to be that they all use
> tmp_check/postmaster.log . So anything that examines the logs gets
> confused by seeing some other postgres instance's logs, or a mixture,
> trampling everywhere.

Amen.

> I'll be surprised if there aren't other problems though. Rather than
> trying to fix it all up, this seems like a good argument for
> backporting the updated suite from 9.6 or pg10, with PostgresNode etc.
> I already have a working tree with that done to use src/test/recovery
> in 9.5, but haven't updated src/bin/scripts etc yet.

Yup. Even if PostgresNode.pm is not back-patched, a small trick is to
append the PID of the process running the TAP test to the log file
name as in the patch attached. This gives enough uniqueness for the
tests to pass with a high parallel degree.

A second error that I have spotted is in the tests of pg_rewind, which
would fail in parallel as the same data folders are used for each
test. Using the same trick with $$ makes the tests more stable.

A third error is a failure in contrib/test_decoding, and this has been
addressed by Andres in 60f826c.

Attached is a patch for the first two ones, which makes the tests more
robust. I am myself annoyed by parallel tests failing when working on
patches for back-branches, so having at least a minimal fix would be
nice.
-- 
Michael


tap-stability-95.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] PG10 transition tables, wCTEs and multiple operations on the same table

[Thomas Munro]

On Wed, Jun 7, 2017 at 9:42 AM, Robert Haas  wrote:
> I think I'd like to walk back my earlier statements about reverting
> this patch just a little bit.  Although putting the tuplestore at the
> wrong level does seem like a fairly significant design mistake, Thomas
> more or less convinced me yesterday on a Skype call that relocating it
> to the ModifyTable node might not be that much work.  If it's a
> 150-line patch, it'd probably be less disruptive to install a fix than
> to revert the whole thing (and maybe put it back in again, in some
> future release).

I spent a couple of hours drafting a proof-of-concept to see if my
hunch was right.  It seems to work correctly so far and isn't huge
(but certainly needs more testing and work):

 6 files changed, 156 insertions(+), 109 deletions(-)

It applies on top of the other patch[1].  It extends
TransitionCaptureState to hold the the new and old tuplestores for
each ModifyTable node, instead of using global variables.  The result
is that tuplestores don't get mixed up, and there aren't any weird
assumptions about the order of execution as discussed earlier.
Example:

  with wcte as (insert into table1 values (42))
insert into table2 values ('hello world');
  NOTICE:  trigger = table2_trig, old table = , new table =
("hello world")
  NOTICE:  trigger = table1_trig, old table = , new table = (42)

Summary of how these patches relate:

1.  In the inheritance patch[1], TransitionCaptureState is introduced.
It holds flags that control whether we capture tuples.  There is one
of these per ModifyTable node.  In master we use the flags in
TriggerDesc to control transition tuple capture directly, but we
needed a way for ModifyTable's result rel's TriggerDesc to affect all
child tables that are touched.  My proposal is to do that by inventing
this new object to activate transition tuple capture while modifying
child tables too.  It is passed into the ExecAR*Trigger() functions of
all relations touched by the ModifyTable node.

2.  In the attached patch, that struct is extended to hold the actual
tuplestores.  They are used for two purposes: ExecAR*Trigger()
captures tuples into them (instead of using global variables to find
the tuplestores to capture tuples into), and ExecA[RS]*Trigger() keeps
hold of the TransitionCaptureState in the after trigger queue so that
when the queued event is eventually executed AfterTriggerExecute() can
expose the correct tuplestores to triggers.

There are a couple of things that definitely need work and I'd welcome
any comments:

1.  I added a pointer to TransitionCaptureState to AfterTriggerShared,
in order to record which tuplestores a queued after trigger event
should see.  I suspected that enqueuing pointers like that wouldn't be
popular, and when I ran the idea past Andres on IRC he just said
"yuck" :-)  Perhaps there needs to be a way to convert this into an
index into some array in EState, ... or something else.  The basic
requirement is that the AfterTriggerExecute() needs to know *which*
tuplestores should be visible to the trigger when it runs.  I believe
the object lifetime is sound (the TransitionCaptureState lasts until
ExecutorEnd(), and triggers are fired before that during
ExecutorFinish()).

2.  I didn't think about what execReplication.c needs.  Although that
code apparently doesn't know how to fire AS triggers, it does know how
to fire AR triggers (so that RI works?), and in theory those might
have transition tables, so I guess that needs to use
MakeTransitionCaptureState() -- but it seems to lack a place to keep
that around, and I ran out of time thinking about that today.

Thoughts?

[1] 
https://www.postgresql.org/message-id/CAEepm%3D1dGNzh98Gt21fn_Ed6k20sVB-NuAARE1EF693itK6%3DLg%40mail.gmail.com

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


transition-tuples-from-wctes-v1.patch
Description: Binary data

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

Re: [HACKERS] Notes on testing Postgres 10b1

[Andres Freund]

On 2017-06-07 14:29:04 +0900, Michael Paquier wrote:
> On Wed, Jun 7, 2017 at 2:01 PM, Josh Berkus  wrote:
> > Q1. Why does wal_level default to "replica" and not "logical"?
> 
> The difference of WAL generated is way higher between
> archive->hot_standby than hot_standby->logical. And unlike replica,
> logical decoding is not something that is widely spread in user's
> deployments to justify changing to such a default. At least that's
> what I recall on the matter.

Right.  I think what we really want there is some form of magic
switching to logical when a slot is present.  Thats easy enough on the
master, a good bit harder when we allow decoding on standbys, which
Craig's working on.


> > Q2: I thought we were going to finally change the pg_dump default to
> > "custom" format in this release?  No?
> 
> I don't recall any discussion on this matter, but my memory may fail me.

Nothing here either.


- 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] Notes on testing Postgres 10b1

[Michael Paquier]

On Wed, Jun 7, 2017 at 2:01 PM, Josh Berkus  wrote:
> Q1. Why does wal_level default to "replica" and not "logical"?

The difference of WAL generated is way higher between
archive->hot_standby than hot_standby->logical. And unlike replica,
logical decoding is not something that is widely spread in user's
deployments to justify changing to such a default. At least that's
what I recall on the matter.

> Q2: I thought we were going to finally change the pg_dump default to
> "custom" format in this release?  No?

I don't recall any discussion on this matter, but my memory may fail me.
-- 
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] BEFORE trigger can cause undetected partition constraint violation

[Amit Langote]

On 2017/06/07 11:57, Amit Langote wrote:
> How about we export ExecPartitionCheck() out of execMain.c and call it
> just before ExecFindPartition() using the root table's ResultRelInfo?

Turns out there wasn't a need to export ExecPartitionCheck after all.
Instead of calling it from execModifyTable.c and copy.c, it's better to
call it at the beginning of ExecFindPartition() itself.  That way, there
is no need to add the same code both in CopyFrom() and ExecInsert(), nor
is there need to make ExecPartitionCheck() public.  That's how the patch
attached with the previous email does it anyway.

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

[Neha Khatri]

On Mon, May 15, 2017 at 12:45 PM, Bruce Momjian  wrote:

> On Thu, May 11, 2017 at 11:50:03PM -0400, Tom Lane wrote:
> > Michael Paquier  writes:
> > > Bruce, the release notes do not mention yet that support for cleartext
> > > passwords is removed. This has been done recently by Heikki in
> > > eb61136d. This has as consequence that CREATE ROLE PASSWORD
> > > UNENCRYPTED returns an error, and that password_encryption loses its
> > > value 'off' compared to last releases.
> >
> > The release notes only say that they're current through 4-22.  The
> > usual process is to update them in batches every so often.  It'd be
> > great if Bruce has time to do another round before Monday, but the
> > current situation is not really broken.
>
> Done.  Applied patch attached.
>
>
The patch introduced one release note item twice in
https://www.postgresql.org/docs/10/static/release-10.html :


   -

   Rename WAL-related functions and views to use lsn instead of location (David
   Rowley)
   -

   RenameWAL-related functions and views to use lsn instead of location (David
   Rowley)

Perhaps just one entry is good.

Regards,
Neha

[HACKERS] Notes on testing Postgres 10b1

[Josh Berkus]

Folks,

I've put together some demos on PostgreSQL 10beta1.  Here's a few
feedback notes based on my experience with it.

Things I tested


* Logical replication pub/sub with replicating only two tables out of a
12-table FK heirarchy, including custom data types

* Partitioning a log-structured table, including a range type, exclusion
constraint, and foreign key.

* Various Parallel index queries on a 100m-row pgbench table

* Full text JSON search in a books database

* SCRAM authentication for local connections and replication

Positive changes beyond the obvious
---

* Yay defaults with replication on!

* Having defaults on the various _workers all devolve from max_workers
is also great.

* Constraint exclusion + partitioning Just Worked.

Questions
--

Q1. Why does wal_level default to "replica" and not "logical"?

Q2: I thought we were going to finally change the pg_dump default to
"custom" format in this release?  No?

Problems


P1. On the publishing node, logical replication relies on the *implied*
correspondence of the application_name and the replication_slot both
being named the same as the publication in order to associate a
particular publication with a particular replication connection.
However, there's absolutely nothing preventing me from also creating a
binary replication connection by the same name  It really seems like we
need a field in pg_stat_replication or pg_replication_slots which lists
the publication.


P2: If I create a subscription on a table with no primary key, I do not
recieve a warning.  There should be a warning, since in most cases such
a subscription will not work.  I suggest the text:

"logical replication target relation "public.fines" has no primary key.
Either create one, or set REPLICA IDENTITY index and set the published
relation to REPLICA IDENTITY FULL."


P3: apparently jsonb_to_tsvector with lang parameter isn't immutable?
This means that it can't be used for indexing:

libdata=# create index bookdata_fts on bookdata using gin ((
to_tsvector('english',bookdata)));
ERROR:  functions in index expression must be marked IMMUTABLE

... and indeed it's not:

select proname, prosrc, proargtypes, provolatile from pg_proc where
proname = 'to_tsvector';
   proname   | prosrc | proargtypes | provolatile
-++-+-
 to_tsvector | jsonb_to_tsvector  | 3802| s
 to_tsvector | to_tsvector_byid   | 3734 25 | i
 to_tsvector | to_tsvector| 25  | s
 to_tsvector | json_to_tsvector   | 114 | s
 to_tsvector | jsonb_to_tsvector_byid | 3734 3802   | s
 to_tsvector | json_to_tsvector_byid  | 3734 114| s

... can we fix that?


-- 
Josh Berkus
Containers & Databases Oh My!


-- 
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] Adding support for Default partition in partitioning

[Jeevan Ladhe]

IIUC, default partition constraints is simply NOT IN ( other sibling partitions>).
> If constraint on the default partition refutes the new partition's
> constraints that means we have overlapping partition, and perhaps
> error.
>

You are correct Amul, but this error will be thrown before we try to
check for the default partition data. So, in such cases I think we really
do not need to have logic to check if default partition refutes the new
partition contraints.

Regards,
Jeevan Ladhe

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

[amul sul]

On Wed, Jun 7, 2017 at 2:08 AM, Jeevan Ladhe
 wrote:
[...]
>>
>> The code in check_default_allows_bound() to check whether the default
>> partition
>> has any rows that would fit new partition looks quite similar to the code
>> in
>> ATExecAttachPartition() checking whether all rows in the table being
>> attached
>> as a partition fit the partition bounds. One thing that
>> check_default_allows_bound() misses is, if there's already a constraint on
>> the
>> default partition refutes the partition constraint on the new partition,
>> we can
>> skip the scan of the default partition since it can not have rows that
>> would
>> fit the new partition. ATExecAttachPartition() has code to deal with a
>> similar
>> case i.e. the table being attached has a constraint which implies the
>> partition
>> constraint. There may be more cases which check_default_allows_bound()
>> does not
>> handle but ATExecAttachPartition() handles. So, I am wondering whether
>> it's
>> better to somehow take out the common code into a function and use it. We
>> will
>> have to deal with a difference through. The first one would throw an error
>> when
>> finding a row that satisfies partition constraints whereas the second one
>> would
>> throw an error when it doesn't find such a row. But this difference can be
>> handled through a flag or by negating the constraint. This would also take
>> care
>> of Amit Langote's complaint about foreign partitions. There's also another
>> difference that the ATExecAttachPartition() queues the table for scan and
>> the
>> actual scan takes place in ATRewriteTable(), but there is not such queue
>> while
>> creating a table as a partition. But we should check if we can reuse the
>> code to
>> scan the heap for checking a constraint.
>>
>> In case of ATTACH PARTITION, probably we should schedule scan of default
>> partition in the alter table's work queue like what
>> ATExecAttachPartition() is
>> doing for the table being attached. That would fit in the way alter table
>> works.
>
>
> I am still working on this.
> But, about your comment here:
> "if there's already a constraint on the default partition refutes the
> partition
> constraint on the new partition, we can skip the scan":
> I am so far not able to imagine such a case, since default partition
> constraint
> can be imagined something like "minus infinity to positive infinity with
> some finite set elimination", and any new non-default partition being added
> would simply be a set of finite values(at-least in case of list, but I think
> range
> should not also differ here). Hence one cannot imply the other here.
> Possibly,
> I might be missing something that you had visioned when you raised the flag,
> please correct me if I am missing something.
>

IIUC, default partition constraints is simply NOT IN ().
If constraint on the default partition refutes the new partition's
constraints that means we have overlapping partition, and perhaps
error.


Regards,
Amul


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

Re: [HACKERS] Parallel Aggregation support for aggregate functions that use transitions not implemented for array_agg

[Andres Freund]

On 2017-06-07 00:03:15 -0400, Tom Lane wrote:
> Andres Freund  writes:
> > On 2017-06-06 23:32:53 -0400, Peter Eisentraut wrote:
> >> I'm not sure how you would parallelize these, since in most uses you
> >> want to have a deterministic output order.
> 
> > Unless you specify ORDER BY you don't really have that anyway, consider
> > hash-aggregation.  If you want deterministic order, you really need an
> > ORDER BY inside the aggregate.
> 
> Hash aggregation does not destroy the property that array_agg/string_agg
> will produce results whose components appear in the order that the
> subquery emitted them in.  It only causes the various aggregate results
> in a GROUP BY query to themselves appear in random order.

Whoa, I obviously should stop working tonight.  I think it's still a
hugely useful to parallelize such aggregates - it might be worthwhile to
have two versions of array_agg, one with a serial/combinefunc and one
without...

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] Parallel Aggregation support for aggregate functions that use transitions not implemented for array_agg

[Tom Lane]

Andres Freund  writes:
> On 2017-06-06 23:32:53 -0400, Peter Eisentraut wrote:
>> I'm not sure how you would parallelize these, since in most uses you
>> want to have a deterministic output order.

> Unless you specify ORDER BY you don't really have that anyway, consider
> hash-aggregation.  If you want deterministic order, you really need an
> ORDER BY inside the aggregate.

Hash aggregation does not destroy the property that array_agg/string_agg
will produce results whose components appear in the order that the
subquery emitted them in.  It only causes the various aggregate results
in a GROUP BY query to themselves appear in random order.

Now you could argue that the subquery might've gotten parallelized and
emitted its outputs in some random order, so doing the same thing one
level further up changes nothing.  But you can't defend this on this
basis that it was historically unpredictable, because it wasn't.

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] Fix tab-completion of ALTER SUBSCRIPTION SET PUBLICATION

[Masahiko Sawada]

On Wed, Jun 7, 2017 at 12:41 PM, Peter Eisentraut
 wrote:
> On 6/6/17 04:17, Masahiko Sawada wrote:
>> With this patch, ALTER SUBSCRIPTION  SET PUBLICATION  [TAB]
>> completes with "REFRESH" and "SKIP REFRESH".
>> Specifying either REFRESH or SKIP REFRESH is mandatory after ALTER
>> SUBSCRIPTION SET PUBLICATION, so i think it's good to add this.
>
> That syntax does not exist anymore.
>
> You could add support for the new WITH () syntax.
>

Sorry, I missed it.
Attached updated version patch adds WITH() syntax.

Regards,

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


fix_ALTER_SUB_tab_completion_v2.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] Parallel Aggregation support for aggregate functions that use transitions not implemented for array_agg

[Regina Obe]

> On 6/6/17 13:52, Regina Obe wrote:
>> It seems CREATE  AGGREGATE was expanded in 9.6 to support 
>> parallelization of aggregate functions using transitions, with the 
>> addition of serialfunc and deserialfunc to the aggregate definitions.
>> 
>> https://www.postgresql.org/docs/10/static/sql-createaggregate.html
>> 
>> I was looking at the PostgreSQL 10 source code for some example usages 
>> of this and was hoping that array_agg and string_agg would support the 
>> feature.

> I'm not sure how you would parallelize these, since in most uses you want to 
> have a deterministic output order.

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

Good point.  If that's the reason it wasn't done, that's good just wasn't sure.

But if you didn't have an ORDER BY in your aggregate usage, and you did have 
those transition functions, it shouldn't be any different from any other use 
case right?
I imagine you are right that most folks who use array_agg and string_agg 
usually combine it with array_agg(... ORDER BY ..)

My main reason for asking is that most of the PostGIS geometry and raster 
aggregate functions use transitions and were patterned after array agg.

In the case of PostGIS the sorting is done internally and really only to 
expedite take advantage of things like cascaded union algorithms.  That is 
always done though (so even if each worker does it on just it's batch that's 
still better than having only one worker).
So I think it's still very beneficial to break into separate jobs since in the 
end the gather, will have  say 2 biggish geometries or 2 biggish rasters to 
union if you have 2 workers which is still better than having a million 
smallish geometries/rasters to union

Split Union 

Worker 1:

Parallel agg (internal sort geoms by box)  - Union

Worker 2: 
Parallel Agg (internal sort geoms )  - Union


Gather  Union(union, union) internal sort.


Thanks,
Regina






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

Re: [HACKERS] Parallel Aggregation support for aggregate functions that use transitions not implemented for array_agg

[Andres Freund]

On 2017-06-06 23:32:53 -0400, Peter Eisentraut wrote:
> On 6/6/17 13:52, Regina Obe wrote:
> > It seems CREATE  AGGREGATE was expanded in 9.6 to support parallelization of
> > aggregate functions using transitions, with the addition of serialfunc and
> > deserialfunc to the aggregate definitions.
> > 
> > https://www.postgresql.org/docs/10/static/sql-createaggregate.html
> > 
> > I was looking at the PostgreSQL 10 source code for some example usages of
> > this and was hoping that array_agg and string_agg would support the feature.
> 
> I'm not sure how you would parallelize these, since in most uses you
> want to have a deterministic output order.

Unless you specify ORDER BY you don't really have that anyway, consider
hash-aggregation.  If you want deterministic order, you really need an
ORDER BY inside the aggregate.

- 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] Is ECPG's SET CONNECTION really not thread-aware?

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Michael Meskes
> I'm pretty sure it is indeed thread-aware, although I didn't provide the
> code for this feature myself.
> 
> > So the doc seems to need fix.  The patch is attached.
> 
> Thanks, applied.

Thank you, I'm relieved.

Could you also apply it to past versions if you don't mind?  The oldest 
supported version 9.2 is already thread-aware.

Regards
Takayuki Tsunakawa


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

Re: [HACKERS] Fix tab-completion of ALTER SUBSCRIPTION SET PUBLICATION

[Peter Eisentraut]

On 6/6/17 04:17, Masahiko Sawada wrote:
> With this patch, ALTER SUBSCRIPTION  SET PUBLICATION  [TAB]
> completes with "REFRESH" and "SKIP REFRESH".
> Specifying either REFRESH or SKIP REFRESH is mandatory after ALTER
> SUBSCRIPTION SET PUBLICATION, so i think it's good to add this.

That syntax does not exist anymore.

You could add support for the new WITH () syntax.

-- 
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] Parallel Aggregation support for aggregate functions that use transitions not implemented for array_agg

[Peter Eisentraut]

On 6/6/17 13:52, Regina Obe wrote:
> It seems CREATE  AGGREGATE was expanded in 9.6 to support parallelization of
> aggregate functions using transitions, with the addition of serialfunc and
> deserialfunc to the aggregate definitions.
> 
> https://www.postgresql.org/docs/10/static/sql-createaggregate.html
> 
> I was looking at the PostgreSQL 10 source code for some example usages of
> this and was hoping that array_agg and string_agg would support the feature.

I'm not sure how you would parallelize these, since in most uses you
want to have a deterministic output order.

-- 
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] BEFORE trigger can cause undetected partition constraint violation

[Amit Langote]

On 2017/06/07 1:19, Robert Haas wrote:
> On Mon, Jun 5, 2017 at 1:02 AM, Amit Langote
>  wrote:
>> On 2017/06/03 1:56, Robert Haas wrote:
>>> On Fri, Jun 2, 2017 at 12:51 AM, Amit Langote
>>>  wrote:
 Attached patch makes InitResultRelInfo() *always* initialize the
 partition's constraint, that is, regardless of whether insert/copy is
 through the parent or directly on the partition.  I'm wondering if
 ExecInsert() and CopyFrom() should check if it actually needs to execute
 the constraint?  I mean it's needed if there exists BR insert triggers
 which may change the row, but not otherwise.  The patch currently does not
 implement that check.
>>>
>>> I think it should.  I mean, otherwise we're leaving a
>>> probably-material amount of performance on the table.
>>
>> I agree.  Updated the patch to implement the check.
> 
> OK, so this isn't quite right.  Consider the following test case:
> 
> rhaas=# create table x (a int, b int, c int) partition by list (a);
> CREATE TABLE
> rhaas=# create table y partition of x for values in (1) partition by list (b);
> CREATE TABLE
> rhaas=# create table z partition of y for values in (1);
> CREATE TABLE
> rhaas=# insert into y values (2,1,1);
> INSERT 0 1

Gah!

> The absence of the before-trigger is treated as evidence that z need
> not revalidate the partition constraint, but actually it (or
> something) still needs to enforce the part of it that originates from
> y's ancestors.  In short, this patch would reintroduce the bug that
> was fixed in 39162b2030fb0a35a6bb28dc636b5a71b8df8d1c, and would do so
> by removing the exact same code that was added (by you!) in that
> patch.

I think we will have to go for the "or something" here, which is the way I
should have originally proposed it (I mean the patch that went in as
39162b2030fb0a35a6bb28dc636b5a71b8df8d1c). :-(

How about we export ExecPartitionCheck() out of execMain.c and call it
just before ExecFindPartition() using the root table's ResultRelInfo?  If
the root table is a partition, its ResultRelInfo.ri_PartitionCheck must
have been initialized, which ExecPartitionCheck will check.  Since there
cannot be any BR triggers on the root table (because partitioned), we can
safely do that.  After tuple-routing, if the target leaf partition has BR
triggers, any violating changes they might make will be checked by
ExecConstraints() using the leaf partition's ResultRelInfo, whose
ri_PartitionCheck consists of its own partition constraints plus that of
any of its ancestors that are partitions.  If the leaf partition does not
have any BR triggers we need not check any partition constraints just as
the patch does.  (Remember that we already checked the root table's
partition constraint before we began routing the tuple, as the proposed
updated patch will do, and we don't need to worry about any of
intermediate ancestors, because if the tuple does not satisfy the
constraint of any one of those, tuple-routing will fail anyway.)

I proposed a similar thing in the hash partitioning thread [1], where
Dilip was complaining about name of the table that appears in the
"violates partition constraint" error message.  Even if the tuple failed
an ancestor table's partition constraint, since the ResultRelInfo passed
to ExecConstraints() is the leaf partition's, the name shown is also leaf
partition's.  ISTM, showing the leaf partition's name is fine as long as
it's a NOT NULL or a CHECK constraint failing, because they are explicitly
attached to the leaf table, but maybe not fine when an implicitly
inherited internal partition constraint fails.

Attached updated patch that implements the change described above, in
addition to fixing the originally reported bug.  With the patch:

create table x (a int, b int, c int) partition by list (a);
create table y partition of x for values in (1) partition by list (b);
create table z partition of y for values in (1);

insert into y values (2,1,1);
ERROR:  new row for relation "y" violates partition constraint
DETAIL:  Failing row contains (2, 1, 1).

-- whereas on HEAD, it shows the leaf partition's name
insert into y values (2,1,1);
ERROR:  new row for relation "z" violates partition constraint
DETAIL:  Failing row contains (2, 1, 1).

Thoughts?

Thanks,
Amit

[1]
https://www.postgresql.org/message-id/0ded7a50-0b35-1805-232b-f8edcf4cbadb%40lab.ntt.co.jp
From a84e6b95e69682c387b53a4da962d7270971e2c4 Mon Sep 17 00:00:00 2001
From: amit 
Date: Fri, 2 Jun 2017 12:11:17 +0900
Subject: [PATCH] Check partition constraint more carefully

Partition constraint expressions of a leaf partition are currently not
initialized when inserting through the parent because tuple-routing is
said to implicitly preserve the constraint.  But its BR triggers may
change a row into one that violates the partition constraint and they
are executed after tuple-routing, so any such change must be detected
by checking 

Re: [HACKERS] Why does logical replication launcher set application_name?

[Peter Eisentraut]

On 6/6/17 15:58, Robert Haas wrote:
> The problem with the status quo (after Peter's commit) is that there's
> now nothing at all to identify the logical replication launcher, apart
> from the wait_event field, which is likely to be LogicalLauncherMain
> fairly often if you've got the launcher.  I don't personally see why
> we can't simply adopt Tom's original proposal of setting the query
> string to something like "" which, while
> maybe not as elegant as providing some way to override the
> backend_type field, would be almost no work and substantially better
> for v10 than what we've got now.

The decision was made to add background workers to pg_stat_activity, but
no facility was provided to tell the background workers apart.  Is it
now the job of every background worker to invent a hack to populate some
other pg_stat_activity field with some ad hoc information?  What about
other logical replication worker types, parallel workers, external
background workers, auto-prewarm?

I think the bgw_type addition that I proposed nearby would solve this
quite well, but it needs a bit of work.  And arguably, it's too late for
PG10, but one could also argue that this is a design fault in the
pg_stat_activity extension that is valid to fix in PG10.

-- 
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] inconsistent application_name use in logical workers

[Peter Eisentraut]

On 6/6/17 13:24, Petr Jelinek wrote:
> On 06/06/17 15:07, Peter Eisentraut wrote:
>> On 6/6/17 06:51, Petr Jelinek wrote:
>>> On 06/06/17 04:19, Peter Eisentraut wrote:
 The logical replication code is supposed to use the subscription name as
 the fallback_application_name, but in some cases it uses the slot name,
 which could be different.  See attached patch to correct this.
>>>
>>> Hmm, well the differentiation has a reason though. The application_name
>>> is used for sync rep and having synchronization connection using same
>>> application_name might have adverse effects there because
>>> synchronization connection can be in-front of main apply one, so sync
>>> rep will think something is consumed while it's not.
>>
>> True, we should use a different name for tablesync.c.  But the one in
>> worker.c appears to be a mistake then?
> 
> Yes.

Committed and added a comment.

-- 
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] [BUGS] BUG #14682: row level security not work with partitioned table

[Michael Paquier]

On Wed, Jun 7, 2017 at 9:52 AM, Joe Conway  wrote:
> On 06/06/2017 02:44 PM, Mike Palmiotto wrote:
>> On Tue, Jun 6, 2017 at 4:07 PM, Joe Conway  wrote:
>>> On 06/06/2017 11:57 AM, Mike Palmiotto wrote:
 On Mon, Jun 5, 2017 at 10:36 AM, Robert Haas  wrote:
> On Mon, Jun 5, 2017 at 10:20 AM, Joe Conway  wrote:
>> Unless Robert objects, I'll work with Mike to get a fix posted and
>> committed in the next day or two.
>
> That would be great.  Thanks.

 I have the updated patch with rowsecurity regression tests and rebased
 on master. I've run these and verified locally by feeding
 rowsecurity.sql to psql, but have yet to get the full regression suite
 passing -- it's failing on the constraints regtest and then gets stuck
 in recovery. Undoubtedly something to do with my
 configuration/environment over here. I'm working through those issues
 right now. In the meantime, if you want to see the regression tests as
 they stand, please see the attached patch.
>>>
>>> The constraints test passes here, so presumably something you borked
>>> locally. I only see a rowsecurity failure, which is not surprising since
>>> your patch does not include the changes to expected output ;-)
>>> Please resend with src/test/regress/expected/rowsecurity.out included.
>>
>> It was indeed an issue on my end. Attached are the rowsecurity
>> regression tests and the expected out. Unsurprisingly, all tests pass,
>> because I said so. :)
>>
>> Let me know if you want me to make any revisions.
>
>
> Thanks Mike. I'll take a close look to verify output correctnes, but I
> am concerned that the new tests are unnecessarily complex. Any other
> opinions on that?

Some tests would be good to have. Now, if I read those regression
tests correctly, this is using 10 relations where two would be enough,
one as the parent relation and one as a partition. Then policies apply
on the parent relation. The same kind of policy is defined 4 times,
and there is bloat with GRANT and ALTER TABLE commands.

+SELECT * FROM part_document;
+ did | cid | dlevel |  dauthor  | dtitle
+-+-++---+-
+   1 |  11 |  1 | regress_rls_bob   | my first novel
Adding an "ORDER BY did" as well here would make the test output more
predictable.
-- 
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] Get stuck when dropping a subscription during synchronizing table

[Andres Freund]

On 2017-06-06 19:36:13 +0200, Petr Jelinek wrote:
> So the fact that we moved workers to standard interrupt handling broke
> launcher in subtle ways because it still uses it's own SIGTERM handling
> but some function it calls are using CHECK_FOR_INTERRUPTS (they are used
> by worker as well). I think we need to move launcher to standard
> interrupt handling as well.

Sounds like a good plan.


> As a side note, we are starting to have several IsSomeTypeOfProcess
> functions for these kind of things. I wonder if bgworker infrastructure
> should somehow provide this type of stuff (the proposed bgw_type might
> help there) as well as maybe being able to register interrupt handler
> for the worker (it's really hard to do it via custom SIGTERM handler as
> visible on worker, launcher and walsender issues we are fixing).
> Obviously this is PG11 related thought.

Maybe it's also a sign that the bgworker infrastructure isn't really the
best thing to use for internal processes like parallel workers and lrep
processes - it's really built so core code *doesn't* know anything
specific about them, which isn't really what we want in some of these
cases.  That's not to say that bgworkers and parallelism/lrep shouldn't
share infrastructure, don't get me wrong.


>  
> - LogicalRepCtx->launcher_pid = 0;
> -
> - /* ... and if it returns, we're done */
> - ereport(DEBUG1,
> - (errmsg("logical replication launcher shutting down")));
> + /* Not reachable */
> +}

Maybe put a pg_unreachable() there?


> @@ -2848,6 +2849,14 @@ ProcessInterrupts(void)
>   ereport(FATAL,
>   (errcode(ERRCODE_ADMIN_SHUTDOWN),
>errmsg("terminating logical 
> replication worker due to administrator command")));
> + else if (IsLogicalLauncher())
> + {
> + ereport(DEBUG1,
> + (errmsg("logical replication launcher 
> shutting down")));
> +
> + /* The logical replication launcher can be stopped at 
> any time. */
> + proc_exit(0);
> + }

We could use PgBackendStatus->st_backendType for these, merging these
different paths.

I can take care of this one, if you/Peter want.

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] PG10 transition tables, wCTEs and multiple operations on the same table

[Adam Brusselback]

>
> I'll give it a few days for objections before reverting.
>>
>
> I can only say that the lack of this feature comes up on a weekly basis on
> IRC, and a lot of people would be disappointed to see it reverted.
>

Not that my opinion matters, but I was very much looking forward to this
feature in Postgres 10, but I understand if it just won't be stable enough
to stay in.

I have my fingers crossed these issues can be resolved.

[HACKERS] Re: [BUGS] BUG #14682: row level security not work with partitioned table

[Joe Conway]

On 06/06/2017 02:44 PM, Mike Palmiotto wrote:
> On Tue, Jun 6, 2017 at 4:07 PM, Joe Conway  wrote:
>> On 06/06/2017 11:57 AM, Mike Palmiotto wrote:
>>> On Mon, Jun 5, 2017 at 10:36 AM, Robert Haas  wrote:
 On Mon, Jun 5, 2017 at 10:20 AM, Joe Conway  wrote:
> Unless Robert objects, I'll work with Mike to get a fix posted and
> committed in the next day or two.

 That would be great.  Thanks.
>>>
>>> I have the updated patch with rowsecurity regression tests and rebased
>>> on master. I've run these and verified locally by feeding
>>> rowsecurity.sql to psql, but have yet to get the full regression suite
>>> passing -- it's failing on the constraints regtest and then gets stuck
>>> in recovery. Undoubtedly something to do with my
>>> configuration/environment over here. I'm working through those issues
>>> right now. In the meantime, if you want to see the regression tests as
>>> they stand, please see the attached patch.
>>
>> The constraints test passes here, so presumably something you borked
>> locally. I only see a rowsecurity failure, which is not surprising since
>> your patch does not include the changes to expected output ;-)
>> Please resend with src/test/regress/expected/rowsecurity.out included.
> 
> It was indeed an issue on my end. Attached are the rowsecurity
> regression tests and the expected out. Unsurprisingly, all tests pass,
> because I said so. :)
> 
> Let me know if you want me to make any revisions.


Thanks Mike. I'll take a close look to verify output correctnes, but I
am concerned that the new tests are unnecessarily complex. Any other
opinions on that?

Joe

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



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] sketchy partcollation handling

[Amit Langote]

On 2017/06/07 0:19, Robert Haas wrote:
> On Sun, Jun 4, 2017 at 10:18 PM, Amit Langote
>  wrote:
>> I think we can call it a bug of StorePartitionKey().  I looked at the
>> similar code in index_create() (which actually I had originally looked at
>> for reference when writing the partitioning code in question) and looks
>> like it doesn't store the dependency for collation 0 and for the default
>> collation of the database.  I think the partitioning code should do the
>> same.  Attached find a patch for the same (which also updates the
>> documentation as mentioned above); with the patch:
> 
> Thanks.  Committed.

Thank you.

Regards,
Amit



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

Re: [HACKERS] sketchy partcollation handling

[Amit Langote]

On 2017/06/07 1:08, Tom Lane wrote:
> Robert Haas  writes:
>> On Sun, Jun 4, 2017 at 10:18 PM, Amit Langote
>>  wrote:
>>> BTW, the places which check whether the collation to store a dependency
>>> for is the database default collation don't need to do that.  I mean the
>>> following code block in all of these places:
>>>
>>> /* The default collation is pinned, so don't bother recording it */
>>> if (OidIsValid(attr->attcollation) &&
>>> attr->attcollation != DEFAULT_COLLATION_OID)
> 
>> We could go change them all, but I guess I don't particularly see the point.
> 
> That's an intentional measure to save the catalog activity involved in
> finding out that the default collation is pinned.  It's not *necessary*,
> sure, but it's a useful and easy optimization.

I see.  Thanks for explaining.

Regards,
Amit



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

[HACKERS] Coverage improvements of src/bin/pg_basebackup and pg_receivewal --endpos

[Michael Paquier]

Hi all,

The coverage of pg_basebackup is reaching 50%, which is not bad:
https://coverage.postgresql.org/src/bin/pg_basebackup/index.html
In this set pg_receivewal.c is the bad student with less than 20%.

There are a couple of causes behind that, with no tests like:
- option interactions like --create-slot and --drop-slot.
- replication slot drop.
- end of streaming.
- check handling of history files.
- looking at compressed and uncompressed WAL data.
- restart of pg_receivewal on an existing folder.
- pg_basebackup's tests don't use the progress report.

While it is possible to tackle some of those issues independently,
like pg_basebackup stuff, it seems to me that it would be helpful to
make the tests more deterministic by having an --endpos option for
pg_receivewal, similarly to what exists already in pg_recvlogical.

Thoughts?
-- 
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] PG10 transition tables, wCTEs and multiple operations on the same table

[Peter Geoghegan]

On Tue, Jun 6, 2017 at 5:01 PM, Peter Geoghegan  wrote:
> Also, ISTM that the code within ENRMetadataGetTupDesc() probably
> requires more explanation, resource management wise.

Also, it's not clear why it should be okay that the new type of
ephemeral RTEs introduced don't have permissions checks. There are
currently cases where the user cannot see data that they inserted
themselves (e.g., through RETURNING), on the theory that a before row
trigger might have modified the final contents of the tuple in a way
that the original inserter isn't supposed to know details about.

As the INSERT docs say, "Use of the RETURNING clause requires SELECT
privilege on all columns mentioned in RETURNING". Similarly, the
excluded.* pseudo-relation requires select privilege (on the
corresponding target relation columns) in order to be usable by ON
CONFLICT DO UPDATE.

-- 
Peter Geoghegan


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

Re: [HACKERS] GSoC 2017 : Proposal for predicate locking in gist index

[Kevin Grittner]

On Thu, Jun 1, 2017 at 12:49 AM, Andrew Borodin  wrote:

> First, I just do not know, can VACUUM erase page with predicate lock?

For handling in btree, see:

https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=src/backend/access/nbtree/nbtpage.c;h=f815fd40b20e98a88cb2fb5c71005ea125a459c9;hb=refs/heads/master#l1406

Note also this discussion:

https://www.postgresql.org/message-id/4d669122.80...@enterprisedb.com

It doesn't look like we ever got to the optimization Heikki
suggested in that post, so on rare occasions we could see a false
positive from this.  Perhaps we should give this another look while
we're in the AMs.

> Right now, GiST never deletes pages, as far as I know, so this
> question is only matter of future compatibility.

ok

> Second, when we are doing GiST inserts, we can encounter unfinished
> split. That's not a frequent situation, but still. Should we skip
> finishing split or should we add it to collision check too?

When a page is split, I think you need to call
PredicateLockPageSplit() before it gets to the point that an insert
to get to it.

--
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] PG10 transition tables, wCTEs and multiple operations on the same table

[Peter Geoghegan]

On Tue, Jun 6, 2017 at 3:47 PM, Peter Geoghegan  wrote:
> I suppose you'll need two tuplestores for the ON CONFLICT DO UPDATE
> case -- one for updated tuples, and the other for inserted tuples.

Also, ISTM that the code within ENRMetadataGetTupDesc() probably
requires more explanation, resource management wise.


-- 
Peter Geoghegan


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

[HACKERS] Re: GSoC 2017 weekly progress reports ("Explicitly support predicate locks in index access methods besides b-tree")

[Kevin Grittner]

On Mon, Jun 5, 2017 at 12:40 PM, Shubham Barai  wrote:

> GSoC (week 1)

> 4. went through source code of gist index to understand its implementation
>
> 5. found appropriate places in gist index AM to insert calls to existing
> functions (PredicateLockPage(),  CheckForSerializableConflictIn() ...etc)

When you have a patch for any AM, please post it to the list.  Each
AM will be reviewed and considered for commit separately.

--
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] Error while creating subscription when server is running in single user mode

[Andres Freund]

On 2017-06-06 12:53:21 -0700, Andres Freund wrote:
> On 2017-06-06 15:48:42 -0400, Robert Haas wrote:
> > On Fri, Jun 2, 2017 at 3:24 PM, Andres Freund  wrote:
> > > Latches work in single user mode, it's just that the new code for some
> > > reason uses uninitialized memory as the latch.  As I pointed out above,
> > > the new code really should just use MyLatch instead of
> > > MyProc->procLatch.
> 
> FWIW, I'd misremembered some code here, and we actually reach the
> function initializing the shared latch, even in single user mode.
> 
> 
> > We seem to have accumulated quite a few instance of that.
> > 
> > [rhaas pgsql]$ git grep MyLatch | wc -l
> >  116
> > [rhaas pgsql]$ git grep 'MyProc->procLatch' | wc -l
> >   33
> > 
> > Most of the offenders are in src/backend/replication, but there are
> > some that are related to parallelism as well (bgworker.c, pqmq.c,
> > parallel.c, condition_variable.c).  Maybe we (you?) should just go and
> > change them all.  I don't think using MyLatch instead of
> > MyProc->procLatch has become automatic for everyone yet.
> 
> Nevertheless this should be changed.  Will do.

Here's the patch for that, also addressing some issues I found while
updating those callsites (separate thread started, too).

- Andres
>From 9206ced1dc05d3a9cc99faafa22d5d8b16d998d1 Mon Sep 17 00:00:00 2001
From: Andres Freund 
Date: Tue, 6 Jun 2017 16:13:00 -0700
Subject: [PATCH] Clean up latch related code.

The larger part of this patch replaces usages of MyProc->procLatch
with MyLatch.  The latter works even early during backend startup,
where MyProc->procLatch doesn't yet.  While the affected code
shouldn't run in cases where it's not initialized, it might get copied
into places where it might.  Using MyLatch is simpler and a bit faster
to boot, so there's little point to stick with the previous coding.

While doing so I noticed some weaknesses around newly introduced uses
of latches that could lead to missed events, and an omitted
CHECK_FOR_INTERRUPTS() call in worker_spi.

As all the actual bugs are in v10 code, there doesn't seem to be
sufficient reason to backpatch this.

Author: Andres Freund
Discussion:
https://postgr.es/m/20170606195321.sjmenrfgl2nu6...@alap3.anarazel.de
https://postgr.es/m/20170606210405.sim3yl6vpudhm...@alap3.anarazel.de
Backpatch: -
---
 src/backend/access/transam/parallel.c  |  4 +--
 src/backend/libpq/pqmq.c   |  4 +--
 src/backend/postmaster/bgworker.c  |  4 +--
 .../libpqwalreceiver/libpqwalreceiver.c| 13 
 src/backend/replication/logical/launcher.c | 35 +++---
 src/backend/replication/logical/tablesync.c| 12 
 src/backend/replication/logical/worker.c   | 10 +--
 src/backend/storage/lmgr/condition_variable.c  |  6 ++--
 src/test/modules/worker_spi/worker_spi.c   |  2 ++
 9 files changed, 56 insertions(+), 34 deletions(-)

diff --git a/src/backend/access/transam/parallel.c b/src/backend/access/transam/parallel.c
index cb22174270..16a0bee610 100644
--- a/src/backend/access/transam/parallel.c
+++ b/src/backend/access/transam/parallel.c
@@ -527,9 +527,9 @@ WaitForParallelWorkersToFinish(ParallelContext *pcxt)
 		if (!anyone_alive)
 			break;
 
-		WaitLatch(>procLatch, WL_LATCH_SET, -1,
+		WaitLatch(MyLatch, WL_LATCH_SET, -1,
   WAIT_EVENT_PARALLEL_FINISH);
-		ResetLatch(>procLatch);
+		ResetLatch(MyLatch);
 	}
 
 	if (pcxt->toc != NULL)
diff --git a/src/backend/libpq/pqmq.c b/src/backend/libpq/pqmq.c
index 96939327c3..8fbc03819d 100644
--- a/src/backend/libpq/pqmq.c
+++ b/src/backend/libpq/pqmq.c
@@ -172,9 +172,9 @@ mq_putmessage(char msgtype, const char *s, size_t len)
 		if (result != SHM_MQ_WOULD_BLOCK)
 			break;
 
-		WaitLatch(>procLatch, WL_LATCH_SET, 0,
+		WaitLatch(MyLatch, WL_LATCH_SET, 0,
   WAIT_EVENT_MQ_PUT_MESSAGE);
-		ResetLatch(>procLatch);
+		ResetLatch(MyLatch);
 		CHECK_FOR_INTERRUPTS();
 	}
 
diff --git a/src/backend/postmaster/bgworker.c b/src/backend/postmaster/bgworker.c
index c3454276bf..712d700481 100644
--- a/src/backend/postmaster/bgworker.c
+++ b/src/backend/postmaster/bgworker.c
@@ -1144,7 +1144,7 @@ WaitForBackgroundWorkerShutdown(BackgroundWorkerHandle *handle)
 		if (status == BGWH_STOPPED)
 			break;
 
-		rc = WaitLatch(>procLatch,
+		rc = WaitLatch(MyLatch,
 	   WL_LATCH_SET | WL_POSTMASTER_DEATH, 0,
 	   WAIT_EVENT_BGWORKER_SHUTDOWN);
 
@@ -1154,7 +1154,7 @@ WaitForBackgroundWorkerShutdown(BackgroundWorkerHandle *handle)
 			break;
 		}
 
-		ResetLatch(>procLatch);
+		ResetLatch(MyLatch);
 	}
 
 	return status;
diff --git a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
index 726d1b5bd8..89c34b8225 100644
--- a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
+++ b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
@@ -176,7 +176,7 @@ 

Re: [HACKERS] HACKERS[PROPOSAL] split ProcArrayLock into multiple parts

[Jim Van Fleet]

Hi Sokolov --

I tried your patch. I only had time for doing a few points on power8. 
pgbench rw  on two sockets is awesome! Keeps getting more throughput as 
threads are added -- in contrast to base and my prototype. I did not run 
single socket pgbench.

Hammerdb, 1 socket was in the same ballpark as the base, but slightly 
lower. 2 socket was also in the same ballpark as the base, again slightly 
lower.  I did not do a series of points (just one at the previous "sweet 
spot"), so the "final" results may be better, The ProcArrayLock multiple 
parts was lower except in two socket case. The performance data I 
collected for your patch on hammerdb showed the same sort of issues  as 
the base.

I don't see much point in combining the two because of the ProcArrayLock 
down side -- that is, single socket. poor performance. Unless we could 
come up with some heuristic to use one part on light loads and two on 
heavy (and still stay correct), then I don't see it ... With the 
combination, what I think we would see is awesome pgbench rw, awesome 
hammerdb 2 socket performance, and  degraded single socket hammerdb.

Jim



From:   Sokolov Yura 
To: Jim Van Fleet 
Cc: pgsql-hackers@postgresql.org
Date:   06/05/2017 03:28 PM
Subject:Re: [HACKERS] HACKERS[PROPOSAL] split ProcArrayLock into 
multiple parts
Sent by:pgsql-hackers-ow...@postgresql.org



Excuse me, Jim.

I was tired and misunderstand proposal: I thought of ProcArray sharding, 
but proposal is about ProcArrayLock sharding.

BTW, I just posted improvement to LWLock:

https://www.postgresql.org/message-id/2968c0be065baab8865c4c95de3f435c%40postgrespro.ru

Would you mind to test against that and together with that?

5 июня 2017 г. 11:11 PM пользователь Sokolov Yura 
 написал:
Hi, Jim.

How do you ensure of transaction order?

Example:
- you lock shard A and gather info. You find transaction T1 in-progress.
- Then you unlock shard A.
- T1 completes. T2, that depends on T1, also completes. But T2 was on 
shard B.
- you lock shard B, and gather info from.
- You didn't saw T2 as in progress, so you will lookup into clog then and 
will find it as commited.

Now you see T2 as commited, but T1 as in-progress - clear violation of 
transaction order.

Probably you've already solved this issue. If so it would be great to 
learn the solution.


5 июня 2017 г. 10:30 PM пользователь Jim Van Fleet  
написал:
Hi,

I have been experimenting with splitting  the ProcArrayLock into parts. 
 That is, to Acquire the ProcArrayLock in shared mode, it is only 
necessary to acquire one of the parts in shared mode; to acquire the lock 
in exclusive mode, all of the parts must be acquired in exclusive mode. 
For those interested, I have attached a design description of the change.

This approach has been quite successful on large systems with the hammerdb 
benchmark.With a prototype based on 10 master source and running on power8 
(model 8335-GCA with 2sockets, 20 core)
 hammerdb  improved by 16%; On intel (Intel(R) Xeon(R) CPU E5-2699 v4 @ 
2.20GHz, 2 socket, 44 core) with 9.6 base and prototype hammerdb improved 
by 4%. (attached is a set of spreadsheets for power8.

The down side is that on smaller configurations (single socket) where 
there is less "lock thrashing" in the storage subsystem and there are 
multiple Lwlocks to take for an exclusive acquire, there is a decided 
downturn in performance. On  hammerdb, the prototype was 6% worse than the 
base on a single socket power configuration.

If there is interest in this approach, I will submit a patch.

Jim Van Fleet

Re: [HACKERS] PG10 transition tables, wCTEs and multiple operations on the same table

[Peter Geoghegan]

On Mon, Jun 5, 2017 at 6:40 PM, Thomas Munro
 wrote:
> After sleeping on it, I don't think we need to make that decision here
> though.  I think it's better to just move the tuplestores into
> ModifyTableState so that each embedded DML statement has its own, and
> have ModifyTable pass them to the trigger code explicitly.

I suppose you'll need two tuplestores for the ON CONFLICT DO UPDATE
case -- one for updated tuples, and the other for inserted tuples.


-- 
Peter Geoghegan


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

Re: [HACKERS] libpqrcv_PQexec() seems to violate latch protocol

[Petr Jelinek]

On 06/06/17 23:42, Andres Freund wrote:
> On 2017-06-06 23:24:50 +0200, Petr Jelinek wrote:
>> On 06/06/17 23:17, Andres Freund wrote:
>>> Right.  I found a couple more instance of similarly iffy, although not
>>> quite as broken, patterns in launcher.c.  It's easy to get this wrong,
>>> but it's a lot easy if you do it differently everywhere you use a
>>> latch.  It's not good if code in the same file, by the same author(s),
>>> has different ways of using latches.
>>
>> Huh? I see same pattern everywhere in launcher.c, what am I missing?
> 
> WaitForReplicationWorkerAttach:
> while (...)
> CHECK_FOR_INTERRUPTS();
> /* other stuff including returns */
> WaitLatch()
> WL_POSTMASTER_DEATH
> ResetLatch()
> 
> logicalrep_worker_stop loop 1:
> while (...)
> /* other stuff */
> CHECK_FOR_INTERRUPTS()
> WaitLatch()
> POSTMASTER_DEATH
> ResetLatch()
> /* other stuff including returns */
> logicalrep_worker_stop loop 1:
> while (...)
> /* other stuff including returns */
> CHECK_FOR_INTERRUPTS();
> WaitLatch()
> WL_POSTMASTER_DEATH
> ResetLatch()
> 
> ApplyLauncherMain:
> while (!got_SIGTERM)
> /* lots other stuff */
> WaitLatch()
> WL_POSTMASTER_DEATH
> /* some other stuff */
> ResetLatch()
> (note no CFI)
> 
> they're not hugely different, but subtely there are differences.
> Sometimes you're guaranteed to check for interrupts after resetting the
> latch, in other cases not. Sometimes expensive-ish things happen before
> a CFI...
> 

Ah that's because signals in launcher are broken, see
https://www.postgresql.org/message-id/fe072153-babd-3b5d-8052-73527a6eb...@2ndquadrant.com
which also includes patch to fix it.

We originally had custom signal handling everywhere, then I realized it
was mistake for workers because of locking interaction but missed same
issue with launcher (the CFI in current launcher doesn't work).

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


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

Re: [HACKERS] [BUGS] BUG #14682: row level security not work with partitioned table

[Mike Palmiotto]

On Tue, Jun 6, 2017 at 4:07 PM, Joe Conway  wrote:
> On 06/06/2017 11:57 AM, Mike Palmiotto wrote:
>> On Mon, Jun 5, 2017 at 10:36 AM, Robert Haas  wrote:
>>> On Mon, Jun 5, 2017 at 10:20 AM, Joe Conway  wrote:
 Unless Robert objects, I'll work with Mike to get a fix posted and
 committed in the next day or two.
>>>
>>> That would be great.  Thanks.
>>
>> I have the updated patch with rowsecurity regression tests and rebased
>> on master. I've run these and verified locally by feeding
>> rowsecurity.sql to psql, but have yet to get the full regression suite
>> passing -- it's failing on the constraints regtest and then gets stuck
>> in recovery. Undoubtedly something to do with my
>> configuration/environment over here. I'm working through those issues
>> right now. In the meantime, if you want to see the regression tests as
>> they stand, please see the attached patch.
>
> The constraints test passes here, so presumably something you borked
> locally. I only see a rowsecurity failure, which is not surprising since
> your patch does not include the changes to expected output ;-)
> Please resend with src/test/regress/expected/rowsecurity.out included.

It was indeed an issue on my end. Attached are the rowsecurity
regression tests and the expected out. Unsurprisingly, all tests pass,
because I said so. :)

Let me know if you want me to make any revisions.

Thanks,

--
Mike Palmiotto
Software Engineer
Crunchy Data Solutions
https://crunchydata.com
From 08432d93ed753a1e5cd4585ccf00e900abbd685f Mon Sep 17 00:00:00 2001
From: Mike Palmiotto 
Date: Wed, 24 May 2017 16:54:49 +
Subject: [PATCH] Add RLS support to partitioned tables

This is needed to get RLS policies to apply to the parent partitioned table.
Without this change partitioned tables are skipped.
---
 src/backend/rewrite/rewriteHandler.c  |   3 +-
 src/test/regress/expected/rowsecurity.out | 812 ++
 src/test/regress/sql/rowsecurity.sql  | 222 
 3 files changed, 1036 insertions(+), 1 deletion(-)

diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c
index 35ff8bb..6cd73c1 100644
--- a/src/backend/rewrite/rewriteHandler.c
+++ b/src/backend/rewrite/rewriteHandler.c
@@ -1835,7 +1835,8 @@ fireRIRrules(Query *parsetree, List *activeRIRs, bool forUpdatePushedDown)
 
 		/* Only normal relations can have RLS policies */
 		if (rte->rtekind != RTE_RELATION ||
-			rte->relkind != RELKIND_RELATION)
+			(rte->relkind != RELKIND_RELATION &&
+			rte->relkind != RELKIND_PARTITIONED_TABLE))
 			continue;
 
 		rel = heap_open(rte->relid, NoLock);
diff --git a/src/test/regress/expected/rowsecurity.out b/src/test/regress/expected/rowsecurity.out
index 7bf2936..1e35498 100644
--- a/src/test/regress/expected/rowsecurity.out
+++ b/src/test/regress/expected/rowsecurity.out
@@ -899,6 +899,818 @@ EXPLAIN (COSTS OFF) SELECT * FROM t1 WHERE f_leak(b);
  Filter: f_leak(b)
 (7 rows)
 
+--
+-- Partitioned Tables
+--
+SET SESSION AUTHORIZATION regress_rls_alice;
+CREATE TABLE part_category (
+cidint primary key,
+cname  text
+);
+GRANT ALL ON part_category TO public;
+INSERT INTO part_category VALUES
+(11, 'fiction'),
+(55, 'satire'),
+(99, 'nonfiction');
+CREATE TABLE part_document (
+did int,
+cid int,
+dlevel  int not null,
+dauthor name,
+dtitle  text
+) PARTITION BY RANGE (cid);
+GRANT ALL ON part_document TO public;
+-- Create partitions for document categories
+CREATE TABLE part_document_fiction (
+	LIKE part_document INCLUDING ALL
+) PARTITION BY RANGE (dlevel);
+CREATE TABLE part_document_satire (
+	LIKE part_document INCLUDING ALL
+) PARTITION BY RANGE (dlevel);
+CREATE TABLE part_document_nonfiction (
+	LIKE part_document INCLUDING ALL
+) PARTITION BY RANGE (dlevel);
+ALTER TABLE part_document ATTACH PARTITION part_document_fiction FOR VALUES FROM ('11') TO ('12');
+ALTER TABLE part_document ATTACH PARTITION part_document_satire FOR VALUES FROM ('55') TO ('56');
+ALTER TABLE part_document ATTACH PARTITION part_document_nonfiction FOR VALUES FROM ('99') TO ('100');
+-- Create partitions for document levels
+CREATE TABLE part_document_fiction_1 (LIKE part_document_fiction INCLUDING ALL);
+CREATE TABLE part_document_fiction_2 (LIKE part_document_fiction INCLUDING ALL);
+CREATE TABLE part_document_satire_1 (LIKE part_document_satire INCLUDING ALL);
+CREATE TABLE part_document_satire_2 (LIKE part_document_satire INCLUDING ALL);
+CREATE TABLE part_document_nonfiction_1 (LIKE part_document_nonfiction INCLUDING ALL);
+CREATE TABLE part_document_nonfiction_2 (LIKE part_document_nonfiction INCLUDING ALL);
+GRANT ALL ON part_document_fiction_1 TO public;
+GRANT ALL ON part_document_fiction_2 TO public;
+GRANT ALL ON part_document_satire_1 TO public;
+GRANT ALL ON part_document_satire_2 TO public;
+GRANT ALL ON 

Re: [HACKERS] libpqrcv_PQexec() seems to violate latch protocol

[Andres Freund]

On 2017-06-06 23:24:50 +0200, Petr Jelinek wrote:
> On 06/06/17 23:17, Andres Freund wrote:
> > Right.  I found a couple more instance of similarly iffy, although not
> > quite as broken, patterns in launcher.c.  It's easy to get this wrong,
> > but it's a lot easy if you do it differently everywhere you use a
> > latch.  It's not good if code in the same file, by the same author(s),
> > has different ways of using latches.
> 
> Huh? I see same pattern everywhere in launcher.c, what am I missing?

WaitForReplicationWorkerAttach:
while (...)
CHECK_FOR_INTERRUPTS();
/* other stuff including returns */
WaitLatch()
WL_POSTMASTER_DEATH
ResetLatch()

logicalrep_worker_stop loop 1:
while (...)
/* other stuff */
CHECK_FOR_INTERRUPTS()
WaitLatch()
POSTMASTER_DEATH
ResetLatch()
/* other stuff including returns */
logicalrep_worker_stop loop 1:
while (...)
/* other stuff including returns */
CHECK_FOR_INTERRUPTS();
WaitLatch()
WL_POSTMASTER_DEATH
ResetLatch()

ApplyLauncherMain:
while (!got_SIGTERM)
/* lots other stuff */
WaitLatch()
WL_POSTMASTER_DEATH
/* some other stuff */
ResetLatch()
(note no CFI)

they're not hugely different, but subtely there are differences.
Sometimes you're guaranteed to check for interrupts after resetting the
latch, in other cases not. Sometimes expensive-ish things happen before
a CFI...

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] PG10 transition tables, wCTEs and multiple operations on the same table

[Robert Haas]

On Tue, Jun 6, 2017 at 4:25 PM, Kevin Grittner  wrote:
> Nice as it would be to add a SQL standard feature and advance the
> effort to get to incremental maintenance of materialized views, and
> much as I really appreciate the efforts Thomas has put into trying
> to solve these problems, I agree that it is best to revert the
> feature.  It took years to get an in-depth review, then I was asked
> not to commit it because others were working on patches that would
> conflict.  That just doesn't leave enough time to address these
> issues before release.  Fundamentally, I'm not sure that there is a
> level of interest sufficient to support the effort.

I find it a little unfair that you are blaming the community for not
taking enough interest in this feature when other people are posting
bug fixes for the feature which you can't find time to review (or even
respond to with an i'm-too-busy email) for lengthy periods of time.
Previously, I took the time to review and commit fixes for several
reported fixes which Thomas developed, while you ignored all of the
relevant threads.  Now, he's posted another patch which you said you
would review by last Friday or at the latest by Monday and which you
have not reviewed.  He's also suggested that the fix for this issue
probably relies on that one, so getting that patch reviewed is
relevant to this thread also.

I think I'd like to walk back my earlier statements about reverting
this patch just a little bit.  Although putting the tuplestore at the
wrong level does seem like a fairly significant design mistake, Thomas
more or less convinced me yesterday on a Skype call that relocating it
to the ModifyTable node might not be that much work.  If it's a
150-line patch, it'd probably be less disruptive to install a fix than
to revert the whole thing (and maybe put it back in again, in some
future release).  On the other hand, if you aren't willing to write,
review, or commit any further patches to fix bugs in this feature,
then it can really only stay in the tree if somebody else is willing
to assume completely responsibility for it going forward.

So, are you willing and able to put any effort into this, like say
reviewing the patch Thomas posted, and if so when and how much?  If
you're just done and you aren't going to put any more work into
maintaining it (for whatever reasons), then I think you should say so
straight out.  People shouldn't have to guess whether you're going to
maintain your patch or not.

-- 
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] libpqrcv_PQexec() seems to violate latch protocol

[Petr Jelinek]

On 06/06/17 23:17, Andres Freund wrote:
> On 2017-06-06 17:14:59 -0400, Tom Lane wrote:
>> Andres Freund  writes:
>>> The function  in $subject does:
>>
>>> ResetLatch(>procLatch);
>>> rc = WaitLatchOrSocket(>procLatch,
>>>WL_POSTMASTER_DEATH | WL_SOCKET_READABLE 
>>> |
>>>WL_LATCH_SET,
>>>PQsocket(streamConn),
>>>0,
>>>WAIT_EVENT_LIBPQWALRECEIVER);
>>
>> Yeah, this is certainly broken.
>>
>>> Afaict, the ResetLatch() really should just instead be in the if (rc & 
>>> WL_LATCH_SET) block.
>>
>> And, to be specific, it should be before the CHECK_FOR_INTERRUPTS call,
>> since that is the useful work that we want to be sure occurs after
>> any latch-setting event.
> 
> Right.  I found a couple more instance of similarly iffy, although not
> quite as broken, patterns in launcher.c.  It's easy to get this wrong,
> but it's a lot easy if you do it differently everywhere you use a
> latch.  It's not good if code in the same file, by the same author(s),
> has different ways of using latches.

Huh? I see same pattern everywhere in launcher.c, what am I missing?

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


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

Re: [HACKERS] logical replication - still unstable after all these months

[Petr Jelinek]

On 06/06/17 21:09, Robert Haas wrote:
> On Tue, Jun 6, 2017 at 3:01 PM, Erik Rijkers  wrote:
>> Belated apologies all round for the somewhat provocative $subject; but I
>> felt at that moment that this item needed some extra attention.
> 
> FWIW, it seemed like a pretty fair subject line to me given your test
> results.  I think it's in everyone's interest to get this feature
> stabilized before we ship a final release - people will rely on it,
> and if it drops even one row even occasionally, it's going to be a big
> problem for our users.
> 

+1, I considered the issues solved when snapshot builder issues were
fixed so it was good reminder to check again.

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


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

Re: [HACKERS] libpqrcv_PQexec() seems to violate latch protocol

[Andres Freund]

On 2017-06-06 17:14:59 -0400, Tom Lane wrote:
> Andres Freund  writes:
> > The function  in $subject does:
> 
> > ResetLatch(>procLatch);
> > rc = WaitLatchOrSocket(>procLatch,
> >WL_POSTMASTER_DEATH | WL_SOCKET_READABLE 
> > |
> >WL_LATCH_SET,
> >PQsocket(streamConn),
> >0,
> >WAIT_EVENT_LIBPQWALRECEIVER);
> 
> Yeah, this is certainly broken.
> 
> > Afaict, the ResetLatch() really should just instead be in the if (rc & 
> > WL_LATCH_SET) block.
> 
> And, to be specific, it should be before the CHECK_FOR_INTERRUPTS call,
> since that is the useful work that we want to be sure occurs after
> any latch-setting event.

Right.  I found a couple more instance of similarly iffy, although not
quite as broken, patterns in launcher.c.  It's easy to get this wrong,
but it's a lot easy if you do it differently everywhere you use a
latch.  It's not good if code in the same file, by the same author(s),
has different ways of using latches.

- 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] libpqrcv_PQexec() seems to violate latch protocol

[Tom Lane]

Andres Freund  writes:
> The function  in $subject does:

> ResetLatch(>procLatch);
> rc = WaitLatchOrSocket(>procLatch,
>WL_POSTMASTER_DEATH | WL_SOCKET_READABLE |
>WL_LATCH_SET,
>PQsocket(streamConn),
>0,
>WAIT_EVENT_LIBPQWALRECEIVER);

Yeah, this is certainly broken.

> Afaict, the ResetLatch() really should just instead be in the if (rc & 
> WL_LATCH_SET) block.

And, to be specific, it should be before the CHECK_FOR_INTERRUPTS call,
since that is the useful work that we want to be sure occurs after
any latch-setting event.

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] libpqrcv_PQexec() seems to violate latch protocol

[Andres Freund]

Hi,

The function  in $subject does:
while (PQisBusy(streamConn))
{
int rc;

/*
 * We don't need to break down the sleep into smaller increments,
 * since we'll get interrupted by signals and can either handle
 * interrupts here or elog(FATAL) within SIGTERM signal handler if
 * the signal arrives in the middle of establishment of
 * replication connection.
 */
ResetLatch(>procLatch);
rc = WaitLatchOrSocket(>procLatch,
   WL_POSTMASTER_DEATH | WL_SOCKET_READABLE |
   WL_LATCH_SET,
   PQsocket(streamConn),
   0,
   WAIT_EVENT_LIBPQWALRECEIVER);
if (rc & WL_POSTMASTER_DEATH)
exit(1);
/* interrupted */
if (rc & WL_LATCH_SET)
{
CHECK_FOR_INTERRUPTS();
continue;
}

Doing ResetLatch();WaitLatch() like that makes it possible to miss a the
latch being set, e.g. if it happens just after WaitLatchOrSocket()
returns.

Afaict, the ResetLatch() really should just instead be in the if (rc & 
WL_LATCH_SET)
block.

Unless somebody protests, I'll make it so.

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] PG10 transition tables, wCTEs and multiple operations on the same table

[Kevin Grittner]

On Tue, Jun 6, 2017 at 3:41 PM, Marko Tiikkaja  wrote:
> On Tue, Jun 6, 2017 at 10:25 PM, Kevin Grittner  wrote:

>> It took years to get an in-depth review, then I was asked
>> not to commit it because others were working on patches that would
>> conflict.  That just doesn't leave enough time to address these
>> issues before release.  Fundamentally, I'm not sure that there is a
>> level of interest sufficient to support the effort.

> I can only say that the lack of this feature comes up on a weekly basis on
> IRC, and a lot of people would be disappointed to see it reverted.

Well, at PGCon I talked with someone who worked on the
implementation in Oracle 20-some years ago.  He said they had a team
of 20 people full time working on the feature for years to get it
working.  Now, I think the PostgreSQL community is a little lighter
on its feet, but without more active participation from others than
there has been so far, I don't intend to take another run at it.
I'll be happy to participate at such point that it's not treated as
a second-class feature set.  Barring that, anyone else who wants to
take the patch and run with it is welcome to do so.

--
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] postgres_fdw cost estimation defaults and documentation

[Robert Haas]

On Mon, Jun 5, 2017 at 2:37 PM, Jeff Janes  wrote:
> In the documentation for fdw_startup_cost, it says "This represents the
> additional overhead of establishing a connection, parsing and planning the
> query on the remote side, etc.".  I think that "establishing a connection"
> should be stricken. Either you need a connection or you don't, there is
> nothing the optimizer can do about this. And if do need one, you only
> establish one once (at most), not once per query sent to the remote side.  I
> think the implementation correctly doesn't try to account for the overhead
> of establishing a connection, so the docs should remove that claim.

I don't really think there's anything wrong with having "establishing
a connection" in this paragraph.  There's a difference between
estimating something in a simplistic way that doesn't necessarily
capture all the variables inherent in the real cost, and not intending
to estimate it.  For example, seq_page_cost and random_page_cost
estimate the cost of reading a page, and they make no attempt to
distinguish between the cost of reading a page from the OS page cache
and reading a page from disk, even though those things take radically
different amounts of time.  The fact that the physical I/O happens
only sometimes doesn't mean that these GUCs aren't trying to account
for that cost, and, similarly, the fact that a connection to the
foreign server needs to be established only sometimes does not mean
that fdw_startup_cost isn't trying to cover the cost of that.  You can
adjust random_page_cost and seq_page_cost up or down depending on how
much caching you think you'll get (and the documentation recommends
this).  And you can adjust fdw_startup_cost based on how often you
think you'll need to establish a new connection (but it's probably not
worth bothering with).

-- 
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] PG10 transition tables, wCTEs and multiple operations on the same table

[Marko Tiikkaja]

On Tue, Jun 6, 2017 at 10:25 PM, Kevin Grittner  wrote:

> Nice as it would be to add a SQL standard feature and advance the
> effort to get to incremental maintenance of materialized views, and
> much as I really appreciate the efforts Thomas has put into trying
> to solve these problems, I agree that it is best to revert the
> feature.  It took years to get an in-depth review, then I was asked
> not to commit it because others were working on patches that would
> conflict.  That just doesn't leave enough time to address these
> issues before release.  Fundamentally, I'm not sure that there is a
> level of interest sufficient to support the effort.
>
> I'll give it a few days for objections before reverting.
>

I can only say that the lack of this feature comes up on a weekly basis on
IRC, and a lot of people would be disappointed to see it reverted.


.m

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

[Jeevan Ladhe]

Hi Ashutosh,

Thanks for the detailed review.

Also, please find my feedback on your comments in-lined, I also addressed
the comments given by Robert in attached patch:

On Sat, Jun 3, 2017 at 5:13 PM, Ashutosh Bapat  wrote:

> Here's some detailed review of the code.
>
> @@ -1883,6 +1883,15 @@ heap_drop_with_catalog(Oid relid)
>  if (OidIsValid(parentOid))
>  {
>  /*
> + * Default partition constraints are constructed run-time from the
> + * constraints of its siblings(basically by negating them), so any
> + * change in the siblings needs to rebuild the constraints of the
> + * default partition. So, invalidate the sibling default
> partition's
> + * relcache.
> + */
> +InvalidateDefaultPartitionRelcache(parentOid);
> +
> Do we need a lock on the default partition for doing this? A query might be
> scanning the default partition directly and we will invalidate the relcache
> underneath it. What if two partitions are being dropped simultaneously and
> change default constraints simultaneously. Probably the lock on the parent
> helps there, but need to check it. What if the default partition cache is
> invalidated because partition gets added/dropped to the default partition
> itself. If we need a lock on the default partition, we will need to
> check the order in which we should be obtaining the locks so as to avoid
> deadlocks.


Done. I have taken a lock on default partition after acquiring a lock on
parent
relation where ever applicable.


> This also means that we have to test PREPARED statements involving
> default partition. Any addition/deletion/attach/detach of other partition
> should invalidate those cached statements.
>

Will add this in next version of patch.


> +if (partition_bound_has_default(boundinfo))
> +{
> +overlap = true;
> +with = boundinfo->default_index;
> +}
> You could possibly rewrite this as
> overlap = partition_bound_has_default(boundinfo);
> with = boundinfo->default_index;
> that would save one indentation and a conditional jump.


Done


> +if (partdesc->nparts > 0 && partition_bound_has_default(boundinfo))
> +check_default_allows_bound(parent, spec);
> If the table has a default partition, nparts > 0, nparts > 0 check looks
> redundant. The comments above should also explain that this check doesn't
> trigger when a default partition is added since we don't expect an existing
> default partition in such a case.
>

The check nparts > 0, is needed to make sure that the boundinfo is non-null,
i.e. to confirm that there exists at least one partition so that
partition_bound_has_default() does not result in segmentation fault.
I have changed the condition as below to make it more intuitive:
if (boundinfo && partition_bound_has_default(boundinfo))
Also, I have updated the comment.


> + * Checks if there exists any row in the default partition that passes the
> + * check for constraints of new partition, if any reports an error.
> grammar two conflicting ifs in the same statement. You may want to rephrase
> this as "This function checks if there exists a row in the default
> partition that fits in the new
> partition and throws an error if it finds one."
>

Done


> +if (new_spec->strategy != PARTITION_STRATEGY_LIST)
> +return;
> This should probably be an Assert. When default range partition is
> supported
> this function would silently return, meaning there is no row in the default
> partition which fits the new partition. We don't want that behavior.
>

Agreed, changed.


> The code in check_default_allows_bound() to check whether the default
> partition
> has any rows that would fit new partition looks quite similar to the code
> in
> ATExecAttachPartition() checking whether all rows in the table being
> attached
> as a partition fit the partition bounds. One thing that
> check_default_allows_bound() misses is, if there's already a constraint on
> the
> default partition refutes the partition constraint on the new partition,
> we can
> skip the scan of the default partition since it can not have rows that
> would
> fit the new partition. ATExecAttachPartition() has code to deal with a
> similar
> case i.e. the table being attached has a constraint which implies the
> partition
> constraint. There may be more cases which check_default_allows_bound()
> does not
> handle but ATExecAttachPartition() handles. So, I am wondering whether it's
> better to somehow take out the common code into a function and use it. We
> will
> have to deal with a difference through. The first one would throw an error
> when
> finding a row that satisfies partition constraints whereas the second one
> would
> throw an error when it doesn't find such a row. But this difference can be
> handled through a flag or by negating the constraint. This would 

Re: [HACKERS] Broken hint bits (freeze)

[Sergey Burladyan]

Dmitriy Sarafannikov  writes:

> Starting and stopping master after running pg_upgrade but before rsync to 
> collect statistics
> was a bad idea.

But, starting and stopping master after running pg_upgrade is *required*
by documentation:
https://www.postgresql.org/docs/9.6/static/pgupgrade.html
> f. Start and stop the new master cluster
> In the new master cluster, change wal_level to replica in the postgresql.conf 
> file and then start and stop the cluster.

and there is no any suggestion to disable autovacuum for it.

-- 
Sergey Burladyan


-- 
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] intermittent failures in Cygwin from select_parallel tests

[Robert Haas]

On Tue, Jun 6, 2017 at 4:25 PM, Tom Lane  wrote:
> (I'm tempted to add something like this permanently, at DEBUG1 or DEBUG2
> or so.)

I don't mind adding it permanently, but I think that's too high.
Somebody running a lot of parallel queries could easily get enough
messages to drown out the stuff they actually want to see at that
level.  I wouldn't object to DEBUG3 though.

-- 
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] intermittent failures in Cygwin from select_parallel tests

[Tom Lane]

Robert Haas  writes:
> On Tue, Jun 6, 2017 at 2:21 PM, Tom Lane  wrote:
>> Hmm.  With some generous assumptions it'd be possible to think that
>> aa1351f1eec4adae39be59ce9a21410f9dd42118 triggered this.  That commit was
>> present in 20 successful lorikeet runs before the first of these failures,
>> which is a bit more than the MTBF after that, but not a huge amount more.
>> That commit in itself looks innocent enough, but could it have exposed
>> some latent bug in bgworker launching?

> Hmm, that's a really interesting idea, but I can't quite put together
> a plausible theory around it.

Yeah, me either, but we're really theorizing in advance of the data here.
Andrew, could you apply the attached patch on lorikeet and run the
regression tests enough times to get a couple of failures?  Then grepping
the postmaster log for 'parallel worker' should give you results like

2017-06-06 16:20:12.393 EDT [31216] LOG:  starting PID 31216, parallel worker 
for PID 31215, worker number 0
2017-06-06 16:20:12.400 EDT [31216] LOG:  stopping PID 31216, parallel worker 
for PID 31215, worker number 0
2017-06-06 16:20:12.406 EDT [31217] LOG:  starting PID 31217, parallel worker 
for PID 31215, worker number 3
2017-06-06 16:20:12.406 EDT [31218] LOG:  starting PID 31218, parallel worker 
for PID 31215, worker number 2
2017-06-06 16:20:12.406 EDT [31219] LOG:  starting PID 31219, parallel worker 
for PID 31215, worker number 1
2017-06-06 16:20:12.406 EDT [31220] LOG:  starting PID 31220, parallel worker 
for PID 31215, worker number 0
2017-06-06 16:20:12.412 EDT [31218] LOG:  stopping PID 31218, parallel worker 
for PID 31215, worker number 2
2017-06-06 16:20:12.412 EDT [31219] LOG:  stopping PID 31219, parallel worker 
for PID 31215, worker number 1
2017-06-06 16:20:12.412 EDT [31220] LOG:  stopping PID 31220, parallel worker 
for PID 31215, worker number 0
2017-06-06 16:20:12.412 EDT [31217] LOG:  stopping PID 31217, parallel worker 
for PID 31215, worker number 3
... etc etc ...

If it looks different from that in a crash case, we'll have something
to go on.

(I'm tempted to add something like this permanently, at DEBUG1 or DEBUG2
or so.)

regards, tom lane

diff --git a/src/backend/access/transam/parallel.c b/src/backend/access/transam/parallel.c
index cb22174..d3cb26c 100644
*** a/src/backend/access/transam/parallel.c
--- b/src/backend/access/transam/parallel.c
*** ParallelWorkerMain(Datum main_arg)
*** 950,955 
--- 950,961 
  	Assert(ParallelWorkerNumber == -1);
  	memcpy(, MyBgworkerEntry->bgw_extra, sizeof(int));
  
+ 	/* Log parallel worker startup. */
+ 	ereport(LOG,
+ 			(errmsg("starting PID %d, %s, worker number %d",
+ 	MyProcPid, MyBgworkerEntry->bgw_name,
+ 	ParallelWorkerNumber)));
+ 
  	/* Set up a memory context and resource owner. */
  	Assert(CurrentResourceOwner == NULL);
  	CurrentResourceOwner = ResourceOwnerCreate(NULL, "parallel toplevel");
*** ParallelWorkerMain(Datum main_arg)
*** 1112,1117 
--- 1118,1129 
  
  	/* Report success. */
  	pq_putmessage('X', NULL, 0);
+ 
+ 	/* Log parallel worker shutdown. */
+ 	ereport(LOG,
+ 			(errmsg("stopping PID %d, %s, worker number %d",
+ 	MyProcPid, MyBgworkerEntry->bgw_name,
+ 	ParallelWorkerNumber)));
  }
  
  /*

-- 
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] PG10 transition tables, wCTEs and multiple operations on the same table

[Kevin Grittner]

Nice as it would be to add a SQL standard feature and advance the
effort to get to incremental maintenance of materialized views, and
much as I really appreciate the efforts Thomas has put into trying
to solve these problems, I agree that it is best to revert the
feature.  It took years to get an in-depth review, then I was asked
not to commit it because others were working on patches that would
conflict.  That just doesn't leave enough time to address these
issues before release.  Fundamentally, I'm not sure that there is a
level of interest sufficient to support the effort.

I'll give it a few days for objections before reverting.

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

[HACKERS] Re: [BUGS] BUG #14682: row level security not work with partitioned table

[Joe Conway]

On 06/06/2017 11:57 AM, Mike Palmiotto wrote:
> On Mon, Jun 5, 2017 at 10:36 AM, Robert Haas  wrote:
>> On Mon, Jun 5, 2017 at 10:20 AM, Joe Conway  wrote:
>>> Unless Robert objects, I'll work with Mike to get a fix posted and
>>> committed in the next day or two.
>>
>> That would be great.  Thanks.
> 
> I have the updated patch with rowsecurity regression tests and rebased
> on master. I've run these and verified locally by feeding
> rowsecurity.sql to psql, but have yet to get the full regression suite
> passing -- it's failing on the constraints regtest and then gets stuck
> in recovery. Undoubtedly something to do with my
> configuration/environment over here. I'm working through those issues
> right now. In the meantime, if you want to see the regression tests as
> they stand, please see the attached patch.

The constraints test passes here, so presumably something you borked
locally. I only see a rowsecurity failure, which is not surprising since
your patch does not include the changes to expected output ;-)
Please resend with src/test/regress/expected/rowsecurity.out included.

Thanks,

Joe

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



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] Why does logical replication launcher set application_name?

[Robert Haas]

On Sat, Jun 3, 2017 at 3:33 AM, Kuntal Ghosh  wrote:
>> Agreed with this.
>>
>> However, I am not sure about the bgw_name_extra. I think I would have
>> preferred keeping full bgw_name field which would be used where full
>> name is needed and bgw_type where only the worker type is used. The
>> concatenation just doesn't sit well with me, especially if it requires
>> the bgw_name_extra to start with space.
>
> +1.

+1 from me, too.

The problem with the status quo (after Peter's commit) is that there's
now nothing at all to identify the logical replication launcher, apart
from the wait_event field, which is likely to be LogicalLauncherMain
fairly often if you've got the launcher.  I don't personally see why
we can't simply adopt Tom's original proposal of setting the query
string to something like "" which, while
maybe not as elegant as providing some way to override the
backend_type field, would be almost no work and substantially better
for v10 than what we've got now.

-- 
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] Error while creating subscription when server is running in single user mode

[Andres Freund]

On 2017-06-06 15:48:42 -0400, Robert Haas wrote:
> On Fri, Jun 2, 2017 at 3:24 PM, Andres Freund  wrote:
> > Latches work in single user mode, it's just that the new code for some
> > reason uses uninitialized memory as the latch.  As I pointed out above,
> > the new code really should just use MyLatch instead of
> > MyProc->procLatch.

FWIW, I'd misremembered some code here, and we actually reach the
function initializing the shared latch, even in single user mode.


> We seem to have accumulated quite a few instance of that.
> 
> [rhaas pgsql]$ git grep MyLatch | wc -l
>  116
> [rhaas pgsql]$ git grep 'MyProc->procLatch' | wc -l
>   33
> 
> Most of the offenders are in src/backend/replication, but there are
> some that are related to parallelism as well (bgworker.c, pqmq.c,
> parallel.c, condition_variable.c).  Maybe we (you?) should just go and
> change them all.  I don't think using MyLatch instead of
> MyProc->procLatch has become automatic for everyone yet.

Nevertheless this should be changed.  Will do.


- 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] Error while creating subscription when server is running in single user mode

[Robert Haas]

On Fri, Jun 2, 2017 at 3:24 PM, Andres Freund  wrote:
> Latches work in single user mode, it's just that the new code for some
> reason uses uninitialized memory as the latch.  As I pointed out above,
> the new code really should just use MyLatch instead of
> MyProc->procLatch.

We seem to have accumulated quite a few instance of that.

[rhaas pgsql]$ git grep MyLatch | wc -l
 116
[rhaas pgsql]$ git grep 'MyProc->procLatch' | wc -l
  33

Most of the offenders are in src/backend/replication, but there are
some that are related to parallelism as well (bgworker.c, pqmq.c,
parallel.c, condition_variable.c).  Maybe we (you?) should just go and
change them all.  I don't think using MyLatch instead of
MyProc->procLatch has become automatic for everyone yet.

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


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

Re: [HACKERS] Fix performance degradation of contended LWLock on NUMA

[Robert Haas]

On Mon, Jun 5, 2017 at 9:22 AM, Sokolov Yura
 wrote:
> Good day, everyone.
>
> This patch improves performance of contended LWLock.
> It was tested on 4 socket 72 core x86 server (144 HT) Centos 7.1
> gcc 4.8.5
>
> Patch makes lock acquiring in single CAS loop:
> 1. LWLock->state is read, and ability for lock acquiring is detected.
>   If there is possibility to take a lock, CAS tried.
>   If CAS were successful, lock is aquired (same to original version).
> 2. but if lock is currently held by other backend, we check ability for
>   taking WaitList lock. If wait list lock is not help by anyone, CAS
>   perfomed for taking WaitList lock and set LW_FLAG_HAS_WAITERS at once.
>   If CAS were successful, then LWLock were still held at the moment wait
>   list lock were held - this proves correctness of new algorithm. And
>   Proc is queued to wait list then.
> 3. Otherwise spin_delay is performed, and loop returns to step 1.

Interesting work.  Thanks for posting.

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


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

Re: [HACKERS] Parallel Aggregation support for aggregate functions that use transitions not implemented for array_agg

[Andres Freund]

On 2017-06-06 12:23:49 -0700, David Fetter wrote:
> On Tue, Jun 06, 2017 at 01:52:45PM -0400, Regina Obe wrote:
> > It seems CREATE  AGGREGATE was expanded in 9.6 to support parallelization of
> > aggregate functions using transitions, with the addition of serialfunc and
> > deserialfunc to the aggregate definitions.
> > 
> > https://www.postgresql.org/docs/10/static/sql-createaggregate.html
> > 
> > I was looking at the PostgreSQL 10 source code for some example usages of
> > this and was hoping that array_agg and string_agg would support the feature.
> > At a cursory glance, it seems they do not use this.
> > Examples I see that do support it are the average and standard deviation
> > functions.
> > 
> > Is there a reason for this or it just wasn't gotten to?

I'd suggest trying to write a parallel version of them ;).  Shouldn't be
too hard.


> I'd bet on lack of tuits.  Anything with text has to deal with
> collation issues, etc., that may make this trickier than it first
> appears.

I don't see how collations makes things more complicated here.

- 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] Parallel Aggregation support for aggregate functions that use transitions not implemented for array_agg

[David Fetter]

On Tue, Jun 06, 2017 at 01:52:45PM -0400, Regina Obe wrote:
> It seems CREATE  AGGREGATE was expanded in 9.6 to support parallelization of
> aggregate functions using transitions, with the addition of serialfunc and
> deserialfunc to the aggregate definitions.
> 
> https://www.postgresql.org/docs/10/static/sql-createaggregate.html
> 
> I was looking at the PostgreSQL 10 source code for some example usages of
> this and was hoping that array_agg and string_agg would support the feature.
> At a cursory glance, it seems they do not use this.
> Examples I see that do support it are the average and standard deviation
> functions.
> 
> Is there a reason for this or it just wasn't gotten to?

I'd bet on lack of tuits.  Anything with text has to deal with
collation issues, etc., that may make this trickier than it first
appears.

Best,
David.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter  XMPP: david(dot)fetter(at)gmail(dot)com

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


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

Re: [HACKERS] Re: Alter subscription..SET - NOTICE message is coming for table which is already removed

[Peter Eisentraut]

On 6/3/17 01:04, Noah Misch wrote:
> On Wed, May 31, 2017 at 10:15:50PM -0400, Peter Eisentraut wrote:
>> On 5/30/17 13:25, Masahiko Sawada wrote:
>>> I think this cause is that the relation status entry could be deleted
>>> by ALTER SUBSCRIPTION REFRESH before corresponding table sync worker
>>> starting. Attached patch fixes issues reported on this thread so far.
>>
>> This looks like a reasonable change, but it conflicts with the change
>> discussed on "logical replication - still unstable after all these
>> months".  I think I'll deal with that one first.
> 
> The above-described topic is currently a PostgreSQL 10 open item.  You own
> this open item.  Please observe the policy on open item ownership and send a
> status update within three calendar days of this message.  Include a date for
> your subsequent status update.

I'm working on this now and will report back tomorrow.

-- 
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] Should we standardize on a type for signal handler flags?

[Andres Freund]

On 2017-06-06 14:45:37 -0400, Tom Lane wrote:
> Andres Freund  writes:
> > A reasonable rule would actually be to only use [u]int32 and
> > sig_atomic_t, never bool.
> 
> Yes, I'd agree with that.

Cool.  I propose we change, once branched, the existing code using
booleans, and codify that practice in sources.sgml already existing
section about signal handlers.

- 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] logical replication - still unstable after all these months

[Robert Haas]

On Tue, Jun 6, 2017 at 3:01 PM, Erik Rijkers  wrote:
> Belated apologies all round for the somewhat provocative $subject; but I
> felt at that moment that this item needed some extra attention.

FWIW, it seemed like a pretty fair subject line to me given your test
results.  I think it's in everyone's interest to get this feature
stabilized before we ship a final release - people will rely on it,
and if it drops even one row even occasionally, it's going to be a big
problem for our users.

-- 
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] intermittent failures in Cygwin from select_parallel tests

[Robert Haas]

On Tue, Jun 6, 2017 at 2:21 PM, Tom Lane  wrote:
>> One thought is that the only places where shm_mq_set_sender() should
>> be getting invoked during the main regression tests are
>> ParallelWorkerMain() and ExecParallelGetReceiver, and both of those
>> places using ParallelWorkerNumber to figure out what address to pass.
>> So if ParallelWorkerNumber were getting set to the same value in two
>> different parallel workers - e.g. because the postmaster went nuts and
>> launched two processes instead of only one - or if
>> ParallelWorkerNumber were not getting initialized at all or were
>> getting initialized to some completely bogus value, it could cause
>> this symptom.
>
> Hmm.  With some generous assumptions it'd be possible to think that
> aa1351f1eec4adae39be59ce9a21410f9dd42118 triggered this.  That commit was
> present in 20 successful lorikeet runs before the first of these failures,
> which is a bit more than the MTBF after that, but not a huge amount more.
>
> That commit in itself looks innocent enough, but could it have exposed
> some latent bug in bgworker launching?

Hmm, that's a really interesting idea, but I can't quite put together
a plausible theory around it.  I mean, it seems like that commit could
make launching bgworkers faster, which could conceivably tickle some
heretofore-latent timing-related bug.  But it wouldn't, IIUC, make the
first worker start any faster than before - it would just make them
more closely-spaced thereafter, and it's not very obvious how that
would cause a problem.

Another idea is that the commit in question is managing to corrupt
BackgroundWorkerList somehow.  maybe_start_bgworkers() is using
slist_foreach_modify(), but previously it always returned after
calling do_start_bgworker, and now it doesn't.  So if
do_start_bgworker() did something that could modify the list
structure, then perhaps maybe_start_bgworkers() would get confused.  I
don't really think that this theory has any legs, though.

-- 
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 - still unstable after all these months

[Erik Rijkers]

On 2017-06-06 20:53, Peter Eisentraut wrote:

On 6/4/17 22:38, Petr Jelinek wrote:

Committed that, with some further updates of comments to reflect the


Belated apologies all round for the somewhat provocative $subject; but I 
felt at that moment that this item needed some extra attention.


I don't know if it worked but I'm glad that it is solved ;)

Thanks,

Erik Rijkers





--
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] [BUGS] BUG #14682: row level security not work with partitioned table

[Mike Palmiotto]

On Mon, Jun 5, 2017 at 10:36 AM, Robert Haas  wrote:
> On Mon, Jun 5, 2017 at 10:20 AM, Joe Conway  wrote:
>> Unless Robert objects, I'll work with Mike to get a fix posted and
>> committed in the next day or two.
>
> That would be great.  Thanks.

I have the updated patch with rowsecurity regression tests and rebased
on master. I've run these and verified locally by feeding
rowsecurity.sql to psql, but have yet to get the full regression suite
passing -- it's failing on the constraints regtest and then gets stuck
in recovery. Undoubtedly something to do with my
configuration/environment over here. I'm working through those issues
right now. In the meantime, if you want to see the regression tests as
they stand, please see the attached patch.

Thanks,

-- 
Mike Palmiotto
Software Engineer
Crunchy Data Solutions
https://crunchydata.com
From 48a9586881872d4b8c9ca77e0c0da48db611e326 Mon Sep 17 00:00:00 2001
From: Mike Palmiotto 
Date: Wed, 24 May 2017 16:54:49 +
Subject: [PATCH] Add RLS support to partitioned tables

This is needed to get RLS policies to apply to the parent partitioned table.
Without this change partitioned tables are skipped.
---
 src/backend/rewrite/rewriteHandler.c |   3 +-
 src/test/regress/sql/rowsecurity.sql | 223 +++
 2 files changed, 225 insertions(+), 1 deletion(-)

diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c
index 35ff8bb..6cd73c1 100644
--- a/src/backend/rewrite/rewriteHandler.c
+++ b/src/backend/rewrite/rewriteHandler.c
@@ -1835,7 +1835,8 @@ fireRIRrules(Query *parsetree, List *activeRIRs, bool forUpdatePushedDown)
 
 		/* Only normal relations can have RLS policies */
 		if (rte->rtekind != RTE_RELATION ||
-			rte->relkind != RELKIND_RELATION)
+			(rte->relkind != RELKIND_RELATION &&
+			rte->relkind != RELKIND_PARTITIONED_TABLE))
 			continue;
 
 		rel = heap_open(rte->relid, NoLock);
diff --git a/src/test/regress/sql/rowsecurity.sql b/src/test/regress/sql/rowsecurity.sql
index 1b6896e..c4ba136 100644
--- a/src/test/regress/sql/rowsecurity.sql
+++ b/src/test/regress/sql/rowsecurity.sql
@@ -44,6 +44,7 @@ GRANT EXECUTE ON FUNCTION f_leak(text) TO public;
 -- BASIC Row-Level Security Scenario
 
 SET SESSION AUTHORIZATION regress_rls_alice;
+
 CREATE TABLE uaccount (
 pguser  name primary key,
 seclv   int
@@ -308,6 +309,228 @@ SET row_security TO OFF;
 SELECT * FROM t1 WHERE f_leak(b);
 EXPLAIN (COSTS OFF) SELECT * FROM t1 WHERE f_leak(b);
 
+--
+-- Partitioned Tables
+--
+
+SET SESSION AUTHORIZATION regress_rls_alice;
+
+CREATE TABLE part_category (
+cidint primary key,
+cname  text
+);
+GRANT ALL ON part_category TO public;
+INSERT INTO part_category VALUES
+(11, 'fiction'),
+(55, 'satire'),
+(99, 'nonfiction');
+
+CREATE TABLE part_document (
+did int,
+cid int,
+dlevel  int not null,
+dauthor name,
+dtitle  text
+) PARTITION BY RANGE (cid);
+GRANT ALL ON part_document TO public;
+
+-- Create partitions for document categories
+CREATE TABLE part_document_fiction (
+	LIKE part_document INCLUDING ALL
+) PARTITION BY RANGE (dlevel);
+
+CREATE TABLE part_document_satire (
+	LIKE part_document INCLUDING ALL
+) PARTITION BY RANGE (dlevel);
+
+CREATE TABLE part_document_nonfiction (
+	LIKE part_document INCLUDING ALL
+) PARTITION BY RANGE (dlevel);
+
+ALTER TABLE part_document ATTACH PARTITION part_document_fiction FOR VALUES FROM ('11') TO ('12');
+ALTER TABLE part_document ATTACH PARTITION part_document_satire FOR VALUES FROM ('55') TO ('56');
+ALTER TABLE part_document ATTACH PARTITION part_document_nonfiction FOR VALUES FROM ('99') TO ('100');
+
+-- Create partitions for document levels
+CREATE TABLE part_document_fiction_1 (LIKE part_document_fiction INCLUDING ALL);
+CREATE TABLE part_document_fiction_2 (LIKE part_document_fiction INCLUDING ALL);
+CREATE TABLE part_document_satire_1 (LIKE part_document_satire INCLUDING ALL);
+CREATE TABLE part_document_satire_2 (LIKE part_document_satire INCLUDING ALL);
+CREATE TABLE part_document_nonfiction_1 (LIKE part_document_nonfiction INCLUDING ALL);
+CREATE TABLE part_document_nonfiction_2 (LIKE part_document_nonfiction INCLUDING ALL);
+
+GRANT ALL ON part_document_fiction_1 TO public;
+GRANT ALL ON part_document_fiction_2 TO public;
+GRANT ALL ON part_document_satire_1 TO public;
+GRANT ALL ON part_document_satire_2 TO public;
+GRANT ALL ON part_document_nonfiction_1 TO public;
+GRANT ALL ON part_document_nonfiction_2 TO public;
+
+ALTER TABLE part_document_fiction ATTACH PARTITION part_document_fiction_1 FOR VALUES FROM ('1') TO ('2');
+ALTER TABLE part_document_fiction ATTACH PARTITION part_document_fiction_2 FOR VALUES FROM ('2') TO ('3');
+ALTER TABLE part_document_satire ATTACH PARTITION part_document_satire_1 FOR VALUES FROM ('1') TO ('2');
+ALTER TABLE part_document_satire ATTACH 

Re: [HACKERS] logical replication - still unstable after all these months

[Peter Eisentraut]

On 6/4/17 22:38, Petr Jelinek wrote:
> On 03/06/17 16:12, Jeff Janes wrote:
>>
>> On Fri, Jun 2, 2017 at 4:10 PM, Petr Jelinek
>> > wrote:
>>
>>
>> While I was testing something for different thread I noticed that I
>> manage transactions incorrectly in this patch. Fixed here, I didn't test
>> it much yet (it takes a while as you know :) ). Not sure if it's related
>> to the issue you've seen though.
>>
>>
>> This conflicts again with Peter E's recent commit
>> 3c9bc2157a4f465b3c070d1250597568d2dc285f
>>
> 
> Rebased on top of current HEAD.

Committed that, with some further updates of comments to reflect the
changes.

I do like the simplification of the state progression.

Perhaps it could be simplified even further, by eliminating the SYNCDONE
setting in LogicalRepSyncTableStart() and making it go through the apply
loop and end up in process_syncing_tables_for_sync() in all cases.
Which is kind of what the comments at the top of the file suggest would
happen anyway.

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


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

Re: [HACKERS] Should we standardize on a type for signal handler flags?

[Tom Lane]

Andres Freund  writes:
> A reasonable rule would actually be to only use [u]int32 and
> sig_atomic_t, never bool.

Yes, I'd agree with 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] Should we standardize on a type for signal handler flags?

[Tom Lane]

Robert Haas  writes:
> On Tue, Jun 6, 2017 at 1:33 PM, Tom Lane  wrote:
>> That's an argument from false premises.  The question here is what types
>> are safe for an interrupt handler to *change*, not what can it read.

> OK, but we certainly have code in signal handlers that does:

> int save_errno = errno;
> /* stuff */
> errno = save_errno;

> If that's not a signal handler changing an int, color me confused.

But it puts it back the same way at the end, so there's no visible issue
for mainline code.  Even if the interrupted code were in the midst of
changing adjacent values, nothing bad would happen.

Perhaps it would help to give a concrete example of the type of hazard
you face with a non-interrupt-safe datatype --- which, for the present
purpose, is one that takes more than one CPU instruction to load or
store.  (Andres is right that cross-process safety is a different and
stronger requirement.)  Suppose we have a machine with no byte-wide
load/store ops, only word-wide, and the compiler has laid out four
bool variables a,b,c,d in the same word.  If the mainline code is
trying to set d to 1, it's going to do something more or less like

ldw r2, ...address...
ori r2, $1
stw r2, ...address...

Now, if that sequence gets interrupted by a signal handler that tries to
change the value of a, b, or c, the change will be lost when we reach the
stw, because that's overwriting more than just the programmer-intended
target byte.  On the other hand, there's no problem with the handler
*reading* any of those values --- it will see a consistent state.  It also
wouldn't matter if it changed one of them and then changed it back before
returning.

In practice this would never be an issue for "errno" anyway, because int
is surely a datatype that can be loaded and stored in one instruction ---
the C standard actually defines int as being the machine's most
efficiently manipulatable type, so I think that's a pretty safe
assumption.  But I'm not convinced the same is universally true for
"char".

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] Should we standardize on a type for signal handler flags?

[Robert Haas]

On Tue, Jun 6, 2017 at 2:24 PM, Andres Freund  wrote:
> Don't think it's actually clear that errno is an integer - might very
> well be just a sig_atomic_t, which can contain values up to like 127 or
> so.   I think the bigger point Tom was making is that we actually know
> an int4 is safe - otherwise we'd have crashed and burned a long time ago
> - but that that might be different for *smaller* datatypes because
> $platform doesn't really do smaller writes atomically (turning them into
> read-or-write operations either in microcode or assembly).

Oh, right, I remember hearing about that issue before, but it had
slipped my mind completely.

> Alpha,
> s390, pa-risc appear to have such behaviour cross-cpu - although that
> doesn't necessarily imply the same is true for handlers as well.

Hmm, OK.  We've already decided Alpha is safely dead, but s390 and
pa-risc are ostensibly not dead.

-- 
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] Should we standardize on a type for signal handler flags?

[Andres Freund]

On 2017-06-06 14:13:29 -0400, Robert Haas wrote:
> On Tue, Jun 6, 2017 at 1:33 PM, Tom Lane  wrote:
> >> I think that's a pretty good argument, really.  If there exists a
> >> platform where only sig_atomic_t is safe to read from a signal
> >> handler, then we already don't work on that platform.  Even saving and
> >> restoring errno isn't safe in that case.
> >
> > That's an argument from false premises.  The question here is what types
> > are safe for an interrupt handler to *change*, not what can it read.
> 
> OK, but we certainly have code in signal handlers that does:
> 
> int save_errno = errno;
> /* stuff */
> errno = save_errno;
> 
> If that's not a signal handler changing an int, color me confused.

Don't think it's actually clear that errno is an integer - might very
well be just a sig_atomic_t, which can contain values up to like 127 or
so.   I think the bigger point Tom was making is that we actually know
an int4 is safe - otherwise we'd have crashed and burned a long time ago
- but that that might be different for *smaller* datatypes because
$platform doesn't really do smaller writes atomically (turning them into
read-or-write operations either in microcode or assembly).  Alpha,
s390, pa-risc appear to have such behaviour cross-cpu - although that
doesn't necessarily imply the same is true for handlers as well.

A reasonable rule would actually be to only use [u]int32 and
sig_atomic_t, never bool.

- 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] UPDATE of partition key

[Robert Haas]

On Mon, Jun 5, 2017 at 2:51 AM, Amit Kapila  wrote:
>> Greg/Amit's idea of using the CTID field rather than an infomask bit
>> seems like a possibly promising approach.  Not everything that needs
>> bit-space can use the CTID field, so using it is a little less likely
>> to conflict with something else we want to do in the future than using
>> a precious infomask bit.  However, I'm worried about this:
>>
>> /* Make sure there is no forward chain link in t_ctid */
>> tp.t_data->t_ctid = tp.t_self;
>>
>> The comment does not say *why* we need to make sure that there is no
>> forward chain link, but it implies that some code somewhere in the
>> system does or at one time did depend on no forward link existing.
>
> I think it is to ensure that EvalPlanQual mechanism gets invoked in
> the right case.   The visibility routine will return HeapTupleUpdated
> both when the tuple is deleted or updated (updated - has a newer
> version of the tuple), so we use ctid to decide if we need to follow
> the tuple chain for a newer version of the tuple.

That would explain why need to make sure that there *is* a forward
chain link in t_ctid for an update, but it doesn't explain why we need
to make sure that there *isn't* a forward link for delete.

> The proposed change in WARM tuple patch uses ip_posid field of CTID
> and we are planning to use ip_blkid field.  Here is the relevant text
> and code from WARM tuple patch:
>
> "Store the root line pointer of the WARM chain in the t_ctid.ip_posid
> field of the last tuple in the chain and mark the tuple header with
> HEAP_TUPLE_LATEST flag to record that fact."
>
> +#define HeapTupleHeaderSetHeapLatest(tup, offnum) \
> +do { \
> + AssertMacro(OffsetNumberIsValid(offnum)); \
> + (tup)->t_infomask2 |= HEAP_LATEST_TUPLE; \
> + ItemPointerSetOffsetNumber(&(tup)->t_ctid, (offnum)); \
> +} while (0)
>
> For further details, refer patch 0001-Track-root-line-pointer-v23_v26
> in the below e-mail:
> https://www.postgresql.org/message-id/CABOikdOTstHK2y0rDk%2BY3Wx9HRe%2BbZtj3zuYGU%3DVngneiHo5KQ%40mail.gmail.com

OK.

-- 
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] intermittent failures in Cygwin from select_parallel tests

[Tom Lane]

Robert Haas  writes:
> On Mon, Jun 5, 2017 at 10:40 AM, Andrew Dunstan
>  wrote:
>> Buildfarm member lorikeet is failing occasionally with a failed
>> assertion during the select_parallel regression tests like this:

> I don't *think* we've made any relevant code changes lately.  The only
> thing that I can see as looking at all relevant is
> b6dd1271281ce856ab774fc0b491a92878e3b501, but that doesn't really seem
> like it can be to blame.

Yeah, I don't believe that either.  That could have introduced a hard
failure (if something were relying on initializing a field before where
I put the memsets) but it's hard to see how it could produce an
intermittent and platform-specific one.

> One thought is that the only places where shm_mq_set_sender() should
> be getting invoked during the main regression tests are
> ParallelWorkerMain() and ExecParallelGetReceiver, and both of those
> places using ParallelWorkerNumber to figure out what address to pass.
> So if ParallelWorkerNumber were getting set to the same value in two
> different parallel workers - e.g. because the postmaster went nuts and
> launched two processes instead of only one - or if
> ParallelWorkerNumber were not getting initialized at all or were
> getting initialized to some completely bogus value, it could cause
> this symptom.

Hmm.  With some generous assumptions it'd be possible to think that
aa1351f1eec4adae39be59ce9a21410f9dd42118 triggered this.  That commit was
present in 20 successful lorikeet runs before the first of these failures,
which is a bit more than the MTBF after that, but not a huge amount more.

That commit in itself looks innocent enough, but could it have exposed
some latent bug in bgworker launching?

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] UPDATE of partition key

[Robert Haas]

On Fri, Jun 2, 2017 at 7:07 AM, Amit Khandekar  wrote:
> So, according to that, below would be the logic :
>
> Run partition constraint check on the original NEW row.
> If it succeeds :
> {
> Fire BR UPDATE trigger on the original partition.
> Run partition constraint check again with the modified NEW row
> (may be do this only if the trigger modified the partition key)
> If it fails,
> abort.
> Else
> proceed with the usual local update.
> }
> else
> {
> Fire BR UPDATE trigger on original partition.
> Find the right partition for the modified NEW row.
> If it is the same partition,
> proceed with the usual local update.
> else
> do the row movement.
> }

Sure, that sounds about right, although the "Fire BR UPDATE trigger on
the original partition." is the same in both branches, so I'm not
quite sure why you have that in the "if" block.

>> Actually, it seems like that's probably the
>> *easiest* behavior to implement.  Otherwise, you might fire triggers,
>> discover that you need to re-route the tuple, and then ... fire
>> triggers again on the new partition, which might reroute it again?
>
> Why would update BR trigger fire on the new partition ? On the new
> partition, only BR INSERT trigger would fire if at all we decide to
> fire delete+insert triggers. And insert trigger would not again cause
> the tuple to be re-routed because it's an insert.

OK, sure, that makes sense.  I guess it's really the insert case that
I was worried about -- if we have a BEFORE ROW INSERT trigger and it
changes the tuple and we reroute it, I think we'd have to fire the
BEFORE ROW INSERT on the new partition, which might change the tuple
again and cause yet another reroute, and in this worst case this is an
infinite loop.  But it sounds like we're going to fix that problem --
I think correctly -- by only ever allowing the tuple to be routed
once.  If some trigger tries to make a change the tuple after that
such that re-routing is required, they get an error.  And what you are
describing here seems like it will be fine.

> But now I think you are saying, the row that is being inserted into
> the new partition might get again modified by the INSERT trigger on
> the new partition, which might in turn cause it to fail the new
> partition constraint. But in that case, it will not cause another row
> movement, because in the new partition, it's an INSERT, not an UPDATE,
> so the operation would end there, aborted.

Yeah, that's what I was worried about.  I didn't want a row movement
to be able to trigger another row movement and so on ad infinitum.

-- 
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] Should we standardize on a type for signal handler flags?

[Robert Haas]

On Tue, Jun 6, 2017 at 1:33 PM, Tom Lane  wrote:
>> I think that's a pretty good argument, really.  If there exists a
>> platform where only sig_atomic_t is safe to read from a signal
>> handler, then we already don't work on that platform.  Even saving and
>> restoring errno isn't safe in that case.
>
> That's an argument from false premises.  The question here is what types
> are safe for an interrupt handler to *change*, not what can it read.

OK, but we certainly have code in signal handlers that does:

int save_errno = errno;
/* stuff */
errno = save_errno;

If that's not a signal handler changing an int, color me confused.

-- 
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: retry shm attach for windows (WAS: Re: [HACKERS] OK, so culicidae is *still* broken)

[Robert Haas]

On Tue, Jun 6, 2017 at 1:27 PM, Tom Lane  wrote:
> Robert Haas  writes:
>> On Tue, Jun 6, 2017 at 12:44 PM, Tom Lane  wrote:
>>> By definition, the address range we're trying to reuse worked successfully
>>> in the postmaster process.  I don't see how forcing a specific address
>>> could do anything but create an additional risk of postmaster startup
>>> failure.
>
>> If the postmaster picked an address where other things are unlikely to
>> get loaded, then that would increase the chances of child processes
>> finding it available, wouldn't it?
>
> But how would we know that a particular address range is more unlikely
> than others to have a conflict? (And even if we do know that, what
> happens when there is a conflict anyway?)  I sure don't want to be in
> the business of figuring out what to use across all the different Windows
> versions there are, to say nothing of the different antivirus products
> that might be causing the problem.

I'm not quite sure how to respond to this.  How do we know any piece
of information about anything, ever?  Sometimes we figure it out by
looking for relevant sources using, say, Google, and other times we
determine it by experiment or from first principles.  You and Andres
were having a discussion earlier about gathering this exact
information, so apparently you thought it might be possible back then:

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

Now, that having been said, I agree that no address range is perfectly
safe (and that's why it's good to have retries).  I also agree that
this is likely to be heavily platform-dependent, which is why I wrote
DSM the way that I did instead of (as Noah was advocating) trying to
solve the problem of getting a constant mapping across all processes
in a parallel group.  But since nobody's keen on the idea of trying to
tolerate having the main shared memory segment at different addresses
in different processes, we'll have to come up with some other solution
for that case.  If the retry thing doesn't plug the hole adequately,
trying to put the initial allocation in some place that's less likely
to induce conflicts seems like the next thing to try.

> Also, the big picture here is that we ought to be working towards allowing
> our Windows builds to use ASLR; our inability to support that is not
> something to be proud of in 2017.  No predetermined-address scheme is
> likely to be helpful for that.

Sure, retrying is better for that as well, as I already said upthread,
but that doesn't mean that putting the initial mapping someplace less
likely to conflict couldn't reduce the need for retries.

The even-bigger picture here is that both this issue and the need for
DSA and DSM are due to the fact that we've picked an unpopular
programming model with poor operating system support.   If we switch
from using processes to using threads, we don't have to deal with all
of this nonsense any more, and we'd solve some other problems, too -
e.g. at least on Windows, I think backend startup would get quite a
bit faster.  Obviously, anybody else who is using processes + shared
memory is going to run into the same problems we're hitting, and if
operating system manufacturers wanted to make this kind of programming
easy, they could do it.  We're expending a lot of effort here because
we're swimming against the current.

-- 
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] Parallel Aggregation support for aggregate functions that use transitions not implemented for array_agg

[Regina Obe]

It seems CREATE  AGGREGATE was expanded in 9.6 to support parallelization of
aggregate functions using transitions, with the addition of serialfunc and
deserialfunc to the aggregate definitions.

https://www.postgresql.org/docs/10/static/sql-createaggregate.html

I was looking at the PostgreSQL 10 source code for some example usages of
this and was hoping that array_agg and string_agg would support the feature.
At a cursory glance, it seems they do not use this.
Examples I see that do support it are the average and standard deviation
functions.

Is there a reason for this or it just wasn't gotten to?


Thanks,
Regina





-- 
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] Get stuck when dropping a subscription during synchronizing table

[Petr Jelinek]

On 03/06/17 18:53, Peter Eisentraut wrote:
> On 6/2/17 14:52, Peter Eisentraut wrote:
>> On 5/24/17 15:14, Petr Jelinek wrote:
>>> All the locking works just fine the way it is in master. The issue with
>>> deadlock with apply comes from the wrong handling of the SIGTERM in the
>>> apply (we didn't set InterruptPending). I changed the SIGTERM handler in
>>> patch 0001 just to die which is actually the correct behavior for apply
>>> workers. I also moved the connection cleanup code to the
>>> before_shmem_exit callback (similar to walreceiver) and now that part
>>> works correctly.
>>
>> I have committed this, in two separate parts.  This should fix the
>> originally reported issue.
>>
>> I will continue to work through your other patches.  I notice there is
>> still a bit of discussion about another patch, so please let me know if
>> there is anything else I should be looking for.
> 
> I have committed the remaining two patches.  I believe this fixes the
> originally reported issue.
> 

So the fact that we moved workers to standard interrupt handling broke
launcher in subtle ways because it still uses it's own SIGTERM handling
but some function it calls are using CHECK_FOR_INTERRUPTS (they are used
by worker as well). I think we need to move launcher to standard
interrupt handling as well. It's not same as other processes though as
it's allowed to be terminated any time (just like autovacuum launcher)
so we just proc_exit(0) instead of FATALing out.

This is related to the nightjar failures btw.

As a side note, we are starting to have several IsSomeTypeOfProcess
functions for these kind of things. I wonder if bgworker infrastructure
should somehow provide this type of stuff (the proposed bgw_type might
help there) as well as maybe being able to register interrupt handler
for the worker (it's really hard to do it via custom SIGTERM handler as
visible on worker, launcher and walsender issues we are fixing).
Obviously this is PG11 related thought.

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services
From 3fb7ed57d669beba3feba894a11c9dcff8e60414 Mon Sep 17 00:00:00 2001
From: Petr Jelinek 
Date: Tue, 6 Jun 2017 19:26:12 +0200
Subject: [PATCH] Use standard interrupt handling in logical replication
 launcher

---
 src/backend/replication/logical/launcher.c | 41 --
 src/backend/tcop/postgres.c|  9 +++
 src/include/replication/logicallauncher.h  |  2 ++
 3 files changed, 27 insertions(+), 25 deletions(-)

diff --git a/src/backend/replication/logical/launcher.c b/src/backend/replication/logical/launcher.c
index 5aaf24b..52169f1 100644
--- a/src/backend/replication/logical/launcher.c
+++ b/src/backend/replication/logical/launcher.c
@@ -81,7 +81,6 @@ static void logicalrep_worker_cleanup(LogicalRepWorker *worker);
 
 /* Flags set by signal handlers */
 static volatile sig_atomic_t got_SIGHUP = false;
-static volatile sig_atomic_t got_SIGTERM = false;
 
 static bool on_commit_launcher_wakeup = false;
 
@@ -623,20 +622,6 @@ logicalrep_worker_onexit(int code, Datum arg)
 	ApplyLauncherWakeup();
 }
 
-/* SIGTERM: set flag to exit at next convenient time */
-static void
-logicalrep_launcher_sigterm(SIGNAL_ARGS)
-{
-	int			save_errno = errno;
-
-	got_SIGTERM = true;
-
-	/* Waken anything waiting on the process latch */
-	SetLatch(MyLatch);
-
-	errno = save_errno;
-}
-
 /* SIGHUP: set flag to reload configuration at next convenient time */
 static void
 logicalrep_launcher_sighup(SIGNAL_ARGS)
@@ -798,13 +783,14 @@ ApplyLauncherMain(Datum main_arg)
 
 	before_shmem_exit(logicalrep_launcher_onexit, (Datum) 0);
 
+	Assert(LogicalRepCtx->launcher_pid == 0);
+	LogicalRepCtx->launcher_pid = MyProcPid;
+
 	/* Establish signal handlers. */
 	pqsignal(SIGHUP, logicalrep_launcher_sighup);
-	pqsignal(SIGTERM, logicalrep_launcher_sigterm);
+	pqsignal(SIGTERM, die);
 	BackgroundWorkerUnblockSignals();
 
-	LogicalRepCtx->launcher_pid = MyProcPid;
-
 	/*
 	 * Establish connection to nailed catalogs (we only ever access
 	 * pg_subscription).
@@ -812,7 +798,7 @@ ApplyLauncherMain(Datum main_arg)
 	BackgroundWorkerInitializeConnection(NULL, NULL);
 
 	/* Enter main loop */
-	while (!got_SIGTERM)
+	for (;;)
 	{
 		int			rc;
 		List	   *sublist;
@@ -822,6 +808,8 @@ ApplyLauncherMain(Datum main_arg)
 		TimestampTz now;
 		long		wait_time = DEFAULT_NAPTIME_PER_CYCLE;
 
+		CHECK_FOR_INTERRUPTS();
+
 		now = GetCurrentTimestamp();
 
 		/* Limit the start retry to once a wal_retrieve_retry_interval */
@@ -894,13 +882,16 @@ ApplyLauncherMain(Datum main_arg)
 		ResetLatch(>procLatch);
 	}
 
-	LogicalRepCtx->launcher_pid = 0;
-
-	/* ... and if it returns, we're done */
-	ereport(DEBUG1,
-			(errmsg("logical replication launcher shutting down")));
+	/* Not reachable */
+}
 
-	proc_exit(0);
+/*
+ * Is current process the logical replication launcher?
+ */
+bool
+IsLogicalLauncher(void)
+{
+	return 

Re: [HACKERS] Should we standardize on a type for signal handler flags?

[Tom Lane]

Robert Haas  writes:
> On Sun, Jun 4, 2017 at 7:21 PM, Andres Freund  wrote:
>> Well, we already have some variables that aren't actually booleans,
>> although I think all of them are only read not manipulated in signal
>> handlers (InterruptHoldoffCount etc).  So one could argue that there's
>> no safety benefit in sig_atomic_t, because we're already using in other
>> places.

> I think that's a pretty good argument, really.  If there exists a
> platform where only sig_atomic_t is safe to read from a signal
> handler, then we already don't work on that platform.  Even saving and
> restoring errno isn't safe in that case.

That's an argument from false premises.  The question here is what types
are safe for an interrupt handler to *change*, not what can it read.

Having said that, this is a good point:

>> We also already rely on int32 stores being atomic in other
>> parts of the code, although that's between processes not between signal
>> / normal path of execution.

> I don't think the issues are much different.

That would license us to use int32 communication variables, but it still
doesn't mean that "bool" is safe.

In practice, the sort of architecture where bool would be a problem is
one that doesn't have narrower-than-word-wide memory access instructions,
so that changing a char-sized variable involves loading a word,
manipulating a byte within the word, and storing it back.  I cut my
teeth on some machines like that, but I dunno if any still exist in
the wild.

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] Should we standardize on a type for signal handler flags?

[Andres Freund]

On 2017-06-06 13:07:57 -0400, Robert Haas wrote:
> > We also already rely on int32 stores being atomic in other
> > parts of the code, although that's between processes not between signal
> > / normal path of execution.
> 
> I don't think the issues are much different.  Presumably no CPU
> delivers a signal halfway through a CPU instruction, so if we can rely
> on a 4 byte store being indivisible from the perspective of some other
> CPU, it seems fine to also rely on that being true in the signal
> handler case.

The signal handler case is the "weaker" one I think - you'll only ever
see the result of an entire CPU instruction, whereas cross-cpu
concurrency can allow another cpu to see state from the *middle* of an
instruction if not atomic.

- 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: retry shm attach for windows (WAS: Re: [HACKERS] OK, so culicidae is *still* broken)

[Tom Lane]

Robert Haas  writes:
> On Tue, Jun 6, 2017 at 12:44 PM, Tom Lane  wrote:
>> By definition, the address range we're trying to reuse worked successfully
>> in the postmaster process.  I don't see how forcing a specific address
>> could do anything but create an additional risk of postmaster startup
>> failure.

> If the postmaster picked an address where other things are unlikely to
> get loaded, then that would increase the chances of child processes
> finding it available, wouldn't it?

But how would we know that a particular address range is more unlikely
than others to have a conflict?  (And even if we do know that, what
happens when there is a conflict anyway?)  I sure don't want to be in
the business of figuring out what to use across all the different Windows
versions there are, to say nothing of the different antivirus products
that might be causing the problem.

Also, the big picture here is that we ought to be working towards allowing
our Windows builds to use ASLR; our inability to support that is not
something to be proud of in 2017.  No predetermined-address scheme is
likely to be helpful for 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] inconsistent application_name use in logical workers

[Petr Jelinek]

On 06/06/17 15:07, Peter Eisentraut wrote:
> On 6/6/17 06:51, Petr Jelinek wrote:
>> On 06/06/17 04:19, Peter Eisentraut wrote:
>>> The logical replication code is supposed to use the subscription name as
>>> the fallback_application_name, but in some cases it uses the slot name,
>>> which could be different.  See attached patch to correct this.
>>
>> Hmm, well the differentiation has a reason though. The application_name
>> is used for sync rep and having synchronization connection using same
>> application_name might have adverse effects there because
>> synchronization connection can be in-front of main apply one, so sync
>> rep will think something is consumed while it's not.
> 
> True, we should use a different name for tablesync.c.  But the one in
> worker.c appears to be a mistake then?
> 

Yes.

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


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

Re: [HACKERS] sketchy partcollation handling

[Robert Haas]

On Tue, Jun 6, 2017 at 11:37 AM, Kevin Hale Boyes  wrote:
> On 6 June 2017 at 09:19, Robert Haas  wrote:
>> Thanks.  Committed.
>
> The changes to catalogs.sgml has introduced a double "the" in this part of
> the sentence "this contains the OID of the the collation".
> The other section already had the double "the".

Uggh!  It was just pointed out to me off-list that I credited the
wrong Kevin in the commit message for this fix.  My apologies.

-- 
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] Should we standardize on a type for signal handler flags?

[Robert Haas]

On Sun, Jun 4, 2017 at 7:21 PM, Andres Freund  wrote:
> Well, we already have some variables that aren't actually booleans,
> although I think all of them are only read not manipulated in signal
> handlers (InterruptHoldoffCount etc).  So one could argue that there's
> no safety benefit in sig_atomic_t, because we're already using in other
> places.

I think that's a pretty good argument, really.  If there exists a
platform where only sig_atomic_t is safe to read from a signal
handler, then we already don't work on that platform.  Even saving and
restoring errno isn't safe in that case.  And if no such platform
exists, then I don't know what the benefit is of worrying about
sig_atomic_t at all.  If "int" is anyway going to be "volatile int",
then why should "bool" be written "sig_atomic_t" rather than "volatile
bool"?

> We also already rely on int32 stores being atomic in other
> parts of the code, although that's between processes not between signal
> / normal path of execution.

I don't think the issues are much different.  Presumably no CPU
delivers a signal halfway through a CPU instruction, so if we can rely
on a 4 byte store being indivisible from the perspective of some other
CPU, it seems fine to also rely on that being true in the signal
handler case.

-- 
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: retry shm attach for windows (WAS: Re: [HACKERS] OK, so culicidae is *still* broken)

[Robert Haas]

On Tue, Jun 6, 2017 at 12:44 PM, Tom Lane  wrote:
> By definition, the address range we're trying to reuse worked successfully
> in the postmaster process.  I don't see how forcing a specific address
> could do anything but create an additional risk of postmaster startup
> failure.

If the postmaster picked an address where other things are unlikely to
get loaded, then that would increase the chances of child processes
finding it available, wouldn't 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] intermittent failures in Cygwin from select_parallel tests

[Robert Haas]

On Mon, Jun 5, 2017 at 10:40 AM, Andrew Dunstan
 wrote:
> Buildfarm member lorikeet is failing occasionally with a failed
> assertion during the select_parallel regression tests like this:
>
>
> 2017-06-03 05:12:37.382 EDT [59327d84.1160:38] LOG:  statement: select 
> count(*) from tenk1, tenk2 where tenk1.hundred > 1 and tenk2.thousand=0;
> TRAP: FailedAssertion("!(vmq->mq_sender == ((void *)0))", File: 
> "/home/andrew/bf64/root/HEAD/pgsql.build/../pgsql/src/backend/storage/ipc/shm_mq.c",
>  Line: 221)
>
> I'll see if I can find out why, but if anyone has any ideas why this might be 
> happening (started about 3 weeks ago) that would be helpful.

I don't *think* we've made any relevant code changes lately.  The only
thing that I can see as looking at all relevant is
b6dd1271281ce856ab774fc0b491a92878e3b501, but that doesn't really seem
like it can be to blame.

One thought is that the only places where shm_mq_set_sender() should
be getting invoked during the main regression tests are
ParallelWorkerMain() and ExecParallelGetReceiver, and both of those
places using ParallelWorkerNumber to figure out what address to pass.
So if ParallelWorkerNumber were getting set to the same value in two
different parallel workers - e.g. because the postmaster went nuts and
launched two processes instead of only one - or if
ParallelWorkerNumber were not getting initialized at all or were
getting initialized to some completely bogus value, it could cause
this symptom.

What ought to be happening, if there are N workers launched by a
parallel query, is that ParallelWorkerNumber should be different in
every worker, over the range 0 to N-1.  I think if I were you my first
step would be to verify that ParallelWorkerNumber is in that range in
the crashed backend, and if it is, my second step would be to add some
debugging code to ParallelWorkerMain() to print it out in every worker
that gets launched and make sure they're all in range and different.

All of the above might be going in the wrong direction entirely, but
it's the first thing that comes to mind for me.

-- 
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: retry shm attach for windows (WAS: Re: [HACKERS] OK, so culicidae is *still* broken)

[Tom Lane]

Robert Haas  writes:
> I think the idea of retrying process creation (and I definitely agree
> with Tom and Magnus that we have to retry process creation, not just
> individual mappings) is a good place to start.  Now if we find that we
> are having to retry frequently, then I think we might need to try
> something along the lines of what Andres proposed and what nginx
> apparently did.  However, any fixed address will be prone to
> occasional failures (or maybe, on some systems, regular failures) if
> that particular address happens to get claimed by something.  I don't
> think we can say that there is any address where that definitely won't
> happen.  So I would say let's do this retry thing first, and then if
> that proves inadequate, we can also try moving the mappings to a range
> where conflicts are less likely.

By definition, the address range we're trying to reuse worked successfully
in the postmaster process.  I don't see how forcing a specific address
could do anything but create an additional risk of postmaster startup
failure.

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] Tweaking tab completion for some backslash commands

[Masahiko Sawada]

On Sun, Jun 4, 2017 at 6:10 AM, Tom Lane  wrote:
> Masahiko Sawada  writes:
>> Attached patch tweaks tab completion for some backslash commands.
>
> Pushed, thanks!
>

Thank you!

Regards,

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


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

Re: retry shm attach for windows (WAS: Re: [HACKERS] OK, so culicidae is *still* broken)

[Robert Haas]

On Mon, Jun 5, 2017 at 10:10 PM, Amit Kapila  wrote:
> Agreed.  By the way, while browsing about this problem, I found that
> one other open source (nginx) has used a solution similar to what
> Andres was proposing upthread to solve this problem.  Refer:
> https://github.com/nginx/nginx/commit/2ec8cfcd7d34415a99c3f3db3024ae954c00d0dd
>
> Just to be clear, by above, I don't mean to say that if some other
> open source is using some solution, we should also use it, but I think
> it is worth considering (especially if it is a proven solution - just
> saying based on the time (2015) it has been committed and sustained in
> the code).

I think the idea of retrying process creation (and I definitely agree
with Tom and Magnus that we have to retry process creation, not just
individual mappings) is a good place to start.  Now if we find that we
are having to retry frequently, then I think we might need to try
something along the lines of what Andres proposed and what nginx
apparently did.  However, any fixed address will be prone to
occasional failures (or maybe, on some systems, regular failures) if
that particular address happens to get claimed by something.  I don't
think we can say that there is any address where that definitely won't
happen.  So I would say let's do this retry thing first, and then if
that proves inadequate, we can also try moving the mappings to a range
where conflicts are less likely.

-- 
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] Extra Vietnamese unaccent rules

[Bruce Momjian]

On Tue, Jun  6, 2017 at 12:15:13PM -0400, Tom Lane wrote:
> Bruce Momjian  writes:
> > There seems to be a problem.  I can't see a patch dated 2017-06-07 on
> > the commitfest page:
> > https://commitfest.postgresql.org/14/1161/
> 
> It looks to me like the patch is buried inside a multipart/alternative
> MIME section.  That's evidently causing our mail archives to miss its
> presence.  The latest message does show as having an attachment in the
> archives, but I think there's some delay before the CF app will notice.

OK, I see had picked up my email as the lastest, not the latest patch. 
I see now the second patch email appears properly on the webpage, so we
are good:

https://commitfest.postgresql.org/14/1161/

-- 
  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] sketchy partcollation handling

[Robert Haas]

On Tue, Jun 6, 2017 at 11:37 AM, Kevin Hale Boyes  wrote:
> On 6 June 2017 at 09:19, Robert Haas  wrote:
>> Thanks.  Committed.
>
> The changes to catalogs.sgml has introduced a double "the" in this part of
> the sentence "this contains the OID of the the collation".
> The other section already had the double "the".

Well, that's not a good thing for the
the patch to have done.

-- 
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: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

[Tom Lane]

Robert Haas  writes:
> I don't think it's true that we force the latest TLS version to be
> used.  The comment says:

> /*
>  * We use SSLv23_method() because it can negotiate use of the highest
>  * mutually supported protocol version, while alternatives like
>  * TLSv1_2_method() permit only one specific version.  Note
> that we don't
>  * actually allow SSL v2 or v3, only TLS protocols (see below).
>  */

> IIUC, this is specifically so that we don't force the use of TLS 1.2
> or TLS 1.1 or TLS 1.0.

Right.  IIUC, there's no way (at least in older OpenSSL versions) to say
directly "we only want TLS >= 1.0", so we have to do it like this.
I found a comment on the web saying "SSLv23_method would be better named
AutoNegotiate_method", which seems accurate.

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] BEFORE trigger can cause undetected partition constraint violation

[Robert Haas]

On Mon, Jun 5, 2017 at 1:02 AM, Amit Langote
 wrote:
> On 2017/06/03 1:56, Robert Haas wrote:
>> On Fri, Jun 2, 2017 at 12:51 AM, Amit Langote
>>  wrote:
>>> Attached patch makes InitResultRelInfo() *always* initialize the
>>> partition's constraint, that is, regardless of whether insert/copy is
>>> through the parent or directly on the partition.  I'm wondering if
>>> ExecInsert() and CopyFrom() should check if it actually needs to execute
>>> the constraint?  I mean it's needed if there exists BR insert triggers
>>> which may change the row, but not otherwise.  The patch currently does not
>>> implement that check.
>>
>> I think it should.  I mean, otherwise we're leaving a
>> probably-material amount of performance on the table.
>
> I agree.  Updated the patch to implement the check.

OK, so this isn't quite right.  Consider the following test case:

rhaas=# create table x (a int, b int, c int) partition by list (a);
CREATE TABLE
rhaas=# create table y partition of x for values in (1) partition by list (b);
CREATE TABLE
rhaas=# create table z partition of y for values in (1);
CREATE TABLE
rhaas=# insert into y values (2,1,1);
INSERT 0 1

The absence of the before-trigger is treated as evidence that z need
not revalidate the partition constraint, but actually it (or
something) still needs to enforce the part of it that originates from
y's ancestors.  In short, this patch would reintroduce the bug that
was fixed in 39162b2030fb0a35a6bb28dc636b5a71b8df8d1c, and would do so
by removing the exact same code that was added (by you!) in that
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] Re: [GSOC 17] Eliminate O(N^2) scaling from rw-conflict tracking in serializable transactions

[Mengxing Liu]

Hi, Kevin and Alvaro. 

I think disk I/O is not the bottleneck in our experiment, but the global lock 
is. 

For disk I/O, there are two evidences:
1) The total throughput is not more than 10 Ktps. Only a half are update 
transactions. An update transaction modifies 20 tuples; each tuple's size is 
about 100B.  
So the data written to the disk should be less than 10MB per second. Even we 
take write-ahead log in consideration (just double it), the data should be less 
than 20MB/s. 
I replaced ramdisk with SSD, and "iostat" shows the same result, while our 
SSD's sequential write speed is more than 700MB/s. 
2) I changed isolation level from "serializable" to "read committed". As the 
isolation requirement becomes looser, throughput is increased. But in this 
case, the CPU utilization is nearly 100%. (It's about 50% in serializable model)
Therefore, disk I/O is not the bottleneck.

For the lock:
I read the source code in predicate.c; I found many functions use a global 
lock:  SerializableXactHashLock. So there is only one process on working at any 
time!
As the problem of CPU not being fully used just happened after I had changed 
isolation level to "serailizable", this global lock should be the bottleneck.
Unfortunately, "perf" seems unable to record time waiting for locks.
I did it by hand.  Specifically, I use function "gettimeofday" just before 
acquiring locks and after releasing locks. 
In this way, I found function CheckForSerializableIn/CheckForSerializableOut 
takes more than 10% of running time, which is far bigger than what reported by 
perf in the last email.

If my statement is right, it sounds like good news as we find where the problem 
is.
Kevin has mentioned that the lock is used to protect the linked list. So I want 
to replace the linked list with hash table in the next step. After that I will 
try to remove this lock carefully.
But  in this way, our purpose has been changed. O(N^2) tracking is not the 
bottleneck, the global lock is.

By the way, using "gettimeofday" to profile is really ugly. 
Perf lock can only record kernel mutex, and requires kernel support, so I 
didn't use it.
Do you have any good idea about profiling time waiting for locks?


> -Original Messages-
> From: "Mengxing Liu" 
> Sent Time: 2017-06-05 00:27:51 (Monday)
> To: "Kevin Grittner" 
> Cc: "Alvaro Herrera" , 
> "pgsql-hackers@postgresql.org" 
> Subject: Re: Re: [HACKERS] Re: [GSOC 17] Eliminate O(N^2) scaling from 
> rw-conflict tracking in serializable transactions
> 
> 
> 
> 
> > -Original Messages-
> > From: "Kevin Grittner" 
> 
> > > I tried 30 cores. But the CPU utilization is about 45%~70%.
> > > How can we distinguish  where the problem is? Is disk I/O or Lock?
> > 
> > A simple way is to run `vmstat 1` for a bit during the test.  Can
> > you post a portion of the output of that here?  If you can configure
> > the WAL directory to a separate mount point (e.g., use the --waldir
> > option of initdb), a snippet of `iostat 1` output would be even
> > better.
> 
> "vmstat 1" output is as follow. Because I used only 30 cores (1/4 of all),  
> cpu user time should be about 12*4 = 48. 
> There seems to be no process blocked by IO. 
> 
> procs ---memory-- ---swap-- -io -system-- 
> --cpu-
>  r  b   swpd   free   buff  cache   si   sobibo   in   cs us sy id wa 
> st
> 28  0  0 981177024 315036 7084376000 0 900  1  0 
> 99  0  0
> 21  1  0 981178176 315036 7084378400 0 0 25482 329020 12  
> 3 85  0  0
> 18  1  0 981179200 315036 7084379200 0 0 26569 323596 12  
> 3 85  0  0
> 17  0  0 981175424 315036 7084380800 0 0 25374 322992 12  
> 4 85  0  0
> 12  0  0 981174208 315036 7084382400 0 0 24775 321577 12  
> 3 85  0  0
>  8  0  0 981179328 315036 7084533600 0 0 13115 199020  6  
> 2 92  0  0
> 13  0  0 981179200 315036 7084579200 0 0 22893 301373 11  
> 3 87  0  0
> 11  0  0 981179712 315036 7084580800 0 0 26933 325728 12  
> 4 84  0  0
> 30  0  0 981178304 315036 7084582400 0 0 23691 315821 11  
> 4 85  0  0
> 12  1  0 981177600 315036 7084583200 0 0 29485 320166 12  
> 4 84  0  0
> 32  0  0 981180032 315036 7084584800 0 0 25946 316724 12  
> 4 84  0  0
> 21  0  0 981176384 315036 7084586400 0 0 24227 321938 12  
> 4 84  0  0
> 21  0  0 981178880 315036 7084588000 0 0 25174 326943 13  
> 4 83  0  0
> 
> I used ramdisk to speedup the disk IO. Therefore, iostat can not give useful 
> information. 
> 
> > I think the best thing may be if you can generate a CPU flame graph
> > of the worst case you can make for these lists:
> > http://www.brendangregg.com/flamegraphs.html  

Re: [HACKERS] Extra Vietnamese unaccent rules

[Tom Lane]

Bruce Momjian  writes:
> There seems to be a problem.  I can't see a patch dated 2017-06-07 on
> the commitfest page:
>   https://commitfest.postgresql.org/14/1161/

It looks to me like the patch is buried inside a multipart/alternative
MIME section.  That's evidently causing our mail archives to miss its
presence.  The latest message does show as having an attachment in the
archives, but I think there's some delay before the CF app will notice.

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] Extra Vietnamese unaccent rules

[Bruce Momjian]

On Wed, Jun  7, 2017 at 01:06:22AM +0900, Man Trieu wrote:
> 2017-06-07 0:31 GMT+09:00 Bruce Momjian :
> I added the thread but there was no change.  (I think the thread was
> already present.)  It appears it is not seeing this patch as the latest
> patch.
> 
> Does anyone know why this is happening?
> 
> 
> 
> May be due to my Mac's mailer? Sorry but I try one more time to attach the
> patch by webmail.

It is getting weirder.  It has picked up my email report of a commitfest
problem as the latest patch (even though there was no patch), and your
second posting is not listed:

https://commitfest.postgresql.org/14/1161/

I think we need someone who knows the rules of how the commitfest finds
patches.

-- 
  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] sketchy partcollation handling

[Tom Lane]

Robert Haas  writes:
> On Sun, Jun 4, 2017 at 10:18 PM, Amit Langote
>  wrote:
>> BTW, the places which check whether the collation to store a dependency
>> for is the database default collation don't need to do that.  I mean the
>> following code block in all of these places:
>> 
>> /* The default collation is pinned, so don't bother recording it */
>> if (OidIsValid(attr->attcollation) &&
>> attr->attcollation != DEFAULT_COLLATION_OID)

> We could go change them all, but I guess I don't particularly see the point.

That's an intentional measure to save the catalog activity involved in
finding out that the default collation is pinned.  It's not *necessary*,
sure, but it's a useful and easy optimization.

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] Extra Vietnamese unaccent rules

[Man Trieu]

2017-06-07 0:31 GMT+09:00 Bruce Momjian :

> On Wed, Jun  7, 2017 at 12:10:25AM +0900, Dang Minh Huong wrote:
> > > On Jun 4, 29 Heisei, at 00:48, Bruce Momjian  wrote:
> >  Shouldn't you use "or is_letter_with_marks()", instead of "or
> len(...)
> > > 1"?  Your test might catch something that isn't based on a 'letter'
> >  (according to is_plain_letter).  Otherwise this looks pretty good to
> >  me.  Please add it to the next commitfest.
> > >>>
> > >>> Thanks for confirm, sir.
> > >>> I will add it to the next CF soon.
> > >>
> > >> Sorry for lately response. I attach the update patch.
> > >
> > > Uh, there is no patch attached.
> > >
> >
> > Sorry sir, reattach the patch.
> > I also added it to the next CF and set reviewers to Thomas Munro. Could
> you confirm for me.
>
> There seems to be a problem.  I can't see a patch dated 2017-06-07 on
> the commitfest page:
>
> https://commitfest.postgresql.org/14/1161/
>
> I added the thread but there was no change.  (I think the thread was
> already present.)  It appears it is not seeing this patch as the latest
> patch.
>
> Does anyone know why this is happening?
>


May be due to my Mac's mailer? Sorry but I try one more time to attach the
patch by webmail.

---
Thanks and best regards,
Dang Minh Huong


unaccent.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] sketchy partcollation handling

[Kevin Hale Boyes]

On 6 June 2017 at 09:19, Robert Haas  wrote:

>
> Thanks.  Committed.
>

The changes to catalogs.sgml has introduced a double "the" in this part of
the sentence "this contains the OID of the the collation".
The other section already had the double "the".

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

[Robert Haas]

On Tue, Jun 6, 2017 at 2:32 AM, Michael Paquier
 wrote:
>>> At the end,
>>> everything has been rejected as Postgres enforces the use of the
>>> newest one when doing the SSL handshake.
>>
>> TLS implementations, or TLS versions?  What does the TLS version have
>> to do with this issue?
>
> I really mean *version* here.

I don't think it's true that we force the latest TLS version to be
used.  The comment says:

/*
 * We use SSLv23_method() because it can negotiate use of the highest
 * mutually supported protocol version, while alternatives like
 * TLSv1_2_method() permit only one specific version.  Note
that we don't
 * actually allow SSL v2 or v3, only TLS protocols (see below).
 */

IIUC, this is specifically so that we don't force the use of TLS 1.2
or TLS 1.1 or TLS 1.0.

It could well be that there's something I don't understand here.

-- 
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] Extra Vietnamese unaccent rules

[Bruce Momjian]

On Wed, Jun  7, 2017 at 12:10:25AM +0900, Dang Minh Huong wrote:
> > On Jun 4, 29 Heisei, at 00:48, Bruce Momjian  wrote:
>  Shouldn't you use "or is_letter_with_marks()", instead of "or len(...)
> > 1"?  Your test might catch something that isn't based on a 'letter'
>  (according to is_plain_letter).  Otherwise this looks pretty good to
>  me.  Please add it to the next commitfest.
> >>> 
> >>> Thanks for confirm, sir.
> >>> I will add it to the next CF soon.
> >> 
> >> Sorry for lately response. I attach the update patch.
> > 
> > Uh, there is no patch attached.
> > 
> 
> Sorry sir, reattach the patch.
> I also added it to the next CF and set reviewers to Thomas Munro. Could you 
> confirm for me.

There seems to be a problem.  I can't see a patch dated 2017-06-07 on
the commitfest page:

https://commitfest.postgresql.org/14/1161/

I added the thread but there was no change.  (I think the thread was
already present.)  It appears it is not seeing this patch as the latest
patch.

Does anyone know why this is happening?

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