Re: [HACKERS] gen_random_uuid security not explicit in documentation

[Heikki Linnakangas]

On 30 June 2017 06:45:04 EEST, Noah Misch  wrote:
>This PostgreSQL 10 open item is past due for your status update. 
>Kindly send
>a status update within 24 hours, and include a date for your subsequent
>status
>update.

I'll fix this some time next week. (I'm on vacation right now)

- Heikki



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

Re: [HACKERS] Request more documentation for incompatibility of parallelism and plpgsql exec_run_select

[Mark Dilger]

> On Jun 29, 2017, at 9:14 PM, Amit Kapila  wrote:
> 
> On Fri, Jun 30, 2017 at 9:25 AM, Mark Dilger  wrote:
>> Hackers,
>> 
>> In src/pl/plpgsql/src/pl_exec.c: exec_run_select intentionally does not
>> allow a parallel plan if a portal will be returned.  This has the practical
>> consequence that a common coding practice (at least for me) of doing
>> something like:
>> 
>> create function myfunc(arg1 text, arg2 text) returns setof myfunctype as $$
>> declare
>>sql text;
>>result  myfunctype;
>> begin
>>-- unsafe interpolation, but this is just a code example
>>sql := 'select foo from bar where a = ' || arg1 || ' and b = ' || 
>> arg2;
>>for result in execute sql loop
>>return next result;
>>end loop;
>>return;
>> end;
>> $$ language plpgsql volatile;
>> 
>> can't run the generated 'sql' in parallel.  I think this is understandable, 
>> but
>> the documentation of this limitation in the sgml docs is thin.  Perhaps
>> someone who understands this limitation better than I do can document it?
>> 
> 
> This is explained in section 15.2 [1], refer below para:
> "The query might be suspended during execution. In any situation in
> which the system thinks that partial or incremental execution might
> occur, no parallel plan is generated. For example, a cursor created
> using DECLARE CURSOR will never use a parallel plan. Similarly, a
> PL/pgSQL loop of the form FOR x IN query LOOP .. END LOOP will never
> use a parallel plan, because the parallel query system is unable to
> verify that the code in the loop is safe to execute while parallel
> query is active."
> 
> Check if that clarifies your doubts, otherwise, we might need to write
> something more so that it can be easier for users to understand.

That's what I was looking for.  I think I had trouble finding it since I was
using plpgsql (notice no slash) and "parallel worker" and so forth to try
to find it.  "PL/pgSQL" and "parallel query" are not terms I use, and did
not notice them buried a few sentences down in this paragraph.

Thanks for the citation.

mark

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

Re: [HACKERS] Race conditions with WAL sender PID lookups

[Alvaro Herrera]

Noah Misch wrote:
> On Wed, Jun 28, 2017 at 06:52:14PM -0400, Alvaro Herrera wrote:
> > I think I'm done with the walsender half of this patch; I still need to
> > review the walreceiver part.  I will report back tomorrow 19:00 CLT.
> 
> This PostgreSQL 10 open item is past due for your status update.  Kindly send
> a status update within 24 hours, and include a date for your subsequent status
> update.  Refer to the policy on open item ownership:
> https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com

Real life took over.  I'll report before tomorrow same time.

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


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

Re: [HACKERS] Request more documentation for incompatibility of parallelism and plpgsql exec_run_select

[Amit Kapila]

On Fri, Jun 30, 2017 at 9:25 AM, Mark Dilger  wrote:
> Hackers,
>
> In src/pl/plpgsql/src/pl_exec.c: exec_run_select intentionally does not
> allow a parallel plan if a portal will be returned.  This has the practical
> consequence that a common coding practice (at least for me) of doing
> something like:
>
> create function myfunc(arg1 text, arg2 text) returns setof myfunctype as $$
> declare
> sql text;
> result  myfunctype;
> begin
> -- unsafe interpolation, but this is just a code example
> sql := 'select foo from bar where a = ' || arg1 || ' and b = ' || 
> arg2;
> for result in execute sql loop
> return next result;
> end loop;
> return;
> end;
> $$ language plpgsql volatile;
>
> can't run the generated 'sql' in parallel.  I think this is understandable, 
> but
> the documentation of this limitation in the sgml docs is thin.  Perhaps
> someone who understands this limitation better than I do can document it?
>

This is explained in section 15.2 [1], refer below para:
"The query might be suspended during execution. In any situation in
which the system thinks that partial or incremental execution might
occur, no parallel plan is generated. For example, a cursor created
using DECLARE CURSOR will never use a parallel plan. Similarly, a
PL/pgSQL loop of the form FOR x IN query LOOP .. END LOOP will never
use a parallel plan, because the parallel query system is unable to
verify that the code in the loop is safe to execute while parallel
query is active."

Check if that clarifies your doubts, otherwise, we might need to write
something more so that it can be easier for users to understand.

[1] - 
https://www.postgresql.org/docs/devel/static/when-can-parallel-query-be-used.html



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


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

[HACKERS] Request more documentation for incompatibility of parallelism and plpgsql exec_run_select

[Mark Dilger]

Hackers,

In src/pl/plpgsql/src/pl_exec.c: exec_run_select intentionally does not
allow a parallel plan if a portal will be returned.  This has the practical
consequence that a common coding practice (at least for me) of doing
something like:

create function myfunc(arg1 text, arg2 text) returns setof myfunctype as $$
declare
sql text;
result  myfunctype; 
begin
-- unsafe interpolation, but this is just a code example
sql := 'select foo from bar where a = ' || arg1 || ' and b = ' || arg2;
for result in execute sql loop
return next result;
end loop;
return;
end;
$$ language plpgsql volatile;

can't run the generated 'sql' in parallel.  I think this is understandable, but
the documentation of this limitation in the sgml docs is thin.  Perhaps
someone who understands this limitation better than I do can document it?
Changing myfunc to create a temporary table, to execute the sql to populate
that temporary table, and to then loop through the temporary table's rows
fixes the problem.  For the real-world example where I hit this, that single
change decreases the runtime from 13.5 seconds to 2.5 seconds.  This
makes sense to me, because doing it that way means pl_exec knows that
it won't be returning a portal for the executed sql, so the parallel plan is
still allowed.

Sorry to be a nag.  I'm only trying to help the next person who might
otherwise trip over this limitation.  Perhaps this belongs in section 15.3.4
or 42.5.4.

mark

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

Re: [HACKERS] Code quality issues in ICU patch

[Noah Misch]

On Sun, Jun 25, 2017 at 09:28:51PM -0700, Noah Misch wrote:
> On Sat, Jun 24, 2017 at 10:03:25AM -0400, Peter Eisentraut wrote:
> > On 6/23/17 12:31, Tom Lane wrote:
> > > icu_to_uchar() and icu_from_uchar(), and perhaps other places, are
> > > touchingly naive about integer overflow hazards in buffer size
> > > calculations.  I call particular attention to this bit in
> > > icu_from_uchar():
> > > 
> > >   len_result = UCNV_GET_MAX_BYTES_FOR_STRING(len_uchar, 
> > > ucnv_getMaxCharSize(icu_converter));
> > > 
> > > The ICU man pages say that that macro is defined as
> > > 
> > > #define UCNV_GET_MAX_BYTES_FOR_STRING(length, maxCharSize)
> > > (((int32_t)(length)+10)*(int32_t)(maxCharSize))
> > > 
> > > which means that getting this to overflow (resulting in
> > > probably-exploitable memory overruns) would be about as hard as taking
> > > candy from a baby.
> > 
> > Here is a patch that should address this.
> 
> [Action required within three days.  This is a generic notification.]
> 
> The above-described topic is currently a PostgreSQL 10 open item.  Peter,
> since you committed the patch believed to have created it, you own this open
> item.  If some other commit is more relevant or if this does not belong as a
> v10 open item, please let us know.  Otherwise, please observe the policy on
> open item ownership[1] and send a status update within three calendar days of
> this message.  Include a date for your subsequent status update.  Testers may
> discover new open items at any time, and I want to plan to get them all fixed
> well in advance of shipping v10.  Consequently, I will appreciate your efforts
> toward speedy resolution.  Thanks.
> 
> [1] 
> https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com

This PostgreSQL 10 open item is past due for your status update.  Kindly send
a status update within 24 hours, and include a date for your subsequent status
update.  Refer to the policy on open item ownership:
https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com


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

Re: [HACKERS] gen_random_uuid security not explicit in documentation

[Noah Misch]

On Sun, Jun 25, 2017 at 09:26:28PM -0700, Noah Misch wrote:
> On Fri, Jun 23, 2017 at 10:23:36AM +0900, Michael Paquier wrote:
> > On Fri, Jun 23, 2017 at 3:02 AM, Heikki Linnakangas  wrote:
> > > I'm inclined to change gen_random_uuid() to throw an error if the server 
> > > is
> > > built with --disable-strong-random, like gen_random_bytes() does. That 
> > > way,
> > > they would behave the same.
> > 
> > No objections to do that. I guess you don't need a patch. As this is
> > new to 10, I have added an open item.
> 
> [Action required within three days.  This is a generic notification.]
> 
> The above-described topic is currently a PostgreSQL 10 open item.  Heikki,
> since you committed the patch believed to have created it, you own this open
> item.  If some other commit is more relevant or if this does not belong as a
> v10 open item, please let us know.  Otherwise, please observe the policy on
> open item ownership[1] and send a status update within three calendar days of
> this message.  Include a date for your subsequent status update.  Testers may
> discover new open items at any time, and I want to plan to get them all fixed
> well in advance of shipping v10.  Consequently, I will appreciate your efforts
> toward speedy resolution.  Thanks.
> 
> [1] 
> https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com

This PostgreSQL 10 open item is past due for your status update.  Kindly send
a status update within 24 hours, and include a date for your subsequent status
update.  Refer to the policy on open item ownership:
https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com


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

Re: [HACKERS] Race conditions with WAL sender PID lookups

[Noah Misch]

On Wed, Jun 28, 2017 at 06:52:14PM -0400, Alvaro Herrera wrote:
> I think I'm done with the walsender half of this patch; I still need to
> review the walreceiver part.  I will report back tomorrow 19:00 CLT.

This PostgreSQL 10 open item is past due for your status update.  Kindly send
a status update within 24 hours, and include a date for your subsequent status
update.  Refer to the policy on open item ownership:
https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com


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

Re: [HACKERS] Get stuck when dropping a subscription during synchronizing table

[Noah Misch]

On Wed, Jun 28, 2017 at 03:22:18AM +, Noah Misch wrote:
> On Fri, Jun 23, 2017 at 09:42:10PM -0400, Peter Eisentraut wrote:
> > On 6/21/17 22:47, Peter Eisentraut wrote:
> > > On 6/20/17 22:44, Noah Misch wrote:
> > >>> A patch has been posted, and it's being reviewed.  Next update Monday.
> > >>
> > >> This PostgreSQL 10 open item is past due for your status update.  Kindly 
> > >> send
> > >> a status update within 24 hours, and include a date for your subsequent 
> > >> status
> > >> update.  Refer to the policy on open item ownership:
> > >> https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com
> > > 
> > > I'm not sure how to proceed here.  Nobody is else seems terribly
> > > interested, and this is really a minor issue, so I don't want to muck
> > > around with the locking endlessly.  Let's say, if there are no new
> > > insights by Friday, I'll pick one of the proposed patches or just close
> > > it without any patch.
> > 
> > After some comments, a new patch has been posted, and I'll give until
> > Monday for review.
> 
> This PostgreSQL 10 open item is past due for your status update.  Kindly send
> a status update within 24 hours, and include a date for your subsequent status
> update.  Refer to the policy on open item ownership:
> https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com

IMMEDIATE ATTENTION REQUIRED.  This PostgreSQL 10 open item is long past due
for your status update.  Please reacquaint yourself with the policy on open
item ownership[1] and then reply immediately.  If I do not hear from you by
2017-07-01 04:00 UTC, I will transfer this item to release management team
ownership without further notice.

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


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

Re: [HACKERS] Causal reads take II

[Thomas Munro]

On Tue, Jun 27, 2017 at 12:20 PM, Thomas Munro
 wrote:
> On Sun, Jun 25, 2017 at 2:36 AM, Simon Riggs  wrote:
>> What I think we need is a joined up plan for load balancing, so that
>> we can understand how it will work. i.e. explain the whole use case
>> and how the solution works.

Here's a proof-of-concept hack of the sort of routing and retry logic
that I think should be feasible with various modern application stacks
(given the right extensions):

https://github.com/macdice/py-pgsync/blob/master/DemoSyncPool.py

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


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

Re: protocol version negotiation (Re: [HACKERS] Libpq PGRES_COPY_BOTH - version compatibility)

[Craig Ringer]

On 29 June 2017 at 20:18, Robert Haas  wrote:

> I'm not sure if non-critical is exactly the right terminology.  What
> we want to do is distinguish between things that are intended as
> protocol-level options vs. things that are intended as GUCs.

We probably also need to be able to differentiate between "this
protocol option would be nice if you support it, please tell me in
your parameter status message if you managed to honour it" vs "this
connection request is invalid if you cannot honour this protocol
option, do not attempt to proceed".

If the option changes the meaning of the request entirely in some way
we probably don't want a server that doesn't understand carrying on
anyway.

But I think it's probably sufficient if the server's reply to the
client includes the options it was able to recognise and honour, so
the client can immediately nope out if it doesn't like the server's
lack of support for some option.

> Maybe we should instead pick a GUC namespace and reserve it for the
> use of protocol level options; e.g. pg_protocol. becomes
> invalid as the name of a GUC, but can be included in a startup packet
> (or do we need to pick something shorter, like _pg, to keep the
> message short?).  Servers ignore anything that they don't understand.
> So:

Yes, but something shorter probably, Tom's already expressed concerns
elsewhere about bandwidth use from reporting server_version_num and I
can't imagine numerous pg_protocol_blahblah entries being popular with
many people at all.

_pg_... seems sufficient IMO, we don't use that in GUCs to date, and
nobody's likely to.

Or to be consistent with protocol and libpq stuff, _pq_... ?

> 1. The client sends a StartupMessage 3.x for version 3.x.  We could
> bump the version explicitly, or perhaps we should just coin a version
> of libpq for every server release; e.g. whatever PostgreSQL 11 ships
> is version 3.11, etc.  It includes any protocol options that don't
> exist today as pg_protocol. in the startup packet.

Not a fan of bumping for the sake of it, it puts more work on 3rd
party drivers for what seems like no real gain.

Bump when we change something.

> 2. If the client version is anything other than 3.0, the server
> responds with a ServerProtocolVersion indicating the highest version
> it supports

Highest and lowest, I think Satyanarayana Narlapuram is right there.
Also which startup parameter protocol options were recognised and
honoured, so the client can immediately notice and bail if the server
didn't recognise something it requires. (think behaviour akin to
sslmode=require for _pg_starttls).

> and ignores any pg_protocol. options not known
> to it as being either third-party extensions or something from a
> future version.

I think ignoring is fine if the server lists the ones it recognises in
its reply, per above.

> If the initial response to the startup message is
> anything other than a ServerProtocolVersion message, the client should
> assume it's talking to a 3.0 server.  (To make this work, we would
> back-patch a change into existing releases to allow any 3.x protocol
> version and ignore any pg_protocol. options that were
> specified.)

Such a backpatch makes sense, and is pretty safe.

-- 
 Craig Ringer   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] Apparent walsender bug triggered by logical replication

[Tom Lane]

Petr Jelinek  writes:
> On 30/06/17 02:07, Tom Lane wrote:
>> I'm also kind of wondering why the "behind the apply" path out of
>> LogicalRepSyncTableStart exists at all; as far as I can tell we'd be much
>> better off if we just let the sync worker exit always as soon as it's done
>> the initial sync, letting any extra catchup happen later.  The main thing
>> the current behavior seems to be accomplishing is to monopolize one of the
>> scarce max_sync_workers_per_subscription slots for the benefit of a single
>> table, for longer than necessary.  Plus it adds additional complicated
>> interprocess signaling.

> Hmm, I don't understand what you mean here. The "letting any extra
> catchup happen later" would never happen if the sync is behind apply as
> apply has already skipped relevant transactions.

Once the sync worker has exited, we have to have some other way of dealing
with that.  I'm wondering why we can't let that other way take over
immediately.  The existing approach is inefficient, according to the
traces I've been poring over all day, and frankly I am very far from
convinced that it's bug-free either.

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] Apparent walsender bug triggered by logical replication

[Petr Jelinek]

On 30/06/17 02:07, Tom Lane wrote:
> I'm also kind of wondering why the "behind the apply" path out of
> LogicalRepSyncTableStart exists at all; as far as I can tell we'd be much
> better off if we just let the sync worker exit always as soon as it's done
> the initial sync, letting any extra catchup happen later.  The main thing
> the current behavior seems to be accomplishing is to monopolize one of the
> scarce max_sync_workers_per_subscription slots for the benefit of a single
> table, for longer than necessary.  Plus it adds additional complicated
> interprocess signaling.
> 

Hmm, I don't understand what you mean here. The "letting any extra
catchup happen later" would never happen if the sync is behind apply as
apply has already skipped relevant transactions.

-- 
  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] Broken hint bits (freeze)

[Bruce Momjian]

On Wed, Jun 28, 2017 at 10:11:35PM -0400, Bruce Momjian wrote:
> On Fri, Jun 23, 2017 at 06:17:47PM +0300, Sergey Burladyan wrote:
> > PS:
> > I successfully upgraded last night from 9.2 to 9.4 and find other issue :-)
> > 
> > It is about hash index and promote:
> > 1. create master
> > 2. create standby from it
> > 3. create unlogged table and hash index like:
> >  create unlogged table test (id int primary key, v text);
> >  create index on test using hash (id);
> > 3. stop master
> > 4. promote standby
> > 
> > now, if you try to upgrade this new promoted master pg_upgrade will stop
> > on this hash index:
> > error while creating link for relation "public.test_id_idx" 
> > ("s/9.2/base/16384/16393" to "m/9.4/base/16422/16393"): No such file or 
> > directory
> > Failure, exiting
> > 
> > I touch this file (s/9.2/base/16384/16393) and rerun pg_upgrade from
> > scratch and it complete successfully.
> 
> Sergey, can you please test if the table "test" is not unlogged, does
> pg_upgrade still fail on the hash index file?

I was able to reproduce this failure on my server.  :-)

What I found is that the problem is larger than I thought.  Sergey is
correct that pg_upgrade fails because there is no hash file associated
with the unlogged table, but in fact a simple access of the unlogged
table with a hash index generates an error:

test=> SELECT * FROM t_u_hash;
ERROR:  could not open file "base/16384/16392": No such file or 
directory

What is interesting is that this is the only combination that generates
an error.  A unlogged able with a btree index or a logged table with a
hash index are fine, e.g.:

   List of relations
 Schema |   Name| Type  |  Owner
+---+---+--
 public | t_btree   | table | postgres
 public | t_hash| table | postgres
 public | t_u_btree | table | postgres
fail-->  public | t_u_hash  | table | postgres

This doesn't fail on PG 10 since we WAL-log hash indexes.

I think we have two questions:

1.  do we fix this in the server
2.  if not, do we fix pg_upgrade

-- 
  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] Apparent walsender bug triggered by logical replication

[Andres Freund]

Hi,

On 2017-06-29 20:07:14 -0400, Tom Lane wrote:
> I was able to make the hang go away by means of the attached patch that
> allows WalSndWaitForWal to exit early if the client has shut down the
> COPY.  However, since that function is miserably underdocumented (like
> most of this code :-(), I have little idea if this is correct or safe.

Seems reasonable to me.


> I also wonder why WalSndWaitForWal is being called for WAL that seemingly
> doesn't exist yet, and whether that doesn't indicate another bug somewhere
> in this stack.

That's pretty normal - we can only send back something once a
transaction is complete, and until that happens we'll just block waiting
for more WAL.

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

[HACKERS] Apparent walsender bug triggered by logical replication

[Tom Lane]

I've been poking into the src/test/subscription TAP tests, thinking
that they seem a lot slower than they ought to be.  The first thing
I came across was this bit in WaitForReplicationWorkerAttach():

/*
 * We need timeout because we generally don't get notified via latch
 * about the worker attach.
 */
rc = WaitLatch(MyLatch,
   WL_LATCH_SET | WL_TIMEOUT | WL_POSTMASTER_DEATH,
   1000L, WAIT_EVENT_BGWORKER_STARTUP);

Tracing showed that the worker is generally done attaching within two
or three milliseconds of the request, making the 1000ms delay in this
loop a rather serious killer of startup performance.  I think the right
way to fix that is to arrange to have a signal sent back from the worker;
but just to confirm that this *is* a bottleneck, I tried cutting the
timeout way back.  Setting it to 10ms indeed made the 001 and 002
subscription tests a good deal faster --- most of the time.  Every second
or third run or so, either the 001 or 002 test would take about 20sec
longer than it ought to.  When I set it to 5ms, the 001 test reliably took
ten times longer than normal, and the 002 test failed completely.

I've spent quite a bit of time tracing down why, but what seems
to be happening is that elimination of the excess delay in 
WaitForReplicationWorkerAttach allows this if-test in
LogicalRepSyncTableStart:

/*--
 * There are now two possible states here:
 * a) Sync is behind the apply.  If that's the case we need to
 *  catch up with it by consuming the logical replication
 *  stream up to the relstate_lsn.  For that, we exit this
 *  function and continue in ApplyWorkerMain().
 * b) Sync is caught up with the apply.  So it can just set
 *  the state to SYNCDONE and finish.
 *--
 */
if (*origin_startpos >= MyLogicalRepWorker->relstate_lsn)

to sometimes fail, whereas it basically never does with the delay, because
the launcher never gets there fast enough to advance relstate_lsn past
what the synchronization worker had set before the worker gets here and
decides it can exit.  That means we do what the comment calls (a), that
is, return to ApplyWorkerMain, start up streaming replication, and enter
LogicalRepApplyLoop.  But, almost immediately, we reach the
synchronization point and decide to shut down again.  What happens then is
that the partner walsender gets hung up for tens of seconds, and until it
sends back the "COPY 0" CommandComplete message, the sync worker can't
finish and exit.

What seems to be the reason for the hang is that WalSndWaitForWal() is
being called to ask for WAL that doesn't exist yet, and unlike the other
loops in walsender.c, it contains no provision for exiting early when the
client indicates it's lost interest.  So we just sit there until something
happens on the master server.  Although this situation might not last very
long on production servers, it still seems like a serious bug.

I was able to make the hang go away by means of the attached patch that
allows WalSndWaitForWal to exit early if the client has shut down the
COPY.  However, since that function is miserably underdocumented (like
most of this code :-(), I have little idea if this is correct or safe.

I also wonder why WalSndWaitForWal is being called for WAL that seemingly
doesn't exist yet, and whether that doesn't indicate another bug somewhere
in this stack.

I'm also kind of wondering why the "behind the apply" path out of
LogicalRepSyncTableStart exists at all; as far as I can tell we'd be much
better off if we just let the sync worker exit always as soon as it's done
the initial sync, letting any extra catchup happen later.  The main thing
the current behavior seems to be accomplishing is to monopolize one of the
scarce max_sync_workers_per_subscription slots for the benefit of a single
table, for longer than necessary.  Plus it adds additional complicated
interprocess signaling.

regards, tom lane

diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
index f845180..2611d62 100644
*** a/src/backend/replication/walsender.c
--- b/src/backend/replication/walsender.c
*** WalSndWaitForWal(XLogRecPtr loc)
*** 1374,1379 
--- 1374,1387 
  		if (pq_flush_if_writable() != 0)
  			WalSndShutdown();
  
+ 		/*
+ 		 * If we have received CopyDone from the client, sent CopyDone
+ 		 * ourselves, and the output buffer is empty, it's time to exit
+ 		 * streaming, so fail the current WAL fetch request.
+ 		 */
+ 		if (!pq_is_send_pending() && streamingDoneSending && streamingDoneReceiving)
+ 			break;
+ 
  		now = GetCurrentTimestamp();
  
  		/* die if timeout was reached */


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make 

Re: protocol version negotiation (Re: [HACKERS] Libpq PGRES_COPY_BOTH - version compatibility)

[Satyanarayana Narlapuram]

-Original Message-
From: pgsql-hackers-ow...@postgresql.org 
[mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Robert Haas
Sent: Thursday, June 29, 2017 5:18 AM
To: Tom Lane 
Cc: Craig Ringer ; Peter Eisentraut ; 
Magnus Hagander ; PostgreSQL-development 

Subject: Re: protocol version negotiation (Re: [HACKERS] Libpq PGRES_COPY_BOTH 
- version compatibility)

> 1. The client sends a StartupMessage 3.x for version 3.x.  We could bump the 
> version explicitly, or perhaps we should just coin a version of libpq for 
> every server release; e.g. whatever PostgreSQL 11 ships is version 3.11, etc. 
>  It includes any protocol options that don't exist today as > 
> pg_protocol. in the startup packet.

+1 on this. Happy to read this conversation. I am hopeful that this provides us 
a path to include parameters needed for Azure database for PostgreSQL service 
(host name, and connection id in the startupmessage). For someone wondering 
what they are, please see the threads below.

https://www.postgresql.org/message-id/DM2PR03MB416343FC02D6E977FEF2EB191C00%40DM2PR03MB416.namprd03.prod.outlook.com
https://www.postgresql.org/message-id/DM2PR03MB4168F3C796B2965FDC4CF9991C00%40DM2PR03MB416.namprd03.prod.outlook.com

2. If the client version is anything other than 3.0, the server responds with a 
ServerProtocolVersion indicating the highest version it supports, and ignores 
any pg_protocol. options not known to it as being either third-party 
extensions or something from a future version.  If the initial response to the 
startup message is anything other than a ServerProtocolVersion message, the 
client should assume it's talking to a 3.0 server.  (To make this work, we 
would back-patch a change into existing releases to allow any 3.x protocol 
version and ignore any pg_protocol. options that were specified.)

> If the client version is anything other than 3.0, the server responds with a 
> ServerProtocolVersion indicating the highest version it supports, and ignores 
> any pg_protocol. options not known to it as being either 
> third-party extensions or something from a future version.  If > the initial 
> response to the startup message is anything other than a 
> ServerProtocolVersion message, the client should assume it's talking to a 3.0 
> server.  (To make this work, we would back-patch a change into existing 
> releases to allow any 3.x protocol version and ignore any > 
> pg_protocol. options that were specified.)

We can avoid one round trip if the server accepts the startupmessage as is 
(including understanding all the parameters supplied by the client), and in the 
cases where server couldn’t accept the startupmessage / require negotiation, it 
should send ServerProtocolVersion message that contains both MIN and MAX 
versions it can support. Providing Min version helps server enforce the client 
Min protocol version, and provides a path to deprecate older versions. Thoughts?


> If either the client or the server is unhappy about the age of the other, 
> then it can disconnect; e.g. if the server is configured to require the use 
> of whizzbang-2 security, and the client protocol version indicates that at 
> most whizzbang-1.9 is available, then the server can close the   > 
> connection with a suitable complaint; conversely, if the connection string 
> had require_whizzbang=2, and the server is too old to support that, then the 
> client can decide to bail out when it receives the ServerProtocolVersion 
> message.

Does the proposal also include the client can negotiate the protocol version on 
the same connection rather than going through connection setup process again? 
The state machine may not sound simple with this proposal but helps bringing 
down total time taken for the login.
Client / server can disconnect any time they think the negotiation failed.


Thanks,
Satya

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

Re: [HACKERS] AdvanceXLInsertBuffer vs. WAL segment compressibility

[Chapman Flack]

On 06/25/17 21:20, Chapman Flack wrote:
> I want to make sure I understand what the deadlock potential is
> in this case. AdvanceXLInsertBuffer will call WaitXLogInsertionsToFinish
> ...
> Does not the fact we hold all the insertion slots exclude the possibility
> that any dirty buffer (preceding the one we're touching) needs to be checked
> for in-flight insertions? [in the filling-out-the-log-tail case only]

Anyone?

Or have I not even achieved 'wrong' yet?

-Chap


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

[Amit Khandekar]

On 22 June 2017 at 01:41, Robert Haas  wrote:
>>> +for (i = 0; i < num_rels; i++)
>>> +{
>>> +ResultRelInfo *resultRelInfo = _rels[i];
>>> +Relationrel = resultRelInfo->ri_RelationDesc;
>>> +Bitmapset *expr_attrs = NULL;
>>> +
>>> +pull_varattnos((Node *) rel->rd_partcheck, 1, _attrs);
>>> +
>>> +/* Both bitmaps are offset by FirstLowInvalidHeapAttributeNumber. 
>>> */
>>> +if (bms_overlap(expr_attrs, GetUpdatedColumns(resultRelInfo, 
>>> estate)))
>>> +return true;
>>> +}
>>>
>>> This seems like an awfully expensive way of performing this test.
>>> Under what circumstances could this be true for some result relations
>>> and false for others;
>>
>> One resultRelinfo can have no partition key column used in its quals,
>> but the next resultRelinfo can have quite different quals, and these
>> quals can have partition key referred. This is possible if the two of
>> them have different parents that have different partition-key columns.
>
> Hmm, true.  So if we have a table foo that is partitioned by list (a),
> and one of its children is a table bar that is partitioned by list
> (b), then we need to consider doing tuple-routing if either column a
> is modified, or if column b is modified for a partition which is a
> descendant of bar.  But visiting that only requires looking at the
> partitioned table and those children that are also partitioned, not
> all of the leaf partitions as the patch does.

The main concern is that the non-leaf partitions are not open (except
root), so we would need to open them in order to get the partition key
of the parents of update resultrels (or get only the partition key
atts and exprs from pg_partitioned_table).

There can be multiple approaches to finding partition key columns.

Approach 1 : When there are a few update result rels and a large
partition tree, we traverse from each of the result rels to their
ancestors , and open their ancestors (get_partition_parent()) to get
the partition key columns. For result rels having common parents, do
this only once.

Approach 2 : If there are only a few partitioned tables, and large
number of update result rels, it would be easier to just open all the
partitioned tables and form the partition key column bitmap out of all
their partition keys. If the bitmap does not have updated columns,
that's not a partition-key-update. So for typical non-partition-key
updates, just opening the partitioned tables will suffice, and so that
would not affect performance of normal updates.

But if the bitmap has updated columns, we can't conclude that it's a
partition-key-update, otherwise it would be false positive. We again
need to further check whether the update result rels belong to
ancestors that have updated partition keys.

Approach 3 : In RelationData, in a new bitmap field (rd_partcheckattrs
?), store partition key attrs that are used in rd_partcheck . Populate
this field during generate_partition_qual().

So to conclude, I think, we can do this :

Scenario 1 :
Only one partitioned table : the root; rest all are leaf partitions.
In this case, it is definitely efficient to just check the root
partition key, which will be sufficient.

Scenario 2 :
There are few non-leaf partitioned tables (3-4) :
Open those tables, and follow 2nd approach above: If we don't find any
updated partition-keys in any of them, well and good. If we do find,
failover to approach 3 : For each of the update resultrels, use the
new rd_partcheckattrs bitmap to know if it uses any of the updated
columns. This would be faster than pulling up attrs from the quals
like how it was done in the patch.

-- 
Thanks,
-Amit Khandekar
EnterpriseDB Corporation
The Postgres Database 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] Reducing pg_ctl's reaction time

[Jeff Janes]

On Thu, Jun 29, 2017 at 11:39 AM, Tom Lane  wrote:

> Jeff Janes  writes:
> > In the now-committed version of this, the 'pg_ctl start' returns
> > successfully as soon as the server reaches a consistent state. Which is
> OK,
> > except that it does the same thing when hot_standby=off.  When
> > hot_standby=off, I would expect it to wait for the end of recovery before
> > exiting with a success code.
>
> Um, won't it be waiting forever with that definition?
>
> regards, tom lane
>

No, this isn't streaming.  It hits the PITR limit (recovery_target_*), or
runs out of archived wal, and then it opens for business.


Cheers,

Jeff

Re: [HACKERS] Reducing pg_ctl's reaction time

[Tom Lane]

Jeff Janes  writes:
> In the now-committed version of this, the 'pg_ctl start' returns
> successfully as soon as the server reaches a consistent state. Which is OK,
> except that it does the same thing when hot_standby=off.  When
> hot_standby=off, I would expect it to wait for the end of recovery before
> exiting with a success code.

Um, won't it be waiting forever with that definition?

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] Reducing pg_ctl's reaction time

[Andres Freund]

On June 29, 2017 10:19:46 AM PDT, Jeff Janes  wrote:
>On Tue, Jun 27, 2017 at 11:59 AM, Tom Lane  wrote:
>
>> I wrote:
>> > Andres Freund  writes:
>> >> On 2017-06-26 17:38:03 -0400, Tom Lane wrote:
>> >>> Hm.  Take that a bit further, and we could drop the connection
>probes
>> >>> altogether --- just put the whole responsibility on the
>postmaster to
>> >>> show in the pidfile whether it's ready for connections or not.
>>
>> >> Yea, that seems quite appealing, both from an architectural,
>simplicity,
>> >> and log noise perspective. I wonder if there's some added
>reliability by
>> >> the connection probe though? Essentially wondering if it'd be
>worthwhile
>> >> to keep a single connection test at the end. I'm somewhat
>disinclined
>> >> though.
>>
>> > I agree --- part of the appeal of this idea is that there could be
>a net
>> > subtraction of code from pg_ctl.  (I think it wouldn't have to link
>libpq
>> > anymore at all, though maybe I forgot something.)  And you get rid
>of a
>> > bunch of can't-connect failure modes, eg kernel packet filter in
>the way,
>> > which probably outweighs any hypothetical reliability gain from
>> confirming
>> > the postmaster state the old way.
>>
>> Here's a draft patch for that.  I quite like the results --- this
>seems
>> way simpler and more reliable than what pg_ctl has done up to now.
>> However, it's certainly arguable that this is too much change for an
>> optional post-beta patch.
>
>
>In the now-committed version of this, the 'pg_ctl start' returns
>successfully as soon as the server reaches a consistent state. Which is
>OK,
>except that it does the same thing when hot_standby=off.  When
>hot_standby=off, I would expect it to wait for the end of recovery
>before
>exiting with a success code.

I've not looked at the committed version, but earlier versions had code dealing 
with that difference; essentially doing what you suggest.

Andres
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.


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

Re: [HACKERS] Reducing pg_ctl's reaction time

[Jeff Janes]

On Tue, Jun 27, 2017 at 11:59 AM, Tom Lane  wrote:

> I wrote:
> > Andres Freund  writes:
> >> On 2017-06-26 17:38:03 -0400, Tom Lane wrote:
> >>> Hm.  Take that a bit further, and we could drop the connection probes
> >>> altogether --- just put the whole responsibility on the postmaster to
> >>> show in the pidfile whether it's ready for connections or not.
>
> >> Yea, that seems quite appealing, both from an architectural, simplicity,
> >> and log noise perspective. I wonder if there's some added reliability by
> >> the connection probe though? Essentially wondering if it'd be worthwhile
> >> to keep a single connection test at the end. I'm somewhat disinclined
> >> though.
>
> > I agree --- part of the appeal of this idea is that there could be a net
> > subtraction of code from pg_ctl.  (I think it wouldn't have to link libpq
> > anymore at all, though maybe I forgot something.)  And you get rid of a
> > bunch of can't-connect failure modes, eg kernel packet filter in the way,
> > which probably outweighs any hypothetical reliability gain from
> confirming
> > the postmaster state the old way.
>
> Here's a draft patch for that.  I quite like the results --- this seems
> way simpler and more reliable than what pg_ctl has done up to now.
> However, it's certainly arguable that this is too much change for an
> optional post-beta patch.


In the now-committed version of this, the 'pg_ctl start' returns
successfully as soon as the server reaches a consistent state. Which is OK,
except that it does the same thing when hot_standby=off.  When
hot_standby=off, I would expect it to wait for the end of recovery before
exiting with a success code.

Cheers,

Jeff

Re: [HACKERS] Dumping database creation options and ACLs

[Adrien Nayrat]

On 12/08/2014 04:21 PM, Ronan Dunklau wrote:
> Hello.
> 
> As of now, the only way to restore database options and ACLs is to use 
> pg_dumpall without the globals options. The often recommended pg_dumpall -g + 
> individual dumps of the target databases doesn't restore those.
> 
> Since pg_dump/pg_restore offer the ability to create the database, it should 
> do 
> so with the correct owner, options and database ACLs. 
> 
> There was some discussion about those issues a while ago (see 
> http://www.postgresql.org/message-id/11646.1272814...@sss.pgh.pa.us for 
> example). As I understand it, the best way to handle that would be to push 
> these modifications in pg_dump, but it is unclear how it should be done with 
> regards to restoring to a different database.
> 
> In the meantime, it would be great to add an option to pg_dumpall allowing to 
> dump this information. We could add the db creation in the output of 
> pg_dumpall -g,  and add a specific --createdb-only option (similar to --roles-
> only and --tablespaces-only).
> 
> Would such a patch be welcome ?
> 
> 
> 

Hello,


As reported by Ronan there's no other option than using pg_dumpall to restore
database options and ACLs.

So, we use this trick to stop pg_dumpall before \connect and then use 
pg_restore:

pg_dumpall -s | sed -rn '/^\\connect/{q}; p' > database+grants.sql


Of course, it is not graceful as we just need results of pg_dumpall -g and what
the dumpCreateDB() function outputs.

What do you think about adding an option like --createdb-only (as suggested by
Ronan) for this?  I'm not fully satisfied with this name though, I'll be happy
if you have a better suggestion.

Attached a naive patch.

-- 
Adrien NAYRAT

http://dalibo.com - http://dalibo.org
diff --git a/src/bin/pg_dump/pg_dumpall.c b/src/bin/pg_dump/pg_dumpall.c
index b14bb8e..35fa22d 100644
--- a/src/bin/pg_dump/pg_dumpall.c
+++ b/src/bin/pg_dump/pg_dumpall.c
@@ -68,6 +68,7 @@ static bool dosync = true;
 
 static int	binary_upgrade = 0;
 static int	column_inserts = 0;
+static int	createdb_only = 0;
 static int	disable_dollar_quoting = 0;
 static int	disable_triggers = 0;
 static int	if_exists = 0;
@@ -121,6 +122,7 @@ main(int argc, char *argv[])
 		{"attribute-inserts", no_argument, _inserts, 1},
 		{"binary-upgrade", no_argument, _upgrade, 1},
 		{"column-inserts", no_argument, _inserts, 1},
+		{"createdb-only", no_argument, _only, 1},
 		{"disable-dollar-quoting", no_argument, _dollar_quoting, 1},
 		{"disable-triggers", no_argument, _triggers, 1},
 		{"if-exists", no_argument, _exists, 1},
@@ -504,13 +506,13 @@ main(int argc, char *argv[])
 		 */
 		if (output_clean)
 		{
-			if (!globals_only && !roles_only && !tablespaces_only)
+			if (!globals_only && !roles_only && !tablespaces_only && !createdb_only)
 dropDBs(conn);
 
-			if (!roles_only && !no_tablespaces)
+			if (!roles_only && !no_tablespaces && !createdb_only)
 dropTablespaces(conn);
 
-			if (!tablespaces_only)
+			if (!tablespaces_only && !createdb_only)
 dropRoles(conn);
 		}
 
@@ -518,7 +520,7 @@ main(int argc, char *argv[])
 		 * Now create objects as requested.  Be careful that option logic here
 		 * is the same as for drops above.
 		 */
-		if (!tablespaces_only)
+		if (!tablespaces_only && !createdb_only)
 		{
 			/* Dump roles (users) */
 			dumpRoles(conn);
@@ -531,7 +533,7 @@ main(int argc, char *argv[])
 		}
 
 		/* Dump tablespaces */
-		if (!roles_only && !no_tablespaces)
+		if (!roles_only && !no_tablespaces && !createdb_only)
 			dumpTablespaces(conn);
 
 		/* Dump CREATE DATABASE commands */
@@ -539,14 +541,14 @@ main(int argc, char *argv[])
 			dumpCreateDB(conn);
 
 		/* Dump role/database settings */
-		if (!tablespaces_only && !roles_only)
+		if (!tablespaces_only && !roles_only && !createdb_only)
 		{
 			if (server_version >= 9)
 dumpDbRoleConfig(conn);
 		}
 	}
 
-	if (!globals_only && !roles_only && !tablespaces_only)
+	if (!globals_only && !roles_only && !tablespaces_only && !createdb_only)
 		dumpDatabases(conn);
 
 	PQfinish(conn);
@@ -594,6 +596,7 @@ help(void)
 	printf(_("  -x, --no-privileges  do not dump privileges (grant/revoke)\n"));
 	printf(_("  --binary-upgrade for use by upgrade utilities only\n"));
 	printf(_("  --column-inserts dump data as INSERT commands with column names\n"));
+	printf(_("  --createdb-only  CREATE and ACL databases commands\n"));
 	printf(_("  --disable-dollar-quoting disable dollar quoting, use SQL standard quoting\n"));
 	printf(_("  --disable-triggers   disable triggers during data-only restore\n"));
 	printf(_("  --if-exists  use IF EXISTS when dropping objects\n"));


signature.asc
Description: OpenPGP digital signature

Re: [BUGS] [HACKERS] Segmentation fault in libpq

[Merlin Moncure]

On Thu, Jun 29, 2017 at 9:12 AM, Tom Lane  wrote:
> Merlin Moncure  writes:
>> On Thu, Jun 29, 2017 at 8:23 AM, Michal Novotny
>>  wrote:
>>> Could you please help me based on information provided above?
>
>> You might want to run your code through some analysis tools (for
>> example, valgrind).
>
> valgrind is not a perfect tool for finding that kind of problem,
> especially if you can't reproduce the crash reliably; but at least
> valgrind is readily available and easy to use, so you might as
> well start there and see if it finds anything.  If you have access
> to any sort of static analysis tool (eg, Coverity), that might be
> more likely to help.  Or you could fall back on manual code
> auditing, if the program isn't very big.

clang static analyzer is another good tool to check out

https://clang-analyzer.llvm.org/

merlin


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

Re: [BUGS] [HACKERS] Segmentation fault in libpq

[Tom Lane]

Merlin Moncure  writes:
> On Thu, Jun 29, 2017 at 8:23 AM, Michal Novotny
>  wrote:
>> Could you please help me based on information provided above?

> You might want to run your code through some analysis tools (for
> example, valgrind).

Yeah, that's what I was about to suggest.  pqexpbuffer.c is pretty
small and paranoid code; it's really hard to see how it could have
crashed there unless something else corrupted its data structure.
While it's always possible that the "something else" was a wild
store from elsewhere in libpq, the lack of similar reports from
others and the fact that you don't sound to be doing anything very
exotic in terms of libpq requests both weigh against that theory.
If I had to bet given this much evidence, I'd bet on a wild store
from somewhere in your application having corrupted the
conn->errorMessage before PQexecParams was entered.  C is not a
language that does much to prevent that kind of bug for you.

valgrind is not a perfect tool for finding that kind of problem,
especially if you can't reproduce the crash reliably; but at least
valgrind is readily available and easy to use, so you might as
well start there and see if it finds anything.  If you have access
to any sort of static analysis tool (eg, Coverity), that might be
more likely to help.  Or you could fall back on manual code
auditing, if the program isn't very big.

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] Segmentation fault in libpq

[Merlin Moncure]

On Thu, Jun 29, 2017 at 8:23 AM, Michal Novotny
 wrote:
> Hi,
>
> comments inline ...
>
>
>
> On 06/29/2017 03:08 PM, Merlin Moncure wrote:
>>
>> On Thu, Jun 29, 2017 at 4:01 AM, Michal Novotny
>>  wrote:
>>>
>>> Hi all,
>>>
>>> we've developed an application using libpq to access a table in the PgSQL
>>> database but we're sometimes experiencing segmentation fault on
>>> resetPQExpBuffer() function of libpq called from PQexecParams() with
>>> prepared query.
>>>
>>> PostgreSQL version is 9.6.3 and the backtrace is:
>>>
>>> Core was generated by `/usr/ti/bin/status-monitor2 -m
>>> /usr/lib64/status-monitor2/modules'.
>>> Program terminated with signal 11, Segmentation fault.
>>> #0  resetPQExpBuffer (str=str@entry=0x9f4a28) at pqexpbuffer.c:152
>>> 152 str->data[0] = '\0';
>>>
>>> Thread 1 (Thread 0x7fdf68de3840 (LWP 3525)):
>>> #0  resetPQExpBuffer (str=str@entry=0x9f4a28) at pqexpbuffer.c:152
>>> No locals.
>>> #1  0x7fdf66e0333d in PQsendQueryStart (conn=conn@entry=0x9f46d0) at
>>> fe-exec.c:1371
>>> No locals.
>>> #2  0x7fdf66e044b9 in PQsendQueryParams (conn=conn@entry=0x9f46d0,
>>> command=command@entry=0x409a98 "SELECT min, hour, day, month, dow,
>>> sensor,
>>> module, params, priority, rt_due FROM sm.cron WHERE sensor = $1 ORDER BY
>>> priority DESC", nParams=nParams@entry=1, paramTypes=paramTypes@entry=0x0,
>>> paramValues=paramValues@entry=0xa2b7b0,
>>> paramLengths=paramLengths@entry=0x0,
>>> paramFormats=paramFormats@entry=0x0, resultFormat=resultFormat@entry=0)
>>> at
>>> fe-exec.c:1192
>>> No locals.
>>> #3  0x7fdf66e0552b in PQexecParams (conn=0x9f46d0, command=0x409a98
>>> "SELECT min, hour, day, month, dow, sensor, module, params, priority,
>>> rt_due
>>> FROM sm.cron WHERE sensor = $1 ORDER BY priority DESC", nParams=1,
>>> paramTypes=0x0, paramValues=0xa2b7b0, paramLengths=0x0, paramFormats=0x0,
>>> resultFormat=0) at fe-exec.c:1871
>>> No locals.
>>>
>>> Unfortunately we didn't have more information from the crash, at least
>>> for
>>> now.
>>>
>>> Is this a known issue and can you help me with this one?
>>
>> Is your application written in C?  We would need to completely rule
>> out your code (say, by double freeing result or something else nasty)
>> before assuming problem was withing libpq itself, particularly in this
>> area of the code.  How reproducible is the problem?
>>
>> merlin
>
>
> The application is written in plain C. The issue is it happens just
> sometimes - sometimes it happens and sometimes it doesn't.  Once it happens
> it causes the application crash but as it's systemd unit with
> Restart=on-failure flag it's automatically being restarted.
>
> What's being done is:
> 1) Ensure connection already exists and create a new one if it doesn't exist
> yet
> 2) Run PQexecParams() with specified $params that has $params_cnt elements:
>
> res = PQexecParams(conn, prepared_query, params_cnt, NULL, (const char
> **)params, NULL, NULL, 0);
>
> 3) Check for result and report error and exit if "PQresultStatus(res) !=
> PGRES_TUPLES_OK"
> 4) Do some processing with the result
> 5) Clear result using PQclear()
>
> It usually works fine but sometimes it's crashing and I don't know how to
> investigate further.
>
> Could you please help me based on information provided above?

You might want to run your code through some analysis tools (for
example, valgrind).  Short of that, to get help here you need to post
the code for review. How big is your application?

merlin


-- 
Sent 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_stop_backup(wait_for_archive := true) on standby server

[Magnus Hagander]

On Thu, Jun 22, 2017 at 6:22 PM, Masahiko Sawada 
wrote:

> On Thu, Jun 22, 2017 at 10:36 PM, Magnus Hagander 
> wrote:
> >
> >
> > On Thu, Jun 22, 2017 at 10:12 AM, Masahiko Sawada  >
> > wrote:
> >>
> >> Hi,
> >>
> >> Since an optional second argument wait_for_archive of pg_stop_backup
> >> has been  introduced in PostgreSQL 10 we can choose whether wait for
> >> archiving. But my colleagues found that we can do pg_stop_backup with
> >> wait_for_archive = true on the standby server but it actually doesn't
> >> wait for WAL archiving. Because this behavior is not documented and we
> >> cannot find out it without reading source code it will confuse the
> >> user.
> >>
> >> I think we can raise an error when pg_stop_backup with
> >> wait_for_archive = true is executed on the standby. Attached patch
> >> change it so that.
> >
> >
> > Wouldn't it be better to make it *work*? If you have
> archive_mode=always, it
> > makes sense to want to wait on the standby as well, does it not?
> >
>
> Yes, ideally it will be better to make it wait for WAL archiving on
> standby server when archive_mode=always. But I think it would be for
> PG11 item, and this item is for PG10.
>
>
I'm not sure. I think this can be considered a bug in the implementation
for 10, and as such is "open for fixing". However, it's not a very critical
bug so I doubt it should be a release blocker, but if someone wants to work
on a fix I think we should commit it.

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

Re: [HACKERS] Segmentation fault in libpq

[Michal Novotny]

Hi,

comments inline ...


On 06/29/2017 03:08 PM, Merlin Moncure wrote:

On Thu, Jun 29, 2017 at 4:01 AM, Michal Novotny
 wrote:

Hi all,

we've developed an application using libpq to access a table in the PgSQL
database but we're sometimes experiencing segmentation fault on
resetPQExpBuffer() function of libpq called from PQexecParams() with
prepared query.

PostgreSQL version is 9.6.3 and the backtrace is:

Core was generated by `/usr/ti/bin/status-monitor2 -m
/usr/lib64/status-monitor2/modules'.
Program terminated with signal 11, Segmentation fault.
#0  resetPQExpBuffer (str=str@entry=0x9f4a28) at pqexpbuffer.c:152
152 str->data[0] = '\0';

Thread 1 (Thread 0x7fdf68de3840 (LWP 3525)):
#0  resetPQExpBuffer (str=str@entry=0x9f4a28) at pqexpbuffer.c:152
No locals.
#1  0x7fdf66e0333d in PQsendQueryStart (conn=conn@entry=0x9f46d0) at
fe-exec.c:1371
No locals.
#2  0x7fdf66e044b9 in PQsendQueryParams (conn=conn@entry=0x9f46d0,
command=command@entry=0x409a98 "SELECT min, hour, day, month, dow, sensor,
module, params, priority, rt_due FROM sm.cron WHERE sensor = $1 ORDER BY
priority DESC", nParams=nParams@entry=1, paramTypes=paramTypes@entry=0x0,
paramValues=paramValues@entry=0xa2b7b0, paramLengths=paramLengths@entry=0x0,
paramFormats=paramFormats@entry=0x0, resultFormat=resultFormat@entry=0) at
fe-exec.c:1192
No locals.
#3  0x7fdf66e0552b in PQexecParams (conn=0x9f46d0, command=0x409a98
"SELECT min, hour, day, month, dow, sensor, module, params, priority, rt_due
FROM sm.cron WHERE sensor = $1 ORDER BY priority DESC", nParams=1,
paramTypes=0x0, paramValues=0xa2b7b0, paramLengths=0x0, paramFormats=0x0,
resultFormat=0) at fe-exec.c:1871
No locals.

Unfortunately we didn't have more information from the crash, at least for
now.

Is this a known issue and can you help me with this one?

Is your application written in C?  We would need to completely rule
out your code (say, by double freeing result or something else nasty)
before assuming problem was withing libpq itself, particularly in this
area of the code.  How reproducible is the problem?

merlin


The application is written in plain C. The issue is it happens just 
sometimes - sometimes it happens and sometimes it doesn't.  Once it 
happens it causes the application crash but as it's systemd unit with 
Restart=on-failure flag it's automatically being restarted.


What's being done is:
1) Ensure connection already exists and create a new one if it doesn't 
exist yet

2) Run PQexecParams() with specified $params that has $params_cnt elements:

res = PQexecParams(conn, prepared_query, params_cnt, NULL, (const char 
**)params, NULL, NULL, 0);


3) Check for result and report error and exit if "PQresultStatus(res) != 
PGRES_TUPLES_OK"

4) Do some processing with the result
5) Clear result using PQclear()

It usually works fine but sometimes it's crashing and I don't know how 
to investigate further.


Could you please help me based on information provided above?

Thanks,
Michal

--
Michal Novotny
System Development Lead
michal.novo...@greycortex.com

GREYCORTEX s.r.o.
Purkynova 127, 61200 Brno
Czech Republic
www.greycortex.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] Segmentation fault in libpq

[Merlin Moncure]

On Thu, Jun 29, 2017 at 4:01 AM, Michal Novotny
 wrote:
> Hi all,
>
> we've developed an application using libpq to access a table in the PgSQL
> database but we're sometimes experiencing segmentation fault on
> resetPQExpBuffer() function of libpq called from PQexecParams() with
> prepared query.
>
> PostgreSQL version is 9.6.3 and the backtrace is:
>
> Core was generated by `/usr/ti/bin/status-monitor2 -m
> /usr/lib64/status-monitor2/modules'.
> Program terminated with signal 11, Segmentation fault.
> #0  resetPQExpBuffer (str=str@entry=0x9f4a28) at pqexpbuffer.c:152
> 152 str->data[0] = '\0';
>
> Thread 1 (Thread 0x7fdf68de3840 (LWP 3525)):
> #0  resetPQExpBuffer (str=str@entry=0x9f4a28) at pqexpbuffer.c:152
> No locals.
> #1  0x7fdf66e0333d in PQsendQueryStart (conn=conn@entry=0x9f46d0) at
> fe-exec.c:1371
> No locals.
> #2  0x7fdf66e044b9 in PQsendQueryParams (conn=conn@entry=0x9f46d0,
> command=command@entry=0x409a98 "SELECT min, hour, day, month, dow, sensor,
> module, params, priority, rt_due FROM sm.cron WHERE sensor = $1 ORDER BY
> priority DESC", nParams=nParams@entry=1, paramTypes=paramTypes@entry=0x0,
> paramValues=paramValues@entry=0xa2b7b0, paramLengths=paramLengths@entry=0x0,
> paramFormats=paramFormats@entry=0x0, resultFormat=resultFormat@entry=0) at
> fe-exec.c:1192
> No locals.
> #3  0x7fdf66e0552b in PQexecParams (conn=0x9f46d0, command=0x409a98
> "SELECT min, hour, day, month, dow, sensor, module, params, priority, rt_due
> FROM sm.cron WHERE sensor = $1 ORDER BY priority DESC", nParams=1,
> paramTypes=0x0, paramValues=0xa2b7b0, paramLengths=0x0, paramFormats=0x0,
> resultFormat=0) at fe-exec.c:1871
> No locals.
>
> Unfortunately we didn't have more information from the crash, at least for
> now.
>
> Is this a known issue and can you help me with this one?

Is your application written in C?  We would need to completely rule
out your code (say, by double freeing result or something else nasty)
before assuming problem was withing libpq itself, particularly in this
area of the code.  How reproducible is the problem?

merlin


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

Re: [HACKERS] Causal reads take II

[Thomas Munro]

On Sat, Jun 24, 2017 at 4:05 PM, Thomas Munro
 wrote:
> On Fri, Jun 23, 2017 at 11:48 PM, Thomas Munro
>  wrote:
>> Maybe it needs a better name.
>
> Ok, how about this: the feature could be called "synchronous replay".
> The new column in pg_stat_replication could be called sync_replay
> (like the other sync_XXX columns).  The GUCs could be called
> synchronous replay, synchronous_replay_max_lag and
> synchronous_replay_lease_time.  The language in log messages could
> refer to standbys "joining the synchronous replay set".

Feature hereby renamed that way.  It seems a lot more
self-explanatory.  Please see attached.

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


synchronous-replay-v1.patch
Description: Binary data


test-synchronous-replay.sh
Description: Bourne shell script
/*
 * A simple test program to test performance and visibility with the
 * synchronous replay patch.
 *
 * Each test loop updates a row on the primary, and then optionally checks if
 * it can see that change immediately on a standby.  If you do this with
 * standard async replication, you should occasionally see an assertion fail
 * if run with --check (depending on the vaguaries of timing -- I can
 * reproduce this very reliably on my system).  If you do it with traditional
 * sync rep, it becomes a little bit less likely (but it's still reliably
 * reproducible on my system).  If you do it with traditional sync rep set up,
 * and "--synchronous-commit remote_apply" then it should no longer be
 * possible to trigger than assertion, but that mode doesn't handle failures.
 * If you do it with --synchronous-replay then you should not be able to
 * reproduce it, no matter which standby you connect to.  If you're using
 * --check and the standby gets dropped (perhaps because you
 * break/disconnect/pause it etc) you should never see that assertion fail
 * (= SELECT running but seeing stale data), instead you should see an error
 * when running the SELECT.
 *
 * Arguments:
 *
 *  --primary  how to connect to the primary
 *  --standby  how to connect to the standby to check
 *  --check   check that the update is visible on standby
 *  --synchronous-replay  enable synchronous replay
 *  --synchronous-commit LEVELset synchronous_commit to LEVEL
 *  --two-phase-commitenable two-phase commit
 *  --loops COUNT how many loops to run through
 *  --verbose chatter
 */

#include 

#include 
#include 
#include 
#include 
#include 
#include 

static double
get_hires_time(void)
{
	struct timeval tv;

	gettimeofday(, NULL);
	return tv.tv_sec + tv.tv_usec / 100.0;
}

int
main(int argc, char *argv[])
{
	PGconn *primary;
	PGconn *standby;
	PGresult *result;
	int i;
	int loops = 1;
	char buffer[1024];
	const char *synchronous_commit = "on";
	bool synchronous_replay = false;
	const char *primary_connstr = "dbname=postgres port=5432";
	const char *standby_connstr = "dbname=postgres port=5441";
	bool check_applied = false;
	bool verbose = false;
	bool two_phase_commit = false;
	double start_time;

	for (i = 1; i != argc; ++i)
	{
		bool more = (i < argc - 1);

		if (strcmp(argv[i], "--verbose") == 0)
			verbose = true;
		else if (strcmp(argv[i], "--check") == 0)
			check_applied = true;
		else if (strcmp(argv[i], "--synchronous-commit") == 0 && more)
			synchronous_commit = argv[++i];
		else if (strcmp(argv[i], "--synchronous-replay") == 0)
			synchronous_replay = true;
		else if (strcmp(argv[i], "--primary") == 0 && more)
			primary_connstr = argv[++i];
		else if (strcmp(argv[i], "--standby") == 0 && more)
			standby_connstr = argv[++i];
		else if (strcmp(argv[i], "--loops") == 0 && more)
			loops = atoi(argv[++i]);
		else if (strcmp(argv[i], "--two-phase-commit") == 0)
			two_phase_commit = true;
		else
		{
			fprintf(stderr, "bad argument\n");
			exit(1);
		}
	}

	primary = PQconnectdb(primary_connstr);
	assert(PQstatus(primary) == CONNECTION_OK);

	standby = PQconnectdb(standby_connstr);
	assert(PQstatus(standby) == CONNECTION_OK);

	snprintf(buffer, sizeof(buffer), "SET synchronous_commit = %s", synchronous_commit);
	result = PQexec(primary, buffer);
	assert(PQresultStatus(result) == PGRES_COMMAND_OK);
	PQclear(result);
	if (synchronous_replay)
	{
		snprintf(buffer, sizeof(buffer), "SET synchronous_replay = on");
		result = PQexec(primary, buffer);
		assert(PQresultStatus(result) == PGRES_COMMAND_OK);
		PQclear(result);
	}

	snprintf(buffer, sizeof(buffer), "SET synchronous_commit = %s", synchronous_commit);
	result = PQexec(standby, buffer);
	assert(PQresultStatus(result) == PGRES_COMMAND_OK);
	PQclear(result);
	if (synchronous_replay)
	{
		snprintf(buffer, sizeof(buffer), "SET synchronous_replay = on");
		result = PQexec(standby, buffer);
		assert(PQresultStatus(result) == PGRES_COMMAND_OK);
		PQclear(result);
	}

	result = PQexec(primary, "CREATE TABLE counter AS SELECT 0 

[HACKERS] Segmentation fault in libpq

[Michal Novotny]

Hi all,

we've developed an application using libpq to access a table in the 
PgSQL database but we're sometimes experiencing segmentation fault on 
resetPQExpBuffer() function of libpq called from PQexecParams() with 
prepared query.


PostgreSQL version is 9.6.3 and the backtrace is:

Core was generated by `/usr/ti/bin/status-monitor2 -m 
/usr/lib64/status-monitor2/modules'.
Program terminated with signal 11, Segmentation fault.
#0  resetPQExpBuffer (str=str@entry=0x9f4a28) at pqexpbuffer.c:152
152 str->data[0] = '\0';

Thread  1 (Thread  0x7fdf68de3840 (LWP 3525)):
#0  resetPQExpBuffer (str=str@entry=0x9f4a28) at pqexpbuffer.c:152
No locals.
#1  0x7fdf66e0333d in PQsendQueryStart (conn=conn@entry=0x9f46d0) at 
fe-exec.c:1371
No locals.
#2  0x7fdf66e044b9 in PQsendQueryParams (conn=conn@entry=0x9f46d0, command=command@entry=0x409a98"SELECT min, hour, day, month, dow, sensor, module, params, priority, 
rt_due FROM sm.cron WHERE sensor = $1 ORDER BY priority DESC", nParams=nParams@entry=1, paramTypes=paramTypes@entry=0x0, paramValues=paramValues@entry=0xa2b7b0, paramLengths=paramLengths@entry=0x0, paramFormats=paramFormats@entry=0x0, resultFormat=resultFormat@entry=0) at fe-exec.c:1192

No locals.
#3  0x7fdf66e0552b in PQexecParams (conn=0x9f46d0, command=0x409a98"SELECT min, hour, day, month, dow, sensor, module, params, priority, 
rt_due FROM sm.cron WHERE sensor = $1 ORDER BY priority DESC", nParams=1, paramTypes=0x0, paramValues=0xa2b7b0, paramLengths=0x0, paramFormats=0x0, resultFormat=0) at fe-exec.c:1871

No locals.

Unfortunately we didn't have more information from the crash, at least 
for now.


Is this a known issue and can you help me with this one?

Thanks,
Michal

--
Michal Novotny
System Development Lead
michal.novo...@greycortex.com

GREYCORTEX s.r.o.
Purkynova 127, 61200 Brno
Czech Republic
www.greycortex.com

Re: protocol version negotiation (Re: [HACKERS] Libpq PGRES_COPY_BOTH - version compatibility)

[Robert Haas]

On Wed, Jun 28, 2017 at 10:27 PM, Tom Lane  wrote:
> Yeah.  Back in the day I helped design the PNG image format, and one
> of the better ideas in it was to make a distinction between critical and
> noncritical chunks within a PNG file; that was exactly the idea you're
> getting at here.  I agree with the suggestion to drive this off a
> parameter-name-based convention.  Having leading underscore indicate
> a noncritical parameter sounds fine.

I'm not sure if non-critical is exactly the right terminology.  What
we want to do is distinguish between things that are intended as
protocol-level options vs. things that are intended as GUCs.  Of
course, there's nothing that actually prevents a a GUC from starting
with an underscore:

rhaas=# set _foo._bar = 1;
SET

Maybe we should instead pick a GUC namespace and reserve it for the
use of protocol level options; e.g. pg_protocol. becomes
invalid as the name of a GUC, but can be included in a startup packet
(or do we need to pick something shorter, like _pg, to keep the
message short?).  Servers ignore anything that they don't understand.
So:

1. The client sends a StartupMessage 3.x for version 3.x.  We could
bump the version explicitly, or perhaps we should just coin a version
of libpq for every server release; e.g. whatever PostgreSQL 11 ships
is version 3.11, etc.  It includes any protocol options that don't
exist today as pg_protocol. in the startup packet.

2. If the client version is anything other than 3.0, the server
responds with a ServerProtocolVersion indicating the highest version
it supports, and ignores any pg_protocol. options not known
to it as being either third-party extensions or something from a
future version.  If the initial response to the startup message is
anything other than a ServerProtocolVersion message, the client should
assume it's talking to a 3.0 server.  (To make this work, we would
back-patch a change into existing releases to allow any 3.x protocol
version and ignore any pg_protocol. options that were
specified.)

If either the client or the server is unhappy about the age of the
other, then it can disconnect; e.g. if the server is configured to
require the use of whizzbang-2 security, and the client protocol
version indicates that at most whizzbang-1.9 is available, then the
server can close the connection with a suitable complaint; conversely,
if the connection string had require_whizzbang=2, and the server is
too old to support that, then the client can decide to bail out when
it receives the ServerProtocolVersion message.

Still just thinking out loud here.  Thoughts?

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


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

Re: [HACKERS] UPDATE of partition key

[Amit Khandekar]

On 29 June 2017 at 07:42, Amit Langote  wrote:
> Hi Amit,
>
> On 2017/06/28 20:43, Amit Khandekar wrote:
>> In attached patch v12
>
> The patch no longer applies and fails to compile after the following
> commit was made yesterday:
>
> commit 501ed02cf6f4f60c3357775eb07578aebc912d3a
> Author: Andrew Gierth 
> Date:   Wed Jun 28 18:55:03 2017 +0100
>
> Fix transition tables for partition/inheritance.

Thanks for informing Amit.

As Thomas mentioned upthread, the above commit already uses a tuple
conversion mapping from leaf partition to root partitioned table
(mt_transition_tupconv_maps), which serves the same purpose as that of
the mapping used in the update-partition-key patch during update tuple
routing (mt_resultrel_maps).

We need to try to merge these two into a general-purpose mapping array
such as mt_leaf_root_maps. I haven't done that in the rebased patch
(attached), so currently it has both these mapping fields.

For transition tables, this map is per-leaf-partition in case of
inserts, whereas it is per-subplan result rel for updates. For
update-tuple routing, the mapping is required to be per-subplan. Now,
for update-row-movement in presence of transition tables, we would
require both per-subplan mapping as well as per-leaf-partition
mapping, which can't be done if we have a single mapping field, unless
we have some way to identify which of the per-leaf partition mapping
elements belong to per-subplan rels.

So, it's not immediately possible to merge them.


update-partition-key_v12_rebased.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] Declarative partitioning - another take

[Etsuro Fujita]

Hi Maksim,

On 2017/04/07 19:52, Maksim Milyutin wrote:

On 07.04.2017 13:05, Etsuro Fujita wrote:



On 2016/12/09 19:46, Maksim Milyutin wrote:

I would like to work on two tasks:
 - insert (and eventually update) tuple routing for foreign partition.
 - the ability to create an index on the parent and have all of the
children inherit it;



There seems to be no work on the first one, so I'd like to work on that.



Yes, you can start to work on this, I'll join later as a reviewer.


Great!  I added the patch to the next commitfest:

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

Sorry for the delay.

Best regards,
Etsuro Fujita



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

[HACKERS] Add support for tuple routing to foreign partitions

[Etsuro Fujita]

Hi,

Here is a patch for $subject.  This is based on Amit's original patch 
(patch '0007-Tuple-routing-for-partitioned-tables-15.patch' in [1]). 
Main changes are:


* I like Amit's idea that for each partition that is a foreign table, 
the FDW performs query planning in the same way as in non-partition 
cases, but the changes to make_modifytable() in the original patch that 
creates a working-copy of Query to pass to PlanForeignModify() seem 
unacceptable.  So, I modified the changes so that we create 
more-valid-looking copies of Query and ModifyTable with the foreign 
partition as target, as I said before.  In relation to this, I also 
allowed expand_inherited_rtentry() to build an RTE and AppendRelInfo for 
each partition of a partitioned table that is an INSERT target, as 
mentioned by Amit in [1], by modifying transformInsertStmt() so that we 
set the inh flag for the target table's RTE to true when the target 
table is a partitioned table.  The partition RTEs are not only 
referenced by FDWs, but used in selecting relation aliases for EXPLAIN 
(see below) as well as extracting plan dependencies in setref.c so that 
we force rebuilding of the plan on any change to partitions.  (We do 
replanning on FDW table-option changes to foreign partitions, currently. 
 See regression tests in the attached patch.)


* The original patch added tuple routing to foreign partitions in COPY 
FROM, but I'm not sure the changes to BeginCopy() in the patch are the 
right way to go.  For example, the patch passes a dummy empty Query to 
PlanForeignModify() to make FDWs perform query planning, but I think 
FDWs would be surprised by the Query.  Currently, we don't support COPY 
to foreign tables, so ISTM that it's better to revisit this issue when 
adding support for COPY to foreign tables.  So, I dropped the COPY part.


* I modified explain.c so that EXPLAIN for an INSERT into a partitioned 
table displays partitions just below the ModifyTable node, and shows 
remote queries for foreign partitions in the same way as EXPLAIN for 
UPDATE/DELETE cases.  Here is an example:


postgres=# explain verbose insert into pt values (1), (2);
QUERY PLAN
---
 Insert on public.pt  (cost=0.00..0.03 rows=2 width=4)
   Foreign Insert on public.fp1
 Remote SQL: INSERT INTO public.t1(a) VALUES ($1)
   Foreign Insert on public.fp2
 Remote SQL: INSERT INTO public.t2(a) VALUES ($1)
   ->  Values Scan on "*VALUES*"  (cost=0.00..0.03 rows=2 width=4)
 Output: "*VALUES*".column1
(7 rows)

Comments are welcome!  Anyway, I'll add this to the next commitfest.

Best regards,
Etsuro Fujita

[1] 
https://www.postgresql.org/message-id/aa874eaf-cd3b-0f75-c647-6c0ea823781e%40lab.ntt.co.jp
*** a/contrib/postgres_fdw/expected/postgres_fdw.out
--- b/contrib/postgres_fdw/expected/postgres_fdw.out
***
*** 6931,6936  NOTICE:  drop cascades to foreign table bar2
--- 6931,7074 
  drop table loct1;
  drop table loct2;
  -- ===
+ -- test tuple routing to foreign-table partitions
+ -- ===
+ create table pt (a int, b int) partition by list (a);
+ create table locp partition of pt for values in (1);
+ create table locfoo (a int check (a in (2)), b int);
+ create foreign table remp partition of pt for values in (2) server loopback 
options (table_name 'locfoo');
+ explain (verbose, costs off)
+ insert into pt values (1, 1), (2, 1);
+QUERY PLAN
+ -
+  Insert on public.pt
+Insert on public.locp
+Foreign Insert on public.remp
+  Remote SQL: INSERT INTO public.locfoo(a, b) VALUES ($1, $2)
+->  Values Scan on "*VALUES*"
+  Output: "*VALUES*".column1, "*VALUES*".column2
+ (6 rows)
+ 
+ insert into pt values (1, 1), (2, 1);
+ select tableoid::regclass, * FROM pt;
+  tableoid | a | b 
+ --+---+---
+  locp | 1 | 1
+  remp | 2 | 1
+ (2 rows)
+ 
+ select tableoid::regclass, * FROM locp;
+  tableoid | a | b 
+ --+---+---
+  locp | 1 | 1
+ (1 row)
+ 
+ select tableoid::regclass, * FROM remp;
+  tableoid | a | b 
+ --+---+---
+  remp | 2 | 1
+ (1 row)
+ 
+ explain (verbose, costs off)
+ insert into pt values (1, 2), (2, 2) returning *;
+QUERY PLAN 
  
+ 

+  Insert on public.pt
+Output: pt.a, pt.b
+Insert on public.locp
+Foreign Insert on public.remp
+  Remote SQL: INSERT INTO public.locfoo(a, b) VALUES ($1, $2) RETURNING a, 
b
+->  Values Scan on "*VALUES*"
+  Output: "*VALUES*".column1, "*VALUES*".column2
+ (7 rows)
+ 
+ insert into pt values (1, 2), (2, 2) returning *;
+  a 

Re: [HACKERS] What does it mean by XLOG_BACKUP_RECORD?

[Masahiko Sawada]

On Thu, Jun 29, 2017 at 10:49 AM, Michael Paquier
 wrote:
> On Thu, Jun 29, 2017 at 10:28 AM, Masahiko Sawada  
> wrote:
>> While reading source codes I found the following comment in xlog.c.
>>
>> /*
>>  * Have we passed our safe starting point? Note that minRecoveryPoint is
>>  * known to be incorrectly set if ControlFile->backupEndRequired, until
>>  * the XLOG_BACKUP_RECORD arrives to advise us of the correct
>>  * minRecoveryPoint. All we know prior to that is that we're not
>>  * consistent yet.
>>  */
>> if (!reachedConsistency && !ControlFile->backupEndRequired &&
>> minRecoveryPoint <= lastReplayedEndRecPtr &&
>> XLogRecPtrIsInvalid(ControlFile->backupStartPoint))
>>
>> What does XLOG_BACKUP_RECORED means by? I could not find such XLOG info 
>> value.
>> Does it mean XLOG_BACKUP_END?
>
> This comment is a thinko, it refers to XLOG_BACKUP_END. This comment
> block could be reworded a bit, it looks cleaner to me to say
> "ControlFile->backupEndRequired is false" instead of just referring to
> the variable itself.

Thanks, I agree to use XLOG_BACKUP_END instead.

> Worse, the current comment implies that
> minRecoveryPoint is incorrectly set if it is true. Bleh.

Looking at the condition, we use minRecoveryPoint only when
ControlFile->backupEndRequired is *false*. So I guess that it means
that minRecoveryPoint is incorrectly set if
ControlFile->backupEndReuired is true. Am I missing something?

Regards,

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


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

Re: [HACKERS] Start logical decoding from any lsn present in the wal segment

[Craig Ringer]

On 29 June 2017 at 16:32, sanyam jain  wrote:
> Hi,
>
> Currently logical decoding finds a consistent point by generating a snapshot
> and stream changes after consistent point.I want to change this behaviour to
> something which stream changes from a lsn in the past provided its present
> in wal segment. Can this behaviour be implemented

No, it can't. Not without infinite catalog bloat, anyway.

To perform logical decoding we need catalogs that contain historic
data in the form of dead tuples preserved in the heaps. Say you ALTER
TABLE to add a column and drop one. There'll be a new pg_attribute row
for the new column, and the old pg_attribute row will get marked with
attisdropped by writing a new row version.

When we're doing logical decoding and examining WAL from before the
ALTER TABLE, we need to see the old state of the catalogs. So a
historic snapshot is created with an (xmin,xmax) range that means it
won't see the new pg_attribute row, and it'll see the old row-version
of the dropped attribute where attisdropped is not set. So it'll see a
table definition that matches how it was at the time the WAL it's
looking at was generated.

If you could only see the latest table definition from pg_catalog
you'd have no way to correctly decode such WAL.

That's what all the catalog_xmin business in the code is about.

There are some other complications too, around finding safe points in
the WAL stream where running xacts are precisely known; see
xl_running_xacts and the snapshot builder.

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

[HACKERS] Start logical decoding from any lsn present in the wal segment

[sanyam jain]

Hi,

Currently logical decoding finds a consistent point by generating a snapshot 
and stream changes after consistent point.I want to change this behaviour to 
something which stream changes from a lsn in the past provided its present in 
wal segment.Can this behaviour be implemented and if yes, what are the steps i 
should perform to implement the same.


Thanks,

Sanyam Jain

Re: [HACKERS] [PATCH] pg_reload_backend to signal SIGHUP to a specific backend

[Dmitry Dolgov]

> On 29 Jun 2017 08:18, "Yugo Nagata"  wrote:
>
> This is because this function is defined as strict

Yes, I realized that few moments after I sent my message.

> Arg type is needed.
>
> =# grant execute on function pg_reload_backend(int) to test_user;

Oh, I see, thank you.

Re: [HACKERS] [PATCH] pg_reload_backend to signal SIGHUP to a specific backend

[Yugo Nagata]

On Wed, 28 Jun 2017 13:59:51 +0200
Remi Colinet  wrote:

> Hi Yugo,
> 
> The patch looks fine. Installed and tested.

Thank you for your interest.

> 
> But a similar function already exists for the Postmaster process.
> 
> Datum
> pg_reload_conf(PG_FUNCTION_ARGS)
> {
> if (kill(PostmasterPid, SIGHUP))
> {
> ereport(WARNING,
> (errmsg("failed to send signal to
> postmaster: %m")));
> PG_RETURN_BOOL(false);
> }
> 
> PG_RETURN_BOOL(true);
> }
> 
> I would suggest to merge your patch with above function which is close to
> yours.
> Btw, your patch looks better that above function code.
> 
> You may for instance retain pg_reload_conf() without argument for the same
> purpose as currently (Postmaster signaling).
> And provide a pid argument to signal a given backend.

Actually, I proposed that version patch in the previous thread[1], but there
as a suggestion to use other function name, so I attached fixed version
in this thread. 

[1] 
https://www.postgresql.org/message-id/20170624000156.74ca9485.nagata%40sraoss.co.jp

> 
> Regards
> Remi
> 
> 
> 2017-06-28 12:17 GMT+02:00 Yugo Nagata :
> 
> > Hi,
> >
> > Attached is a patch of pg_reload_backend that is a function signaling
> > SIGHUP to a specific backend. The original idea is from Michael Paquier[1].
> > The documatation isn't included in this patch yet.
> >
> > We can change some parameters of other backend using the function
> > as bellow.
> >
> > postgres=# alter system set log_min_messages to debug2;
> > ALTER SYSTEM
> > postgres=# select pg_reload_backend(18375);
> >  pg_reload_backend
> > ---
> >  t
> > (1 row)
> >
> > There are some usecases. For example:
> >
> > - changing log configuration in other backends temporally.
> > - changing cost parameters for planner in other backends.
> > - changing autovacuum parameters of an already running autovacuum worker.
> >
> >
> > Hoever, this function need the superuser previlige to execute as well as
> > pg_reload_conf(). Although we can grant the execution to public, it is
> > useless for normal users bacause only superuser can change postgresql.conf.
> >
> > To allow normal users to change parameters in ohter backends, instead
> > we might want another feature, for example, a function like set_config()
> > than can change other backend's parameters using shared memory and SIGUSR1.
> >
> > Any comments would be appreciated.
> >
> > Regards,
> >
> > [1]
> > https://www.postgresql.org/message-id/CAB7nPqT4y8-
> > QoGKEugk99_tFuEOtAshcs5kxOeZ_0w27UtdGyA%40mail.gmail.com
> >
> > --
> > Yugo Nagata 
> >
> >
> > --
> > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> > To make changes to your subscription:
> > http://www.postgresql.org/mailpref/pgsql-hackers
> >
> >


-- 
Yugo Nagata 


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

Re: [HACKERS] [PATCH] pg_reload_backend to signal SIGHUP to a specific backend

[Yugo Nagata]

On Wed, 28 Jun 2017 13:35:12 +0200
Dmitry Dolgov <9erthali...@gmail.com> wrote:

> > On 28 June 2017 at 12:17, Yugo Nagata  wrote:
> >
> > Hi,
> >
> > Attached is a patch of pg_reload_backend that is a function signaling
> > SIGHUP to a specific backend. The original idea is from Michael
> Paquier[1].
> > The documatation isn't included in this patch yet.
> 
> I have few questions. I'm curious, why this function returns something
> different from bool when I'm passing null as an argument?

This is because this function is defined as strict, that is pg_proc.proisstrict
is true, as well as pg_reload_conf, pg_terminate_backend, pg_cancel_bacnend, 
and so on.

> 
> =# select pg_reload_backend(27961);
> WARNING:  PID 27961 is not a PostgreSQL server process
> WARNING:  failed to send signal to backend: 27961
>  pg_reload_backend
> ---
>  f
> (1 row)
> 
> =# select pg_reload_backend(27962);
>  pg_reload_backend
> ---
>  t
> (1 row)
> 
> =# select pg_reload_backend(null);
>  pg_reload_backend
> ---
> 
> (1 row)
> 
> Also for some reason I can't grant an execute permission on this function,
> am I doing something wrong?
> 
> =# grant execute on function pg_reload_backend() to test_user;
> ERROR:  function pg_reload_backend() does not exist
> =# grant execute on function pg_reload_conf() to test_user;
> GRANT

Arg type is needed.

=# grant execute on function pg_reload_backend(int) to test_user;

-- 
Yugo Nagata 


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