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

[Michael Paquier]

On Wed, May 10, 2017 at 11:57 AM, Masahiko Sawada  wrote:
> Barring any objections, I'll add these two issues to open item.

It seems to me that those open items have not been added yet to the
list. If I am following correctly, they could be defined as follows:
- Dropping subscription may stuck if done during tablesync.
-- Analyze deadlock issues with DROP SUBSCRIPTION and apply worker process.
-- Avoid orphaned tablesync worker if apply worker exits before
changing its status.

I am playing with the code to look at both of them... But feel free to
update this thread if I don't show up. There are no test cases, but
some well-placed pg_usleep calls should make both issues easily
reproducible. I have the gut feeling that other things are hidden
behind though.
-- 
Michael


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

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

[Heikki Linnakangas]

On 05/11/2017 07:03 AM, Michael Paquier wrote:

On Thu, May 11, 2017 at 11:50 AM, Bruce Momjian  wrote:

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


As Postgres ODBC now has a hard dependency with libpq, no actions is
taken from there. At least this makes one driver worth mentioning.


FWIW, I wrote a patch for the Go driver at https://github.com/lib/pq, to 
implement SCRAM. It's awaiting review.


I updated the List of Drivers in the Wiki. I added a few drivers that 
were missing, like the ODBC driver, and the pgtclng driver, as well as a 
Go and Rust driver that I'm aware of. I reformatted it, and added a 
column to indicate whether each driver uses libpq or not.


https://wiki.postgresql.org/wiki/List_of_drivers

There is a similar list in our docs:

https://www.postgresql.org/docs/devel/static/external-interfaces.html

Should we update the list in the docs, adding every driver we know of? 
Or curate the list somehow, adding only more popular drivers? Or perhaps 
add a link to the Wiki page from the docs?


We can use this list in the Wiki to track which drivers implement SCRAM.

- 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] Re: logical replication syntax (was DROP SUBSCRIPTION, query cancellations and slot handling)

[Noah Misch]

On Mon, May 08, 2017 at 05:01:00PM -0400, Peter Eisentraut wrote:
> On 5/7/17 04:21, Noah Misch wrote:
> > The above-described topic is currently a PostgreSQL 10 open item.  Peter,
> > since you committed the patch believed to have created it, you own this open
> > item.  If some other commit is more relevant or if this does not belong as a
> > v10 open item, please let us know.  Otherwise, please observe the policy on
> > open item ownership[1] and send a status update within three calendar days 
> > of
> > this message.  Include a date for your subsequent status update.  Testers 
> > may
> > discover new open items at any time, and I want to plan to get them all 
> > fixed
> > well in advance of shipping v10.  Consequently, I will appreciate your 
> > efforts
> > toward speedy resolution.  Thanks.
> 
> I think we have a workable patch, but it needs some rebasing.  I hope to
> have this sorted by Wednesday.

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] Time based lag tracking for logical replication

[Noah Misch]

On Sun, Apr 23, 2017 at 01:10:32AM +0200, Petr Jelinek wrote:
> The time based lag tracking commit [1] added interface for logging
> progress of replication so that we can report lag as time interval
> instead of just bytes. But the patch didn't contain patch for the
> builtin logical replication.
> 
> So I wrote something that implements this.

This is listed as a PostgreSQL 10 open item, but the above makes it sound like
a feature to consider for v11, not a defect in v10.  Why is this an open item?


-- 
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: transition table behavior with inheritance appears broken (was: Declarative partitioning - another take)

[Noah Misch]

On Sat, May 06, 2017 at 06:54:37PM +, Noah Misch wrote:
> On Mon, May 01, 2017 at 11:10:52AM -0500, Kevin Grittner wrote:
> > On Mon, May 1, 2017 at 10:01 AM, Robert Haas  wrote:
> > 
> > > It seems pretty clear to me that this is busted.
> > 
> > I don't think you actually tested anything that is dependent on any
> > of my patches there.
> > 
> > > Adding this as an open item.  Kevin?
> > 
> > It will take some time to establish what legacy behavior is and how
> > the new transition tables are impacted.  My first reaction is that a
> > trigger on the parent should fire for any related action on a child
> > (unless maybe the trigger is defined with an ONLY keyword???) using
> > the TupleDesc of the parent.  Note that the SQL spec mandates that
> > even in a AFTER EACH ROW trigger the transition tables must
> > represent all rows affected by the STATEMENT.  I think that this
> > should be independent of triggers fired at the row level.  I think
> > the rules should be similar for updateable views.
> > 
> > This will take some time to investigate, discuss and produce a
> > patch.  I think best case is Friday.
> 
> [Action required within three days.  This is a generic notification.]
> 
> The above-described topic is currently a PostgreSQL 10 open item.  Kevin,
> 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] Get stuck when dropping a subscription during synchronizing table

[Masahiko Sawada]

On Thu, May 11, 2017 at 4:06 PM, Michael Paquier
 wrote:
> On Wed, May 10, 2017 at 11:57 AM, Masahiko Sawada  
> wrote:
>> Barring any objections, I'll add these two issues to open item.
>
> It seems to me that those open items have not been added yet to the
> list. If I am following correctly, they could be defined as follows:
> - Dropping subscription may stuck if done during tablesync.
> -- Analyze deadlock issues with DROP SUBSCRIPTION and apply worker process.
> -- Avoid orphaned tablesync worker if apply worker exits before
> changing its status.

Thanks, I think correct. Added it to open item.

>
> I am playing with the code to look at both of them... But feel free to
> update this thread if I don't show up. There are no test cases, but
> some well-placed pg_usleep calls should make both issues easily
> reproducible. I have the gut feeling that other things are hidden
> behind though.

I'm also working on this, so will update it if there is.

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

[Petr Jelinek]

On 11/05/17 10:10, Masahiko Sawada wrote:
> On Thu, May 11, 2017 at 4:06 PM, Michael Paquier
>  wrote:
>> On Wed, May 10, 2017 at 11:57 AM, Masahiko Sawada  
>> wrote:
>>> Barring any objections, I'll add these two issues to open item.
>>
>> It seems to me that those open items have not been added yet to the
>> list. If I am following correctly, they could be defined as follows:
>> - Dropping subscription may stuck if done during tablesync.
>> -- Analyze deadlock issues with DROP SUBSCRIPTION and apply worker process.

I think the solution to this is to reintroduce the LWLock that was
removed and replaced with the exclusive lock on catalog [1]. I am afraid
that correct way of handling this is to do both LWLock and catalog lock
(first LWLock under which we kill the workers and then catalog lock so
that something that prevents launcher from restarting them is held till
the end of transaction).

>> -- Avoid orphaned tablesync worker if apply worker exits before
>> changing its status.
> 

The behavior question I have about this is if sync workers should die
when apply worker dies (ie they are tied to apply worker) or if they
should be tied to the subscription.

I guess taking down all the sync workers when apply worker has exited is
easier to solve. Of course it means that if apply worker restarts in
middle of table synchronization, the table synchronization will have to
start from scratch. That being said, in normal operation apply worker
should only exit/restart if subscription has changed or has been
dropped/disabled and I think sync workers want to exit/restart in that
situation as well.

So for example having shmem detach hook for an apply worker (or reusing
the existing one) that searches for all the other workers for same
subscription and shuts them down as well sounds like solution to this.

[1]
https://www.postgresql.org/message-id/cahgqgwhpi8ky-yanffe0sgmhktsycqltnkx07bw9s7-rn17...@mail.gmail.com

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

[HACKERS] alter table..drop constraint pkey, left not null un-dropped

[Rajkumar Raghuwanshi]

Hi All,

I have created a table with primary key, and then dropped primary key from
table. But table still have not null constraint added by primary key.

Is there any other statement to delete primary key with not null?
or this is an expected behaviour of pg?

postgres=# create table tbl (c1 int primary key);
CREATE TABLE
postgres=# \d+ tbl
Table "public.tbl"
 Column |  Type   | Collation | Nullable | Default | Storage | Stats target
| Description
+-+---+--+-+
-+--+-
 c1 | integer |   | not null | | plain   |
|
Indexes:
"tbl_pkey" PRIMARY KEY, btree (c1)

postgres=# alter table tbl drop constraint tbl_pkey;
ALTER TABLE
postgres=# \d+ tbl
Table "public.tbl"
 Column |  Type   | Collation | Nullable | Default | Storage | Stats target
| Description
+-+---+--+-+
-+--+-
 c1 | integer |   | not null | | plain   |
|


Thanks & Regards,
Rajkumar Raghuwanshi
QMG, EnterpriseDB Corporation

[HACKERS] feature wish: filter log_min_duration_statement according to the context (parse|bind|execute|...)

[Marc Mamin]

Hello,
setting log_min_duration_statement to 0 is usefull on test or development 
system, but this may lead to huge log files.
It would often  be useful to discard the parse and bind entries.
maybe a new parameter like "log_min_duration_execute"?
I don't have the skills to implement this though.


best regards,

Marc Mamin

Re: [HACKERS] Bug in pg_dump --table and --exclude-table for declarative partition table handling.

[Ashutosh Bapat]

On Thu, May 11, 2017 at 2:06 AM, Robert Haas  wrote:
> On Tue, May 9, 2017 at 4:21 AM, Jeevan Ladhe
>  wrote:
>>> Current pg_dump --exclude-table option excludes partitioned relation
>>> and dumps all its child relations and vice versa for --table option, which
>>> I think is incorrect.
>>>
>>> In this case we might need to explore all partitions and exclude or
>>> include
>>> from dump according to given pg_dump option, attaching WIP patch proposing
>>> same fix.   Thoughts/Comments?
>>
>> +1.
>>
>> Also, I can see similar issue exists with inheritance.
>
> That's a pretty insightful comment which makes me think that this
> isn't properly categorized as a bug.  Table partitioning is based on
> inheritance and is intended to behave like inheritance except when
> we've decided to make it behave otherwise.  We made no such decision
> in this case, so it behaves like inheritance in this case.  So, this
> is only a bug if the inheritance behavior is also a bug.

We have chosen inheritance as mechanism to implement partitioning, but
users will have different expectations from them. Partitioned table is
a "table" "containing" partitions. So, if we want to dump a table
which is partitioned, we better dump its partitions (which happen to
be tables by themselves) as  well. I don't think we can say that
inheritance parent contains its children, esp. thinking in the context
of multiple inheritance. IOW users would expect us to dump partitioned
table with its partitions and not just the shell.

In case of DROP TABLE  we do drop all the
partitions without specifying CASCADE. To drop an inheritance parent
we need CASCADE to drop its children. So, we have already started
diverging from inheritance.

>
> And I think there's a pretty good argument that it isn't.  Are we
> saying we think that it was always intended that dumping an
> inheritance parent would always dump its inheritance children as well,
> and the code accidentally failed to achieve that?  Are we saying we'd
> back-patch a behavior change in this area to correct the wrong
> behavior we've had since time immemorial?  I can't speak for anyone
> else, but I think the first is probably false and I would vote against
> the second.

I think the inheritance behaviour we have got, whether intentional or
unintentional, is acceptable since we do not consider an inheritance
parent alongwith its children a unit, esp. when multiple inheritance
exists.

>
> That's not to say that this isn't a possibly-useful behavior change.
> I can easily imagine that users would find it convenient to be able to
> dump a whole partitioning hierarchy by just selecting the parent table
> rather than having to list all of the partitions.  But that's also
> true of inheritance.

I agree that its useful behaviour for inheritance but I am not sure
that it's a "feature" for partitioned tables.

> Is partitioning different enough from
> inheritance that the two should have different behaviors, perhaps
> because the partitioning parent can't contain data but the inheritance
> parent could?

Yes, most users would see them as different things, esp. those who
come from other DBMS backgrounds.

> Or should we change the behavior for inheritance as
> well, on the theory that the proposed new behavior is just plain
> better than the current behavior and everyone will want it?

Not necessarily, as is the inheritance behaviour looks sane to me.

> Or should
> we do nothing at all, so as to avoid breaking things for the user who
> says they want to dump JUST the parent and really means it?  Even if
> the parent couldn't contain any rows, it's still got a schema that can
> be dumped.  The option of changing partitioning in one patch and then
> having a separate patch later that makes a similar change for
> inheritance seems like the worst option, since then users might get
> any of three behaviors depending on which release they have.  If we
> want to change this, let's change it all at once.  But first we need
> to get clarity on exactly what we're changing and for what reason.
>
> There is a question of timing.  If we accept that this is not a bug
> but a proposed behavior change, then it's not clear that it really
> qualifies as an open item for v10.  I understand that there's an urge
> to keep tinkering and making things better, but it's far from clear to
> me that anything bad will happen if we just postpone changing anything
> until v11, especially if we decide to change both partitioning and
> inheritance.  I am somewhat inclined to classify this proposed open
> item as a non-bug (i.e. a feature) but I'm also curious to hear what
> others think.

To me this looks like a bug, something to be fixed in v10 only for
partitioned tables. But we don't need to entangle that with
inheritance. Partitioned tables get dumped (or not dumped) as a whole
and inheritance parents as single units.

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company


-- 
Sent via pgsql-hackers maili

Re: [HACKERS] Bug in pg_dump --table and --exclude-table for declarative partition table handling.

[Ashutosh Bapat]

On Thu, May 11, 2017 at 3:05 AM, Tom Lane  wrote:

>
> We should make sure that pg_dump behaves sanely when dumping just
> some elements of a partition tree, of course.  And for that matter,
> pg_restore ought to be able to successfully restore just some elements
> out of a an archive containing more.
>

We add PARTITION OF clause for a table which is partition, so if the
parent is not present while restoring, the restore is going to fail.
-- 
Best Wishes,
Ashutosh Bapat
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] alter table..drop constraint pkey, left not null un-dropped

[Ashutosh Bapat]

On Thu, May 11, 2017 at 3:03 PM, Rajkumar Raghuwanshi
 wrote:
> Hi All,
>
> I have created a table with primary key, and then dropped primary key from
> table. But table still have not null constraint added by primary key.
>
> Is there any other statement to delete primary key with not null?
> or this is an expected behaviour of pg?
>
> postgres=# create table tbl (c1 int primary key);
> CREATE TABLE
> postgres=# \d+ tbl
> Table "public.tbl"
>  Column |  Type   | Collation | Nullable | Default | Storage | Stats target
> | Description
> +-+---+--+-+-+--+-
>  c1 | integer |   | not null | | plain   |
> |
> Indexes:
> "tbl_pkey" PRIMARY KEY, btree (c1)
>
> postgres=# alter table tbl drop constraint tbl_pkey;
> ALTER TABLE
> postgres=# \d+ tbl
> Table "public.tbl"
>  Column |  Type   | Collation | Nullable | Default | Storage | Stats target
> | Description
> +-+---+--+-+-+--+-
>  c1 | integer |   | not null | | plain   |
> |

I don't think we have a way to tell whether NOT NULL constraint was
added for primary key or independently. So, I guess, we can not just
drop it while dropping primary key constraint.

-- 
Best Wishes,
Ashutosh Bapat
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] Re: logical replication syntax (was DROP SUBSCRIPTION, query cancellations and slot handling)

[Petr Jelinek]

On 11/05/17 09:27, Noah Misch wrote:
> On Mon, May 08, 2017 at 05:01:00PM -0400, Peter Eisentraut wrote:
>> On 5/7/17 04:21, Noah Misch wrote:
>>> The above-described topic is currently a PostgreSQL 10 open item.  Peter,
>>> since you committed the patch believed to have created it, you own this open
>>> item.  If some other commit is more relevant or if this does not belong as a
>>> v10 open item, please let us know.  Otherwise, please observe the policy on
>>> open item ownership[1] and send a status update within three calendar days 
>>> of
>>> this message.  Include a date for your subsequent status update.  Testers 
>>> may
>>> discover new open items at any time, and I want to plan to get them all 
>>> fixed
>>> well in advance of shipping v10.  Consequently, I will appreciate your 
>>> efforts
>>> toward speedy resolution.  Thanks.
>>
>> I think we have a workable patch, but it needs some rebasing.  I hope to
>> have this sorted by Wednesday.
> 
> 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
> 

The patch Peter mentioned was committed.

There is however another open item associated with this thread for which
there is another patch as well.

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

[Amit Kapila]

On Fri, Mar 17, 2017 at 4:07 PM, Amit Khandekar  wrote:
> On 4 March 2017 at 12:49, Robert Haas  wrote:
>> On Thu, Mar 2, 2017 at 11:53 AM, Amit Khandekar  
>> wrote:
>>> I think it does not make sense running after row triggers in case of
>>> row-movement. There is no update happened on that leaf partition. This
>>> reasoning can also apply to BR update triggers. But the reasons for
>>> having a BR trigger and AR triggers are quite different. Generally, a
>>> user needs to do some modifications to the row before getting the
>>> final NEW row into the database, and hence [s]he defines a BR trigger
>>> for that. And we can't just silently skip this step only because the
>>> final row went into some other partition; in fact the row-movement
>>> itself might depend on what the BR trigger did with the row. Whereas,
>>> AR triggers are typically written for doing some other operation once
>>> it is made sure the row is actually updated. In case of row-movement,
>>> it is not actually updated.
>>
>> How about running the BR update triggers for the old partition and the
>> AR update triggers for the new partition?  It seems weird to run BR
>> update triggers but not AR update triggers.  Another option would be
>> to run BR and AR delete triggers and then BR and AR insert triggers,
>> emphasizing the choice to treat this update as a delete + insert, but
>> (as Amit Kh. pointed out to me when we were in a room together this
>> week) that precludes using the BEFORE trigger to modify the row.
>>

I also find the current behavior with respect to triggers quite odd.
The two points that appears odd are (a) Executing both before row
update and delete triggers on original partition sounds quite odd. (b)
It seems inconsistent to consider behavior for row and statement
triggers differently

>
> I checked the trigger behaviour in case of UPSERT. Here, when there is
> conflict found, ExecOnConflictUpdate() is called, and then the
> function returns immediately, which means AR INSERT trigger will not
> fire. And ExecOnConflictUpdate() calls ExecUpdate(), which means BR
> and AR UPDATE triggers will be fired. So in short, when an INSERT
> becomes an UPDATE, BR INSERT triggers do fire, but then the BR UPDATE
> and AR UPDATE also get fired. On the same lines, it makes sense in
> case of UPDATE=>DELETE+INSERT operation to fire a BR UPDATE trigger on
> the original table, and then the BR and AR DELETE/INSERT triggers on
> the respective tables.
>

I am not sure if it is good idea to compare it with "Insert On
Conflict Do Update", but  even if we want that way, I think Insert On
Conflict is consistent in statement level triggers which means it will
fire both Insert and Update statement level triggres (as per below
note in docs) whereas the documentation in the patch indicates that
this patch will only fire Update statement level triggers which is
odd.

Note in docs about Insert On Conflict
"Note that with an INSERT with an ON CONFLICT DO UPDATE clause, both
INSERT and UPDATE statement level trigger will be fired.



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

Re: [HACKERS] UPDATE of partition key

[Amit Kapila]

On Wed, May 3, 2017 at 11:22 AM, Amit Khandekar  wrote:
> On 2 May 2017 at 18:17, Robert Haas  wrote:
>> On Tue, Apr 4, 2017 at 7:11 AM, Amit Khandekar  
>> wrote:
>>> Attached updated patch v7 has the above changes.
>>
>
> Attached is the rebased patch, which resolves the above conflicts.
>

Few comments:
1.
Operating directly on partition doesn't allow update to move row.
Refer below example:
create table t1(c1 int) partition by range(c1);
create table t1_part_1 partition of t1 for values from (1) to (100);
create table t1_part_2 partition of t1 for values from (100) to (200);
insert into t1 values(generate_series(1,11));
insert into t1 values(generate_series(110,120));

postgres=# update t1_part_1 set c1=122 where c1=11;
ERROR:  new row for relation "t1_part_1" violates partition constraint
DETAIL:  Failing row contains (122).

2.
-
+static void ExecInitPartitionWithCheckOptions(ModifyTableState *mtstate,
+  Relation root_rel);

Spurious line delete.

3.
+   longer satisfy the partition constraint of the containing partition. In that
+   case, if there is some other partition in the partition tree for which this
+   row satisfies its partition constraint, then the row is moved to that
+   partition. If there isn't such a partition, an error will occur.

Doesn't this error case indicate that this needs to be integrated with
Default partition patch of Rahila or that patch needs to take care
this error case?
Basically, if there is no matching partition, then move it to default partition.

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

Re: [HACKERS] UPDATE of partition key

[Robert Haas]

On Thu, May 11, 2017 at 7:54 AM, Amit Kapila  wrote:
> Few comments:
> 1.
> Operating directly on partition doesn't allow update to move row.
> Refer below example:
> create table t1(c1 int) partition by range(c1);
> create table t1_part_1 partition of t1 for values from (1) to (100);
> create table t1_part_2 partition of t1 for values from (100) to (200);
> insert into t1 values(generate_series(1,11));
> insert into t1 values(generate_series(110,120));
>
> postgres=# update t1_part_1 set c1=122 where c1=11;
> ERROR:  new row for relation "t1_part_1" violates partition constraint
> DETAIL:  Failing row contains (122).

I think that's correct behavior.

-- 
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] Logical decoding truncate

[Friedrich, Steffen]

Hi,

I am writing a logical decoding output plugin decoding WAL to SQL which is 
finally applied to target database.

Is it possible to decode a TRUNCATE statement and the tables involved?
Assuming the SQL statement "TRUNCATE x, y;",  I am interested in decoding the 
operation TRUNCATE and the corresponding tables x and y so that I can 
reconstruct the SQL statement/transaction.
Is that possible?
If so, can you please provide an example or point me into the right direction?

I am currently looking at the structures provided to the commit callback e.g. 
ReorderBufferTXN but so far I have not been able to find the information I am 
looking for. What I found out is that the output plugin's begin and commit 
callbacks are called with has_catalog_changes=1. The change callback is not 
called for a TRUNCATE.

Thank you,
Steffen

WINCOR NIXDORF International GmbH
Sitz der Gesellschaft: Paderborn
Registergericht Paderborn HRB 3507
Geschäftsführer: Dr. Jürgen Wunram (Vorsitzender), Christopher A. Chapman, Olaf 
Heyden, Dr. Ulrich Näher, Rainer Pfeil
Vorsitzender des Aufsichtsrats: Dr. Alexander Dibelius
Steuernummer: 339/5884/0020 - Ust-ID Nr.: DE812927716 - WEEE-Reg.-Nr. DE44477193

Diese E-Mail enthält vertrauliche Informationen.
Wenn Sie nicht der richtige Adressat sind oder diese E-Mail irrtümlich erhalten 
haben,
informieren Sie bitte sofort den Absender und vernichten Sie diese E-Mail.
Das unerlaubte Kopieren sowie die unbefugte Weitergabe dieser E-Mail ist nicht 
gestattet.

This e-mail may contain confidential information.
If you are not the intended recipient (or have received this e-mail in error)
please notify the sender immediately and destroy this e-mail.
Any unauthorised copying, disclosure or distribution of the material in this 
e-mail is strictly forbidden.

Re: [HACKERS] feature wish: filter log_min_duration_statement according to the context (parse|bind|execute|...)

[Stephen Frost]

Greetings,

* Marc Mamin (m.ma...@intershop.de) wrote:
> setting log_min_duration_statement to 0 is usefull on test or development 
> system, but this may lead to huge log files.
> It would often  be useful to discard the parse and bind entries.
> maybe a new parameter like "log_min_duration_execute"?
> I don't have the skills to implement this though.

You might take a look at pgAudit, which allows this kind of filtering
based on what objects are being accessed.

The other option is to look at pg_stat_statements.

In general, I agree that it'd be awful nice to have something like this
built into core.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] UPDATE of partition key

[Amit Khandekar]

On 11 May 2017 at 17:23, Amit Kapila  wrote:
> On Fri, Mar 17, 2017 at 4:07 PM, Amit Khandekar  
> wrote:
>> On 4 March 2017 at 12:49, Robert Haas  wrote:
>>> On Thu, Mar 2, 2017 at 11:53 AM, Amit Khandekar  
>>> wrote:
 I think it does not make sense running after row triggers in case of
 row-movement. There is no update happened on that leaf partition. This
 reasoning can also apply to BR update triggers. But the reasons for
 having a BR trigger and AR triggers are quite different. Generally, a
 user needs to do some modifications to the row before getting the
 final NEW row into the database, and hence [s]he defines a BR trigger
 for that. And we can't just silently skip this step only because the
 final row went into some other partition; in fact the row-movement
 itself might depend on what the BR trigger did with the row. Whereas,
 AR triggers are typically written for doing some other operation once
 it is made sure the row is actually updated. In case of row-movement,
 it is not actually updated.
>>>
>>> How about running the BR update triggers for the old partition and the
>>> AR update triggers for the new partition?  It seems weird to run BR
>>> update triggers but not AR update triggers.  Another option would be
>>> to run BR and AR delete triggers and then BR and AR insert triggers,
>>> emphasizing the choice to treat this update as a delete + insert, but
>>> (as Amit Kh. pointed out to me when we were in a room together this
>>> week) that precludes using the BEFORE trigger to modify the row.
>>>
>
> I also find the current behavior with respect to triggers quite odd.
> The two points that appears odd are (a) Executing both before row
> update and delete triggers on original partition sounds quite odd.
Note that *before* trigger gets fired *before* the update happens. The
actual update may not even happen, depending upon what the trigger
does. And then in our case, the update does not happen; not just that,
it is transformed into delete-insert. So then we should fire
before-delete trigger.

> (b) It seems inconsistent to consider behavior for row and statement
> triggers differently

I am not sure whether we should compare row and statement triggers.
Statement triggers are anyway fired only per-statement, depending upon
whether it is update or insert or delete. It has nothing to do with
how the rows are modified.


>
>>
>> I checked the trigger behaviour in case of UPSERT. Here, when there is
>> conflict found, ExecOnConflictUpdate() is called, and then the
>> function returns immediately, which means AR INSERT trigger will not
>> fire. And ExecOnConflictUpdate() calls ExecUpdate(), which means BR
>> and AR UPDATE triggers will be fired. So in short, when an INSERT
>> becomes an UPDATE, BR INSERT triggers do fire, but then the BR UPDATE
>> and AR UPDATE also get fired. On the same lines, it makes sense in
>> case of UPDATE=>DELETE+INSERT operation to fire a BR UPDATE trigger on
>> the original table, and then the BR and AR DELETE/INSERT triggers on
>> the respective tables.
>>
>
> I am not sure if it is good idea to compare it with "Insert On
> Conflict Do Update", but  even if we want that way, I think Insert On
> Conflict is consistent in statement level triggers which means it will
> fire both Insert and Update statement level triggres (as per below
> note in docs) whereas the documentation in the patch indicates that
> this patch will only fire Update statement level triggers which is
> odd
>
> Note in docs about Insert On Conflict
> "Note that with an INSERT with an ON CONFLICT DO UPDATE clause, both
> INSERT and UPDATE statement level trigger will be fired.

I guess the reason this behaviour is kept for UPSERT, is because the
statement itself suggests : insert/update.


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

[Amit Khandekar]

On 11 May 2017 at 17:24, Amit Kapila  wrote:
> Few comments:
> 1.
> Operating directly on partition doesn't allow update to move row.
> Refer below example:
> create table t1(c1 int) partition by range(c1);
> create table t1_part_1 partition of t1 for values from (1) to (100);
> create table t1_part_2 partition of t1 for values from (100) to (200);
> insert into t1 values(generate_series(1,11));
> insert into t1 values(generate_series(110,120));
>
> postgres=# update t1_part_1 set c1=122 where c1=11;
> ERROR:  new row for relation "t1_part_1" violates partition constraint
> DETAIL:  Failing row contains (122).

Yes, as Robert said, this is expected behaviour. We move the row only
within the partition subtree that has the update table as its root. In
this case, it's the leaf partition.

>
> 3.
> +   longer satisfy the partition constraint of the containing partition. In 
> that
> +   case, if there is some other partition in the partition tree for which 
> this
> +   row satisfies its partition constraint, then the row is moved to that
> +   partition. If there isn't such a partition, an error will occur.
>
> Doesn't this error case indicate that this needs to be integrated with
> Default partition patch of Rahila or that patch needs to take care
> this error case?
> Basically, if there is no matching partition, then move it to default 
> partition.

Will have a look on this. Thanks for pointing this out.


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

[HACKERS] If subscription to foreign table valid ?

[tushar]

Hi,

I observed that -we cannot publish "foreign table" in Publication

postgres=# create foreign table t (n int) server db1_server options 
(table_name 't1');

CREATE FOREIGN TABLE

postgres=# create publication pub for table t;
ERROR:  "t" is not a table
DETAIL:  Only tables can be added to publications.
postgres=#

but same thing is not true for Subscription

postgres=# create foreign table t (n int) server db1_server options 
(table_name 't');

CREATE FOREIGN TABLE
postgres=# alter subscription sub refresh publication ;
NOTICE:  added subscription for table public.t
ALTER SUBSCRIPTION

Is this an expected behavior ?   if we cannot publish then how  can we  
add subscription for it.


--
regards,tushar
EnterpriseDB  https://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] alter table..drop constraint pkey, left not null un-dropped

[Tom Lane]

Rajkumar Raghuwanshi  writes:
> I have created a table with primary key, and then dropped primary key from
> table. But table still have not null constraint added by primary key.

> Is there any other statement to delete primary key with not null?
> or this is an expected behaviour of pg?

Yes, it's expected.  You can use "alter table t alter column c drop not
null" if you don't want the not-null constraint anymore.

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] Bug in pg_dump --table and --exclude-table for declarative partition table handling.

[Jeevan Ladhe]

On Thu, May 11, 2017 at 4:47 PM, Ashutosh Bapat <
ashutosh.ba...@enterprisedb.com> wrote:

>
> We add PARTITION OF clause for a table which is partition, so if the
> parent is not present while restoring, the restore is going to fail.


+1
But, similarly for inheritance if we dump a child table, it's dump is
broken as
of today. When we dump a child table we append "inherits(parenttab)" clause
at
the end of the DDL. Later when we try to restore this table on a database
not
having the parenttab, the restore fails.
So, I consider this as a bug.

Consider following example:

postgres=# create table tab1(a int, b int);
CREATE TABLE
postgres=# create table tab2(c int, d char) inherits(tab1);
CREATE TABLE
postgres=# \! pg_dump postgres -t tab2 > a.sql
postgres=# create database bkdb;
CREATE DATABASE
postgres=# \! psql bkdb < a.sql
SET
SET
SET
SET
SET
SET
SET
SET
SET
SET
SET
ERROR:  relation "tab1" does not exist
ERROR:  relation "tab2" does not exist
ERROR:  relation "tab2" does not exist
invalid command \.
postgres=#

Regards,
Jeevan Ladhe

Re: [HACKERS] Time based lag tracking for logical replication

[Simon Riggs]

On 11 May 2017 at 08:32, Noah Misch  wrote:
> On Sun, Apr 23, 2017 at 01:10:32AM +0200, Petr Jelinek wrote:
>> The time based lag tracking commit [1] added interface for logging
>> progress of replication so that we can report lag as time interval
>> instead of just bytes. But the patch didn't contain patch for the
>> builtin logical replication.
>>
>> So I wrote something that implements this.
>
> This is listed as a PostgreSQL 10 open item, but the above makes it sound like
> a feature to consider for v11, not a defect in v10.  Why is this an open item?

It's an open item because at the time of the Lag Tracker commit it was
believed to be a single line of code that needed to be added to
Logical Replication to make it work with the Lag Tracker
functionality. It didn't make sense for me to add it while the LR code
was still being changed, even though we had code to do that.

Petr's new patch is slightly longer and needed review and some minor
code to add pacing delay.

My own delay in responding has been because of illness. You'll note
that I'd missed response on at least one other mail from you.
Apologies for that, it has set me back some way but I'm better now and
have caught up with other matters. Petr nudged me to look at this
thread again yesterday, so I had been looking at this over last few
days.

Attached patch is Petr's patch, slightly rebased with added pacing
delay, similar to that used by HSFeedback.

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


Add-support-for-time-based-lag-tracking-to-logical-170429.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] Bug in pg_dump --table and --exclude-table for declarative partition table handling.

[Tom Lane]

Jeevan Ladhe  writes:
> On Thu, May 11, 2017 at 4:47 PM, Ashutosh Bapat <
>> We add PARTITION OF clause for a table which is partition, so if the
>> parent is not present while restoring, the restore is going to fail.

> +1
> But, similarly for inheritance if we dump a child table, it's dump is
> broken as
> of today. When we dump a child table we append "inherits(parenttab)" clause
> at
> the end of the DDL. Later when we try to restore this table on a database
> not
> having the parenttab, the restore fails.
> So, I consider this as a bug.

It sounds exactly what I'd expect.  In particular, given that pg_dump has
worked that way for inherited tables from the beginning, it's hard to see
any must-fix bugs here.

You could argue that it would be better for pg_dump to emit something
like

CREATE TABLE c (...);
ALTER TABLE c INHERIT p;

so that if p isn't around, at least c gets created.  And I think it
*would* be better, probably.  But if that isn't a new feature then
I don't know what is.  And partitioning really ought to track the
behavior of inheritance here.

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] Time based lag tracking for logical replication

[Petr Jelinek]

On 11/05/17 15:01, Simon Riggs wrote:
> On 11 May 2017 at 08:32, Noah Misch  wrote:
>> On Sun, Apr 23, 2017 at 01:10:32AM +0200, Petr Jelinek wrote:
>>> The time based lag tracking commit [1] added interface for logging
>>> progress of replication so that we can report lag as time interval
>>> instead of just bytes. But the patch didn't contain patch for the
>>> builtin logical replication.
>>>
>>> So I wrote something that implements this.
>>
>> This is listed as a PostgreSQL 10 open item, but the above makes it sound 
>> like
>> a feature to consider for v11, not a defect in v10.  Why is this an open 
>> item?
> 
> It's an open item because at the time of the Lag Tracker commit it was
> believed to be a single line of code that needed to be added to
> Logical Replication to make it work with the Lag Tracker
> functionality. It didn't make sense for me to add it while the LR code
> was still being changed, even though we had code to do that.
> 
> Petr's new patch is slightly longer and needed review and some minor
> code to add pacing delay.
> 
> My own delay in responding has been because of illness. You'll note
> that I'd missed response on at least one other mail from you.
> Apologies for that, it has set me back some way but I'm better now and
> have caught up with other matters. Petr nudged me to look at this
> thread again yesterday, so I had been looking at this over last few
> days.
> 
> Attached patch is Petr's patch, slightly rebased with added pacing
> delay, similar to that used by HSFeedback.
> 

This looks reasonable. I would perhaps change:
> +   /*
> +* Track lag no more than once per 
> WALSND_LOGICAL_LAG_TRACK_INTERVAL_MS
> +*/

to something like this for extra clarity:
> +   /*
> +* Track lag no more than once per 
> WALSND_LOGICAL_LAG_TRACK_INTERVAL_MS
> +* to avoid flooding the lag tracker on busy servers.
> +*/

-- 
  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] Bug in pg_dump --table and --exclude-table for declarative partition table handling.

[Robert Haas]

On Thu, May 11, 2017 at 9:02 AM, Tom Lane  wrote:
> Jeevan Ladhe  writes:
>> On Thu, May 11, 2017 at 4:47 PM, Ashutosh Bapat <
>>> We add PARTITION OF clause for a table which is partition, so if the
>>> parent is not present while restoring, the restore is going to fail.
>
>> +1
>> But, similarly for inheritance if we dump a child table, it's dump is
>> broken as
>> of today. When we dump a child table we append "inherits(parenttab)" clause
>> at
>> the end of the DDL. Later when we try to restore this table on a database
>> not
>> having the parenttab, the restore fails.
>> So, I consider this as a bug.
>
> It sounds exactly what I'd expect.  In particular, given that pg_dump has
> worked that way for inherited tables from the beginning, it's hard to see
> any must-fix bugs here.

I agree.

> You could argue that it would be better for pg_dump to emit something
> like
>
> CREATE TABLE c (...);
> ALTER TABLE c INHERIT p;
>
> so that if p isn't around, at least c gets created.  And I think it
> *would* be better, probably.  But if that isn't a new feature then
> I don't know what is.  And partitioning really ought to track the
> behavior of inheritance here.

Hmm, I think that'd actually be worse, because it would break the use
case where you want to restore the old contents of one particular
inheritance child.  So you drop that child (but not the parent or any
other children) and then do a selective restore of that one child.
Right now that works fine, but it'll fail with an error if we try to
create the already-extant parent.

I think one answer to the original complaint might be to add a new
flag to pg_dump, something like --recursive-selection, maybe -r for
short, which makes --table, --exclude-table, and --exclude-table-data
cascade to inheritance descendents.  Then if you want to dump your
partition table's definition without picking up the partitions, you
can say pg_dump -t splat, and if you want the children as well you can
say pg_dump -r -t splat.

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


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

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

[Tom Lane]

Heikki Linnakangas  writes:
> I updated the List of Drivers in the Wiki. I added a few drivers that 
> were missing, like the ODBC driver, and the pgtclng driver, as well as a 
> Go and Rust driver that I'm aware of. I reformatted it, and added a 
> column to indicate whether each driver uses libpq or not.

> https://wiki.postgresql.org/wiki/List_of_drivers

> There is a similar list in our docs:

> https://www.postgresql.org/docs/devel/static/external-interfaces.html

> Should we update the list in the docs, adding every driver we know of? 
> Or curate the list somehow, adding only more popular drivers? Or perhaps 
> add a link to the Wiki page from the docs?

Certainly, if that list is out of date, we need to do something about it.

ISTM that the list probably doesn't change fast enough that tracking it
in our SGML docs is a huge problem.  I think it'd likely be good enough
to just add the missing drivers to the list in the docs.  (If you do,
please back-patch that.)

> We can use this list in the Wiki to track which drivers implement SCRAM.

+1.  That *is* something likely to change faster than we can update
the SGML docs.

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] Re: transition table behavior with inheritance appears broken (was: Declarative partitioning - another take)

[Robert Haas]

On Thu, May 11, 2017 at 3:38 AM, Noah Misch  wrote:
> This PostgreSQL 10 open item is past due for your status update.  Kindly send
> a status update within 24 hours, and include a date for your subsequent status
> update.  Refer to the policy on open item ownership:
> https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com

Kevin has not posted to this mailing list since May 3rd.  I don't know
whether he's gone on vacation or been without email access for some
other reason, but I think we'd better assume that he's not likely to
respond to emails demanding immediate action regardless of how many of
them we send.

I'm not prepared to write a patch for this issue, but it seems like
Thomas is on top of that.  If nobody else steps up to the plate I
guess I'm willing to take responsibility for reviewing and committing
that patch once it's in final form, but at this point I don't think
it's going to be possible to get that done before Monday's planned
wrap.

In formal terms, if Kevin forfeits ownership of this item and nobody
else volunteers to adopt it, put me down as owner with a next-update
date of Friday, May 19th, the day after beta1 is expected to ship.

-- 
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] [PATCH] New command to monitor progression of long running queries

[Remi Colinet]

Do you have more details about the failed tests?

Regards,
Remi

2017-05-06 5:38 GMT+02:00 Vinayak Pokale :

>
> On Mon, Apr 17, 2017 at 9:09 PM, Remi Colinet 
> wrote:
>
>> Hello,
>>
>> I've implemented a new command named PROGRESS to monitor progression of
>> long running SQL queries in a backend process.
>>
>> Thank you for the patch.
>
> I am testing your patch but after applying your patch other regression
> test failed.
>
> $ make installcheck
> 13 of 178 tests failed.
>
> Regards,
> Vinayak
>

Re: [HACKERS] If subscription to foreign table valid ?

[Robert Haas]

On Thu, May 11, 2017 at 8:25 AM, tushar  wrote:
> I observed that -we cannot publish "foreign table" in Publication
>
> postgres=# create foreign table t (n int) server db1_server options
> (table_name 't1');
> CREATE FOREIGN TABLE
>
> postgres=# create publication pub for table t;
> ERROR:  "t" is not a table
> DETAIL:  Only tables can be added to publications.
> postgres=#
>
> but same thing is not true for Subscription
>
> postgres=# create foreign table t (n int) server db1_server options
> (table_name 't');
> CREATE FOREIGN TABLE
> postgres=# alter subscription sub refresh publication ;
> NOTICE:  added subscription for table public.t
> ALTER SUBSCRIPTION
>
> Is this an expected behavior ?   if we cannot publish then how  can we  add
> subscription for it.

This is not a complete test case, but it does appear to be odd behavior.

-- 
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] Bug in pg_dump --table and --exclude-table for declarative partition table handling.

[Tom Lane]

Robert Haas  writes:
> On Thu, May 11, 2017 at 9:02 AM, Tom Lane  wrote:
>> You could argue that it would be better for pg_dump to emit something
>> like
>> 
>> CREATE TABLE c (...);
>> ALTER TABLE c INHERIT p;
>> 
>> so that if p isn't around, at least c gets created.  And I think it
>> *would* be better, probably.  But if that isn't a new feature then
>> I don't know what is.  And partitioning really ought to track the
>> behavior of inheritance here.

> Hmm, I think that'd actually be worse, because it would break the use
> case where you want to restore the old contents of one particular
> inheritance child.  So you drop that child (but not the parent or any
> other children) and then do a selective restore of that one child.
> Right now that works fine, but it'll fail with an error if we try to
> create the already-extant parent.

Uh ... what in that is creating the already-extant parent?  What
I'm envisioning is simply that the TOC entry for the child table
contains those two statements rather than "CREATE TABLE c (...)
INHERITS (p)".  The equivalent thing for the partitioned case is
to use a separate ATTACH PARTITION command rather than naming the
parent immediately in the child's CREATE TABLE.  This is independent
of the question of how --table and friends ought to behave.

> I think one answer to the original complaint might be to add a new
> flag to pg_dump, something like --recursive-selection, maybe -r for
> short, which makes --table, --exclude-table, and --exclude-table-data
> cascade to inheritance descendents.

Yeah, you could do it like that.  Another way to do it would be to
create variants of all the selection switches, along the lines of
"--table-all=foo" meaning "foo plus its children".  Then you could
have some switches recursing and others not within the same command.
But maybe that's more flexibility than needed ... and I'm having a
hard time coming up with nice switch names, anyway.

Anyway, I'm still of the opinion that it's fine to leave this as a
future feature.  If we've gotten away without it this long for
inherited tables, it's unlikely to be critical for partitioned tables.

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] [PATCH] New command to monitor progression of long running queries

[Remi Colinet]

That's a good point.

A command is more straightforward because it targets only one backend.
The user is supposed to know which backend pid is taking a long time to
complete based on pg_stat_activity().

This is somehow the same approach as EXPLAIN command.
But the use is limited to psql utility. And this adds one more command.

I see 2 possible choices:

1 - either convert the command into a table.
This is the way it is done on Oracle database with v$session_longops view.
Obviously, this requires probing the status of each backend. This
inconvenient can be mitigated by using a threeshold of a few seconds before
considering a backend progression. v$session_longops only reports long
running queries after at least 6 seconds of execution.
This is less efficient that targeting directly a given pid or backend id.
But this is far better for SQL.

2 - either convert the command into a function
The advantage of a function is that it can accept parameters. So parameters
could be the pid of the backend, the verbosity level, the format (text,
json, ).
This would not reduce the options of the current command. And then a view
could be created on top of the function.


May be a mix of both a function with parameters and a view created on the
function is the solution.

Regards
Remi

2017-05-06 5:57 GMT+02:00 Jaime Casanova :

> On 5 May 2017 at 22:38, Vinayak Pokale  wrote:
> >
> > On Mon, Apr 17, 2017 at 9:09 PM, Remi Colinet 
> > wrote:
> >>
> >> Hello,
> >>
> >> I've implemented a new command named PROGRESS to monitor progression of
> >> long running SQL queries in a backend process.
> >>
> > Thank you for the patch.
> >
>
> sorry if i'm bikeshedding to soon but... why a command instead of a
> function?
> something like pg_progress_backend() will be in sync with
> pg_cancel_backend()/pg_terminate_backend() and the result of such a
> function could be usable by a tool to examine a slow query status
>
> --
> Jaime Casanova  www.2ndQuadrant.com
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>

Re: [HACKERS] If subscription to foreign table valid ?

[Petr Jelinek]

Hi,

On 11/05/17 14:25, tushar wrote:
> Hi,
> 
> I observed that -we cannot publish "foreign table" in Publication
> 
> postgres=# create foreign table t (n int) server db1_server options
> (table_name 't1');
> CREATE FOREIGN TABLE
> 
> postgres=# create publication pub for table t;
> ERROR:  "t" is not a table
> DETAIL:  Only tables can be added to publications.
> postgres=#
> 
> but same thing is not true for Subscription
> 
> postgres=# create foreign table t (n int) server db1_server options
> (table_name 't');
> CREATE FOREIGN TABLE
> postgres=# alter subscription sub refresh publication ;
> NOTICE:  added subscription for table public.t
> ALTER SUBSCRIPTION
> 
> Is this an expected behavior ?   if we cannot publish then how  can we 
> add subscription for it.
> 

Thanks for report. What you can publish and what you can subscribe is
not necessarily same (we can write to relations which we can't capture
from wal, for example unlogged table can't be published but can be
subscribed).

However, the foreign tables indeed can't be subscribed. I originally
planned to have foreign tables allowed on subscriber but it turned out
to be more complex to implement than I had anticipated do I ripped the
code for that from the original patch.

We do check for this, but only during replication which we have to do
because the fact that relation 't' was foreign table during ALTER
SUBSCRIPTION does not mean that it won't be something else half hour later.

I think it does make sense to add check for this into CREATE/ALTER
SUBSCRIBER though so that user is informed immediately about the mistake
rather than by errors in the logs later.

I'll look into writing patch for this. I don't think it's beta blocker
though.

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

[Rahila Syed]

Hello,

Please find attached an updated patch with review comments and bugs
reported till date implemented.

>1.
>In following block, we can just do with def_index, and we do not need
found_def
>flag. We can check if def_index is -1 or not to decide if default partition
is
>present.
found_def is used to set boundinfo->has_default which is used at couple
of other places to check if default partition exists. The implementation is
similar
to has_null.

>3.
>In following function isDefaultPartitionBound, first statement "return
false"
>is not needed.
It is needed to return false if the node is not DefElem.

Todo:
Add regression tests
Documentation

Thank you,
Rahila Syed



On Fri, May 5, 2017 at 1:30 AM, Jeevan Ladhe 
wrote:

> Hi Rahila,
>
> I have started reviewing your latest patch, and here are my initial
> comments:
>
> 1.
> In following block, we can just do with def_index, and we do not need
> found_def
> flag. We can check if def_index is -1 or not to decide if default
> partition is
> present.
>
> @@ -166,6 +172,8 @@ RelationBuildPartitionDesc(Relation rel)
>   /* List partitioning specific */
>   PartitionListValue **all_values = NULL;
>   bool found_null = false;
> + bool found_def = false;
> + int def_index = -1;
>   int null_index = -1;
>
> 2.
> In check_new_partition_bound, in case of PARTITION_STRATEGY_LIST, remove
> following duplicate declaration of boundinfo, because it is confusing and
> after
> your changes it is not needed as its not getting overridden in the if block
> locally.
> if (partdesc->nparts > 0)
> {
> PartitionBoundInfo boundinfo = partdesc->boundinfo;
> ListCell   *cell;
>
>
> 3.
> In following function isDefaultPartitionBound, first statement "return
> false"
> is not needed.
>
> + * Returns true if the partition bound is default
> + */
> +bool
> +isDefaultPartitionBound(Node *value)
> +{
> + if (IsA(value, DefElem))
> + {
> + DefElem *defvalue = (DefElem *) value;
> + if(!strcmp(defvalue->defname, "DEFAULT"))
> + return true;
> + return false;
> + }
> + return false;
> +}
>
> 4.
> As mentioned in my previous set of comments, following if block inside a
> loop
> in get_qual_for_default needs a break:
>
> + foreach(cell1, bspec->listdatums)
> + {
> + Node *value = lfirst(cell1);
> + if (isDefaultPartitionBound(value))
> + {
> + def_elem = true;
> + *defid  = inhrelid;
> + }
> + }
>
> 5.
> In the grammar the rule default_part_list is not needed:
>
> +default_partition:
> + DEFAULT  { $$ = (Node *)makeDefElem("DEFAULT", NULL, @1); }
> +
> +default_part_list:
> + default_partition { $$ = list_make1($1); }
> + ;
> +
>
> Instead you can simply declare default_partition as a list and write it as:
>
> default_partition:
> DEFAULT
> {
> Node *def = (Node *)makeDefElem("DEFAULT", NULL, @1);
> $$ = list_make1(def);
> }
>
> 6.
> You need to change the output of the describe command, which is currently
> as below: postgres=# \d+ test; Table "public.test" Column | Type |
> Collation | Nullable | Default | Storage | Stats target | Description
> +-+---+--+-+-+--+-
> a | integer | | | | plain | | b | date | | | | plain | | Partition key:
> LIST (a) Partitions: pd FOR VALUES IN (DEFAULT), test_p1 FOR VALUES IN (4,
> 5) What about changing the Paritions output as below: Partitions: *pd
> DEFAULT,* test_p1 FOR VALUES IN (4, 5)
>
> 7.
> You need to handle tab completion for DEFAULT.
> e.g.
> If I partially type following command:
> CREATE TABLE pd PARTITION OF test DEFA
> and then press tab, I get following completion:
> CREATE TABLE pd PARTITION OF test FOR VALUES
>
> I did some primary testing and did not find any problem so far.
>
> I will review and test further and let you know my comments.
>
> Regards,
> Jeevan Ladhe
>
> On Thu, May 4, 2017 at 6:09 PM, Rajkumar Raghuwanshi <
> rajkumar.raghuwan...@enterprisedb.com> wrote:
>
>> On Thu, May 4, 2017 at 5:14 PM, Rahila Syed 
>> wrote:
>>
>>> The syntax implemented in this patch is as follows,
>>>
>>> CREATE TABLE p11 PARTITION OF p1 DEFAULT;
>>>
>>> Applied v9 patches, table description still showing old pattern of
>> default partition. Is it expected?
>>
>> create table lpd (a int, b int, c varchar) partition by list(a);
>> create table lpd_d partition of lpd DEFAULT;
>>
>> \d+ lpd
>>  Table "public.lpd"
>>  Column |   Type| Collation | Nullable | Default | Storage  |
>> Stats target | Description
>> +---+---+--+
>> -+--+--+-
>>  a  | integer   |   |  | | plain
>> |  |
>>  b  | integer   |   |  | | plain
>> |  |
>>  c  | character varying |   |  | | extended
>> |  |
>> Partition key: LIST (a)
>> Partitions: lpd_d FOR VALUES IN (DEFAULT)
>>
>
>


default_partition_v10.patch
Description: application/download

--

Re: [HACKERS] snapbuild woes

[Stas Kelvich]

> On 10 May 2017, at 11:43, Petr Jelinek  wrote:
> 
> On 09/05/17 22:11, Erik Rijkers wrote:
>> On 2017-05-09 21:00, Petr Jelinek wrote:
>>> On 09/05/17 19:54, Erik Rijkers wrote:
 On 2017-05-09 11:50, Petr Jelinek wrote:
 
>>> 
>>> Ah okay, so this is same issue that's reported by both Masahiko Sawada
>>> [1] and Jeff Janes [2].
>>> 
>>> [1]
>>> https://www.postgresql.org/message-id/CAD21AoBYpyqTSw%2B%3DES%2BxXtRGMPKh%3DpKiqjNxZKnNUae0pSt9bg%40mail.gmail.com
>>> 
>>> [2]
>>> https://www.postgresql.org/message-id/flat/CAMkU%3D1xUJKs%3D2etq2K7bmbY51Q7g853HLxJ7qEB2Snog9oRvDw%40mail.gmail.com
>>> 
>> 
>> I don't understand why you come to that conclusion: both Masahiko Sawada
>> and Jeff Janes have a DROP SUBSCRIPTION in the mix; my cases haven't. 
>> Isn't that a real difference?
>> 
> 

Just noted another one issue/feature with snapshot builder — when logical 
decoding is in progress
and vacuum full command is being issued quite a big amount of files appears in 
pg_logical/mappings
and reside there till the checkpoint. Also having consequent vacuum full’s 
before checkpoint yields even
more files.

Having two pgbench-filled instances with logical replication between them:

for i in {1..100}; do psql -c 'vacuum full' && ls -la pg_logical/mappings | wc 
-l; done;
VACUUM
  73
VACUUM
 454
VACUUM
1146
VACUUM
2149
VACUUM
3463
VACUUM
5088
<...>
VACUUM
   44708
<…> // checkpoint happens somewhere here
VACUUM
   20921
WARNING:  could not truncate file "base/16384/61773": Too many open files in 
system
ERROR:  could not fsync file 
"pg_logical/mappings/map-4000-4df-0_A4EA29F8-5aa5-5ae6": Too many open files in 
system


I’m not sure whether this is boils down to some of the previous issues 
mentioned here or not, so posting
here as observation.


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




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

Re: [HACKERS] Should pg_current_wal_location() become pg_current_wal_lsn()

[Tom Lane]

Michael Paquier  writes:
> On Thu, May 11, 2017 at 9:15 AM, Bruce Momjian  wrote:
>> On Wed, May 10, 2017 at 01:09:36PM -0700, Joe Conway wrote:
> On 05/10/2017 12:22 PM, Tom Lane wrote:
>>> Hm, well, anybody else want to vote?

>>> +1 for #2

>> Same, +1 for #2 (apply this patch)

> #1, do nothing.

OK, by my count we have

For patch:
Joe Conway
Stephen Frost
Kyotaro Horiguchi (*)
Petr Jelinek (*)
Tom Lane
Bruce Momjian
David Rowley
David Steele
Euler Taveira

For doing nothing:
Peter Eisentraut
Robert Haas
Michael Paquier

I think Kyotaro-san and Petr would have preferred standardizing
on "location" over "lsn", but they were definitely not for doing
nothing.  With or without their votes, it's pretty clear that the
ayes have it.

regards, tom lane


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

Re: [HACKERS] Should pg_current_wal_location() become pg_current_wal_lsn()

[Tom Lane]

Neha Khatri  writes:
> [In case forgotten] pg_controdata and pg_waldump interfaces should also be
> considered for this standardization.

> Following are pg_controldata interfaces that might require change:

>   Latest checkpoint location:
>   Prior checkpoint location:
>   Latest checkpoint's REDO location:
>   Minimum recovery ending location:
>   Backup start location:
>   Backup end location:

My inclination is to leave these messages alone.  They're not really
inconsistent with anything.  Where we seem to be ending up is that
"lsn" will be used in things like function and column names, but
documentation will continue to spell out phrases like "WAL location".

There is another open thread about converting said phrases to be
more consistent --- a lot of them currently say "transaction log
location", which is not a very good choice because it invites
confusion with pg_xact nee pg_clog.  But I think that's mostly
just documentation changes, and in any case it's better done as
a separate patch.

> The pg_waldump help options(and probably documentation) would also require
> change:
>   -e, --end=RECPTR   stop reading at log position RECPTR
>   -s, --start=RECPTR start reading at log position RECPTR

Yeah, probably s/log position/WAL location/ would be better here.
But again that seems like material for a separate patch.  The
current patch does do s/position/location/ in a few places, but
it's not trying to apply that policy everywhere.

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] snapbuild woes

[Petr Jelinek]

On 11/05/17 16:33, Stas Kelvich wrote:
> 
>> On 10 May 2017, at 11:43, Petr Jelinek  wrote:
>>
>> On 09/05/17 22:11, Erik Rijkers wrote:
>>> On 2017-05-09 21:00, Petr Jelinek wrote:
 On 09/05/17 19:54, Erik Rijkers wrote:
> On 2017-05-09 11:50, Petr Jelinek wrote:
>

 Ah okay, so this is same issue that's reported by both Masahiko Sawada
 [1] and Jeff Janes [2].

 [1]
 https://www.postgresql.org/message-id/CAD21AoBYpyqTSw%2B%3DES%2BxXtRGMPKh%3DpKiqjNxZKnNUae0pSt9bg%40mail.gmail.com

 [2]
 https://www.postgresql.org/message-id/flat/CAMkU%3D1xUJKs%3D2etq2K7bmbY51Q7g853HLxJ7qEB2Snog9oRvDw%40mail.gmail.com

>>>
>>> I don't understand why you come to that conclusion: both Masahiko Sawada
>>> and Jeff Janes have a DROP SUBSCRIPTION in the mix; my cases haven't. 
>>> Isn't that a real difference?
>>>
>>
> 
> Just noted another one issue/feature with snapshot builder — when logical 
> decoding is in progress
> and vacuum full command is being issued quite a big amount of files appears 
> in pg_logical/mappings
> and reside there till the checkpoint. Also having consequent vacuum full’s 
> before checkpoint yields even
> more files.
> 
> Having two pgbench-filled instances with logical replication between them:
> 
> for i in {1..100}; do psql -c 'vacuum full' && ls -la pg_logical/mappings | 
> wc -l; done;
> VACUUM
>   73
> VACUUM
>  454
> VACUUM
> 1146
> VACUUM
> 2149
> VACUUM
> 3463
> VACUUM
> 5088
> <...>
> VACUUM
>44708
> <…> // checkpoint happens somewhere here
> VACUUM
>20921
> WARNING:  could not truncate file "base/16384/61773": Too many open files in 
> system
> ERROR:  could not fsync file 
> "pg_logical/mappings/map-4000-4df-0_A4EA29F8-5aa5-5ae6": Too many open files 
> in system
> 
> 
> I’m not sure whether this is boils down to some of the previous issues 
> mentioned here or not, so posting
> here as observation.
> 

This has nothing to do with snapshot builder so this is not the thread
for it. See the comment section near the bottom of
src/backend/access/heap/rewriteheap.c for explanation of why it does
what it does.

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

[HACKERS] eval_const_expresisions and ScalarArrayOpExpr

[Heikki Linnakangas]

Eval_const_expressions() doesn't know about ScalarArrayOpExpr. We 
simplify the arguments, but if all the arguments are booleans, we don't 
take the obvious step of replacing the whole expression with a boolean 
Const. For example:


postgres=# explain select * from foo where 1 IN (1,2,3);
 QUERY PLAN
-
 Result  (cost=0.00..35.50 rows=2550 width=4)
   One-Time Filter: (1 = ANY ('{1,2,3}'::integer[]))
   ->  Seq Scan on foo  (cost=0.00..35.50 rows=2550 width=4)
(3 rows)

I would've expected that to be fully evaluated at plan-time, and 
optimized away.


That seems like an oversight. I guess that scenario doesn't happen very 
often in practice, but there's no reason not to do it when it does. 
Patch attached.


On a side-note, I find it a bit awkward that ScalarArrayOpExpr uses a 
2-element List to hold the scalar and array arguments. Wouldn't it be 
much more straightforward to have explicit "Expr *scalararg" and "Expr 
*arrayarg" fields?


- Heikki



0001-Const-eval-ScalarArrayOpExprs.patch
Description: invalid/octet-stream

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

Re: [HACKERS] Time based lag tracking for logical replication

[Simon Riggs]

On 11 May 2017 at 14:12, Petr Jelinek  wrote:

>> Attached patch is Petr's patch, slightly rebased with added pacing
>> delay, similar to that used by HSFeedback.
>>
>
> This looks reasonable. I would perhaps change:
>> +   /*
>> +* Track lag no more than once per 
>> WALSND_LOGICAL_LAG_TRACK_INTERVAL_MS
>> +*/
>
> to something like this for extra clarity:
>> +   /*
>> +* Track lag no more than once per 
>> WALSND_LOGICAL_LAG_TRACK_INTERVAL_MS
>> +* to avoid flooding the lag tracker on busy servers.
>> +*/

New patch, v3.

Applying in 90 minutes, barring objections.

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


Add-support-for-time-based-lag-tracking-to-logical-170429.v3.patch
Description: Binary data

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

Re: [HACKERS] Cached plans and statement generalization

[Bruce Momjian]

On Wed, May 10, 2017 at 07:11:07PM +0300, Konstantin Knizhnik wrote:
> I am going to continue work on this patch I will be glad to receive any
> feedback and suggestions for its improvement.
> In most cases, applications are not accessing Postgres directly, but using
> some connection pooling layer and so them are not able to use prepared
> statements.
> But at simple OLTP Postgres spent more time on building query plan than on
> execution itself. And it is possible to speedup Postgres about two times at
> such workload!
> Another alternative is true shared plan cache.  May be it is even more
> perspective approach, but definitely much more invasive and harder to
> implement.

Can we back up and get an overview of what you are doing and how you are
doing it?  Our TODO list suggests this order for successful patches:

Desirability -> Design -> Implement -> Test -> Review -> Commit

You kind of started at the Implementation/patch level, which makes it
hard to evaluate.

I think everyone agrees on the Desirability of the feature, but the
Design is the tricky part.  I think the design questions are:

*  What information is stored about cached plans?
*  How are the cached plans invalidated?
*  How is a query matched against a cached plan?

Looking at the options, ideally the plan would be cached at the same
query stage as the stage where the incoming query is checked against the
cache.  However, caching and checking at the same level offers no
benefit, so they are going to be different.  For example, caching a
parse tree at the time it is created, then checking at the same point if
the incoming query is the same doesn't help you because you already had
to create the parse tree get to that point.

A more concrete example is prepared statements.  They are stored at the
end of planning and matched in the parser.  However, you can easily do
that since the incoming query specifies the name of the prepared query,
so there is no trick to matching.

The desire is to cache as late as possible so you cache more work and
you have more detail about the referenced objects, which helps with
cache invalidation.  However, you also want to do cache matching as
early as possible to improve performance.

So, let's look at some options.  One interesting idea from Doug Doole
was to do it between the tokenizer and parser.  I think they are glued
together so you would need a way to run the tokenizer separately and
compare that to the tokens you stored for the cached plan.  The larger
issue is that prepared plans already are checked after parsing, and we
know they are a win, so matching any earlier than that just seems like
overkill and likely to lead to lots of problems.

So, you could do it after parsing but before parse-analysis, which is
kind of what prepared queries do.  One tricky problem is that we don't
bind the query string tokens to database objects until after parse
analysis.

Doing matching before parse-analysis is going to be tricky, which is why
there are so many comments about the approach.  Changing search_path can
certainly affect it, but creating objects in earlier-mentioned schemas
can also change how an object reference in a query is resolved.  Even
obscure things like the creation of a new operator that has higher
precedence in the query could change the plan, though am not sure if
our prepared query system even handles that properly.

Anyway, that is my feedback.  I would like to get an overview of what
you are trying to do and the costs/benefits of each option so we can
best guide you.

-- 
  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] export import bytea from psql

[Daniel Verite]

Pavel Stehule wrote:

> It is similar to my first or second proposal - rejected by Tom :(

Doesn't it differ? ISTM that in the patches/discussion related to:
https://commitfest.postgresql.org/11/787/
it was proposed to change \set in one way or another,
and also in the point #1 of this present thread:

> > 1. using psql variables - we just need to write commands \setfrom
> > \setfrombf - this should be very simple implementation. The value can be
> > used more times. On second hand - the loaded variable can hold lot of
> > memory (and it can be invisible for user).


My comment is: let's not change \set or even create a variant
of it just for importing verbatim file contents, in text or binary,
because interpolating psql variables differently in the query itself
is all we need.


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


-- 
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] eval_const_expresisions and ScalarArrayOpExpr

[Tom Lane]

Heikki Linnakangas  writes:
> Eval_const_expressions() doesn't know about ScalarArrayOpExpr.
> ...
> That seems like an oversight. I guess that scenario doesn't happen very 
> often in practice, but there's no reason not to do it when it does. 
> Patch attached.

Yeah, I think it's a lack-of-round-tuits situation.  Your patch reminds
me of a more ambitious attempt I made awhile back to reduce the amount
of duplicative boilerplate in eval_const_expressions.  I think I wrote
it during last year's beta period, stashed it because I didn't feel like
arguing for applying it right then, and then forgot about it.  Blowing
the dust off, it's attached.  It fixes ArrayRef and RowExpr as well as
ScalarArrayOpExpr, with a net growth of only 20 lines (largely comments).

> On a side-note, I find it a bit awkward that ScalarArrayOpExpr uses a 
> 2-element List to hold the scalar and array arguments. Wouldn't it be 
> much more straightforward to have explicit "Expr *scalararg" and "Expr 
> *arrayarg" fields?

I think it's modeled on OpExpr, which also uses a list though you could
argue for separate leftarg and rightarg fields instead.

regards, tom lane

diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c
index a1dafc8..95489f4 100644
*** a/src/backend/optimizer/util/clauses.c
--- b/src/backend/optimizer/util/clauses.c
*** static List *find_nonnullable_vars_walke
*** 115,120 
--- 115,123 
  static bool is_strict_saop(ScalarArrayOpExpr *expr, bool falseOK);
  static Node *eval_const_expressions_mutator(Node *node,
  			   eval_const_expressions_context *context);
+ static bool contain_non_const_walker(Node *node, void *context);
+ static bool ece_function_is_safe(Oid funcid,
+ 	 eval_const_expressions_context *context);
  static List *simplify_or_arguments(List *args,
  	  eval_const_expressions_context *context,
  	  bool *haveNull, bool *forceTrue);
*** estimate_expression_value(PlannerInfo *r
*** 2443,2448 
--- 2446,2482 
  	return eval_const_expressions_mutator(node, &context);
  }
  
+ /*
+  * The generic case in eval_const_expressions_mutator is to recurse using
+  * expression_tree_mutator, which will copy the given node unchanged but
+  * const-simplify its arguments (if any) as far as possible.  If the node
+  * itself does immutable processing, and each of its arguments were reduced
+  * to a Const, we can then reduce it to a Const using evaluate_expr.  (Some
+  * node types need more complicated logic; for example, a CASE expression
+  * might be reducible to a constant even if not all its subtrees are.)
+  */
+ #define ece_generic_processing(node) \
+ 	expression_tree_mutator((Node *) (node), eval_const_expressions_mutator, \
+ 			(void *) context)
+ 
+ /*
+  * Check whether all arguments of the given node were reduced to Consts.
+  * By going directly to expression_tree_walker, contain_non_const_walker
+  * is not applied to the node itself, only to its children.
+  */
+ #define ece_all_arguments_const(node) \
+ 	(!expression_tree_walker((Node *) (node), contain_non_const_walker, NULL))
+ 
+ /* Generic macro for applying evaluate_expr */
+ #define ece_evaluate_expr(node) \
+ 	((Node *) evaluate_expr((Expr *) (node), \
+ 			exprType((Node *) (node)), \
+ 			exprTypmod((Node *) (node)), \
+ 			exprCollation((Node *) (node
+ 
+ /*
+  * Recursive guts of eval_const_expressions/estimate_expression_value
+  */
  static Node *
  eval_const_expressions_mutator(Node *node,
  			   eval_const_expressions_context *context)
*** eval_const_expressions_mutator(Node *nod
*** 2758,2763 
--- 2792,2816 
  newexpr->location = expr->location;
  return (Node *) newexpr;
  			}
+ 		case T_ScalarArrayOpExpr:
+ 			{
+ ScalarArrayOpExpr *saop;
+ 
+ /* Copy the node and const-simplify its arguments */
+ saop = (ScalarArrayOpExpr *) ece_generic_processing(node);
+ 
+ /* Make sure we know underlying function */
+ set_sa_opfuncid(saop);
+ 
+ /*
+  * If all arguments are Consts, and it's a safe function, we
+  * can fold to a constant
+  */
+ if (ece_all_arguments_const(saop) &&
+ 	ece_function_is_safe(saop->opfuncid, context))
+ 	return ece_evaluate_expr(saop);
+ return (Node *) saop;
+ 			}
  		case T_BoolExpr:
  			{
  BoolExpr   *expr = (BoolExpr *) node;
*** eval_const_expressions_mutator(Node *nod
*** 2982,3022 
  			}
  		case T_ArrayCoerceExpr:
  			{
! ArrayCoerceExpr *expr = (ArrayCoerceExpr *) node;
! Expr	   *arg;
! ArrayCoerceExpr *newexpr;
! 
! /*
!  * Reduce constants in the ArrayCoerceExpr's argument, then
!  * build a new ArrayCoerceExpr.
!  */
! arg = (Expr *) eval_const_expressions_mutator((Node *) expr->arg,
! 			  context);
  
! newexpr = makeNode(ArrayCoerceExpr);
! newexpr->arg = arg;
! newexpr->elemfuncid = expr->elemfuncid

Re: [HACKERS] [PATCH v2] Progress command to monitor progression of long running SQL queries

[Remi Colinet]

That's good point.

I will probably convert the new command to a SQL function.

I was fumbling between a table or a SQL function. Oracle uses
v$session_longops to track progression of long running SQL queries which
have run for more than 6 seconds.
But a function is much better to provide parameters such as the backend pid
and a format specification for the output.

Regards
Remi

2017-05-10 21:52 GMT+02:00 David Fetter :

> On Wed, May 10, 2017 at 06:40:31PM +0200, Remi Colinet wrote:
> > Hello,
> >
> > This is version 2 of the new command name PROGRESS which I wrote in
> > order to monitor long running SQL queries in a Postgres backend
> > process.
>
> Perhaps I missed something important in the discussion, but was there
> a good reason that this isn't a function callable from SQL, i.e. not
> restricted to the psql client?
>
> 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
>

Re: [HACKERS] [BUGS] Concurrent ALTER SEQUENCE RESTART Regression

[Peter Eisentraut]

On 5/10/17 12:24, Andres Freund wrote:
> The issue isn't the strength, but that we currently have this weird
> hackery around open_share_lock():
> /*
>  * Open the sequence and acquire AccessShareLock if needed
>  *
>  * If we haven't touched the sequence already in this transaction,
>  * we need to acquire AccessShareLock.  We arrange for the lock to
>  * be owned by the top transaction, so that we don't need to do it
>  * more than once per xact.
>  */
> 
> This'd probably need to be removed, as we'd otherwise would get very
> weird semantics around aborted subxacts.

Can you explain in more detail what you mean by this?

-- 
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] renaming "transaction log"

[Tom Lane]

Peter Eisentraut  writes:
> Most documentation and error messages still uses the term "transaction
> log" to refer to the write-ahead log.  Here is a patch to rename that,
> which I think should be done, to match the xlog -> wal renaming in APIs.

This will need to be rebased over d10c626de, which made a few of the
same/similar changes but did not try to hit stuff that wasn't adjacent
to what it was changing anyway.  I'm +1 for the idea though.  I think
we should also try to standardize on the terminology "WAL location"
for LSNs, not variants such as "WAL position" or "log position".

regards, tom lane


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

Re: [HACKERS] [POC] hash partitioning

[Dilip Kumar]

On Wed, May 3, 2017 at 6:39 PM, amul sul  wrote:
> On Thu, Apr 27, 2017 at 1:42 AM, Robert Haas  wrote:
>
>>I spent some time today looking at these patches.  It seems like there
>>is some more work still needed here to produce something committable
>>regardless of which way we go, but I am inclined to think that Amul's
>>patch is a better basis for work going forward than Nagata-san's
>>patch. Here are some general comments on the two patches:
>
> Thanks for your time.
>
> [...]
>
>> - Neither patch contains any documentation updates, which is bad.
>
> Fixed in the attached version.

I have done an intial review of the patch and I have some comments.  I
will continue the review
and testing and report the results soon

-
Patch need to be rebased



if (key->strategy == PARTITION_STRATEGY_RANGE)
{
/* Disallow nulls in the range partition key of the tuple */
for (i = 0; i < key->partnatts; i++)
if (isnull[i])
ereport(ERROR,
(errcode(ERRCODE_NULL_VALUE_NOT_ALLOWED),
errmsg("range partition key of row contains null")));
}

We need to add PARTITION_STRATEGY_HASH as well, we don't support NULL
for hash also, right?


RangeDatumContent **content;/* what's contained in each range bound datum?
  * (see the above enum); NULL for list
  * partitioned tables */

This will be NULL for hash as well we need to change the comments.
-

  bool has_null; /* Is there a null-accepting partition? false
  * for range partitioned tables */
  int null_index; /* Index of the null-accepting partition; -1

Comments needs to be changed for these two members as well


+/* One bound of a hash partition */
+typedef struct PartitionHashBound
+{
+ int modulus;
+ int remainder;
+ int index;
+} PartitionHashBound;

It will good to add some comments to explain the structure members


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


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

Re: [HACKERS] [PATCH v2] Progress command to monitor progression of long running SQL queries

[Remi Colinet]

2017-05-11 4:05 GMT+02:00 Michael Paquier :

> On Thu, May 11, 2017 at 1:40 AM, Remi Colinet 
> wrote:
> > This is version 2 of the new command name PROGRESS which I wrote in
> order to
> > monitor long running SQL queries in a Postgres backend process.
>
> It would be a good idea to add this patch to the next commit fest if
> you are expecting some feedback:
> https://commitfest.postgresql.org/14/
> But please don't expect immediate feedback, the primary focus these
> days is to stabilize the upcoming release.
>

Yes, I understand. I will submit the patch to the commit fest once I will
have had a few feedbacks.
I already received some interesting suggestions such as using a SQL
function instead of a whole new command.

The purpose for the moment is to gather feedback and advise about such
feature.

>
> > New command justification
> > 
> >
> > [root@rco pg]# git diff --stat master..
> > [...]
> >  58 files changed, 5972 insertions(+), 2619 deletions(-)
>
> For your first patch, you may want something less... Challenging.
> Please note as well that it would be nice if you review other patches
> to get more familiar with the community process, it is expected that
> for each patch submitted, an equivalent amount of review is done.
> That's hard to measure but for a patch as large as that you will need
> to pick up at least one patch equivalent in difficulty.
>

Acked


> Regarding the patch, this is way to close to the progress facility
> already in place. So why don't you extend it for the executor? We can
> likely come up with something that can help, though I see the part
> where the plan run by a backend is shared among all processes as a
> major bottleneck in performance. You have at least three concepts
> different here:
> - Store plans run in shared memory to allow access to any other processes.
> - Allow a process to look at the plan run by another one with a SQL
> interface.
> - Track the progress of a running plan, via pg_stat_activity.
> All in all, I think that a new command is not adapted, and that you
> had better split each concept before implementing something
> over-engineered like the patch attached.
>

I initially had a look at the progress facility but this seemed to me very
far from the current goal.
Btw, I will reconsider it. Things may have changed.

The plan and its planstate are not permanently shared with other backends.
This would be useless and very costly.

As long as the PROGRESS command is not used, the patch has a very low
impact on the executor.
A few counters and flags are added/updated by the patch in the executor.
This is all what is needed to track progress of execution.

For instance in src/backend/executor/execProcnode.c, I've added:

ExecInitNode()
+ result->plan_rows = 0;

ExecProcNode()
+ node->plan_rows++;

ExecEndNode()
+ node->plan_rows = 0;

This is enough to compute the number of rows fetched by the plan at this
step of the plan.
A few counters have been added to track sorts, scans, hashes, tuple stores.

The disk space used by nodes is also traced. This shows disk resource
needed to complete a SQL query.
This was not initially a purpose of the patch, but I find this very handy.

Some debugging code will also be removed making the change in the executor
very small.
The code change in the executor is less than 50 lines and it is about 150
lines in the tuple sort/store code mainly to add functions to fetch sort
and store status.

The execution tree is dumped in shared memory only when requested by a
monitoring backend sending a signal to the monitored backend.
The monitoring backend first sends a signal which is noted by the backend
running the long SQL query.
Once the long SQL query can deal with the request (interrupt enabled), it
dumps its execution tree in shared memory and then follows up its
execution.

So the dump of the execution tree is done only when it is requested.
This does not seem to incur any performance hit. Or I missed something.

Once the concept is mature, I would split the patch in small consistent
parts to allow a smooth and clean merge.

Regards
Remi


> --
> Michael
>

Re: [HACKERS] [PATCH v2] Progress command to monitor progression of long running SQL queries

[Remi Colinet]

Thx for your comment.

Based on other comments, I will probably convert the command into a SQL
function.
This allows passing arguments such as the one which can be used in the
current command (VERBOSE, FORMAT).

This will avoid keyword collisions.

Regards
Remi

2017-05-10 22:04 GMT+02:00 Pavel Stehule :

> Hi
>
> 2017-05-10 18:40 GMT+02:00 Remi Colinet :
>
>> Hello,
>>
>> This is version 2 of the new command name PROGRESS which I wrote in order
>> to monitor long running SQL queries in a Postgres backend process.
>>
>
> This patch introduce lot of reserved keyword. Why? It can breaks lot of
> applications. Cannot you enhance EXPLAIN command?
>
> Regards
>
> Pavel
>
>

Re: [HACKERS] eval_const_expresisions and ScalarArrayOpExpr

[Heikki Linnakangas]

On 05/11/2017 06:21 PM, Tom Lane wrote:

Heikki Linnakangas  writes:

Eval_const_expressions() doesn't know about ScalarArrayOpExpr.
...
That seems like an oversight. I guess that scenario doesn't happen very
often in practice, but there's no reason not to do it when it does.
Patch attached.


Yeah, I think it's a lack-of-round-tuits situation.  Your patch reminds
me of a more ambitious attempt I made awhile back to reduce the amount
of duplicative boilerplate in eval_const_expressions.  I think I wrote
it during last year's beta period, stashed it because I didn't feel like
arguing for applying it right then, and then forgot about it.


Hmph, now we're almost in beta period again. :-(.


Blowing the dust off, it's attached. It fixes ArrayRef and RowExpr as
well as ScalarArrayOpExpr, with a net growth of only 20 lines
(largely comments).


Nice!


On a side-note, I find it a bit awkward that ScalarArrayOpExpr uses a
2-element List to hold the scalar and array arguments. Wouldn't it be
much more straightforward to have explicit "Expr *scalararg" and "Expr
*arrayarg" fields?


I think it's modeled on OpExpr, which also uses a list though you could
argue for separate leftarg and rightarg fields instead.


Yeah, I think that would be better for OpExpr, too. (For an unary 
operator, rightarg would be unused.)


- Heikki



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

[HACKERS] Safer and faster get_attstatsslot()

[Tom Lane]

Monday's round of security patches was a lot more exciting than I would
have liked, because code that worked fine for Peter and me failed
erratically in the buildfarm.  What eventually emerged was that I'd added
some missing free_attstatsslot() calls in rangetypes_selfuncs.c, and
naively copied the first argument (atttype) from the matching
get_attstatsslot() calls.  One of those atttype values was in fact
wrong for the slot in question; this had been missed for years because
get_attstatsslot() doesn't actually do anything with that argument.
I think that at one point we had, or at least in the original conception
intended to have, an Assert that the atttype matched the actual stats
array element type found in the pg_statistic row; but we had to remove
it because in some cases the type in pg_statistic is only
binary-compatible with the datatype the applied operator is expecting.

So the existing API for get_attstatsslot()/free_attstatsslot() is just
seriously bug-prone.  It would be better if the caller did not have
to supply any type information; indeed really what we'd want is for
get_attstatsslot() to pass back the actual type it found in pg_statistic.

I also realized as I looked at the code that it's exceedingly inefficient
if the array element type is pass-by-reference --- then it'll incur a
separate palloc, copy, and pfree for each element.  We'd be a lot better
off to copy the stats array as a whole, especially since that would come
for free in the probably-common case that the array has to be detoasted.
This code was written with very small stats targets in mind, like about
10, and it just doesn't look very good when you're imagining 1000 or more
entries in the stats array.

So attached is a proposed redesign that makes the API for
get_attstatsslot()/free_attstatsslot() simpler and hopefully more
foolproof.  I've not made any particular attempt to performance-test
it, but it really ought to be a significant win for pass-by-ref element
types.  It will add an array-copying step that wasn't there before when
the element type is pass-by-value and the array isn't toasted, but that
seems like an acceptable price.

BTW, this patch makes the skewColType and skewColTypmod fields of Hash
plan nodes unnecessary, as the only reason for them was to satisfy
get_attstatsslot().  I didn't remove them here but it would make sense
to do so.

Comments?  Is this something that'd be OK to push now, or had I better
sit on it till v11?  Being freshly burned, I kinda want to fix it now,
but I recognize that my judgment may be colored by that.

regards, tom lane

diff --git a/contrib/intarray/_int_selfuncs.c b/contrib/intarray/_int_selfuncs.c
index 9b4a22f..3d92025 100644
*** a/contrib/intarray/_int_selfuncs.c
--- b/contrib/intarray/_int_selfuncs.c
*** _int_matchsel(PG_FUNCTION_ARGS)
*** 136,146 
  	int			nmcelems = 0;
  	float4		minfreq = 0.0;
  	float4		nullfrac = 0.0;
! 	Form_pg_statistic stats;
! 	Datum	   *values = NULL;
! 	int			nvalues = 0;
! 	float4	   *numbers = NULL;
! 	int			nnumbers = 0;
  
  	/*
  	 * If expression is not "variable @@ something" or "something @@ variable"
--- 136,142 
  	int			nmcelems = 0;
  	float4		minfreq = 0.0;
  	float4		nullfrac = 0.0;
! 	AttStatsSlot sslot;
  
  	/*
  	 * If expression is not "variable @@ something" or "something @@ variable"
*** _int_matchsel(PG_FUNCTION_ARGS)
*** 193,198 
--- 189,196 
  	 */
  	if (HeapTupleIsValid(vardata.statsTuple))
  	{
+ 		Form_pg_statistic stats;
+ 
  		stats = (Form_pg_statistic) GETSTRUCT(vardata.statsTuple);
  		nullfrac = stats->stanullfrac;
  
*** _int_matchsel(PG_FUNCTION_ARGS)
*** 200,228 
  		 * For an int4 array, the default array type analyze function will
  		 * collect a Most Common Elements list, which is an array of int4s.
  		 */
! 		if (get_attstatsslot(vardata.statsTuple,
! 			 INT4OID, -1,
  			 STATISTIC_KIND_MCELEM, InvalidOid,
! 			 NULL,
! 			 &values, &nvalues,
! 			 &numbers, &nnumbers))
  		{
  			/*
  			 * There should be three more Numbers than Values, because the
  			 * last three (for intarray) cells are taken for minimal, maximal
  			 * and nulls frequency. Punt if not.
  			 */
! 			if (nnumbers == nvalues + 3)
  			{
  /* Grab the lowest frequency. */
! minfreq = numbers[nnumbers - (nnumbers - nvalues)];
  
! mcelems = values;
! mcefreqs = numbers;
! nmcelems = nvalues;
  			}
  		}
  	}
  
  	/* Process the logical expression in the query, using the stats */
  	selec = int_query_opr_selec(GETQUERY(query) + query->size - 1,
--- 198,227 
  		 * For an int4 array, the default array type analyze function will
  		 * collect a Most Common Elements list, which is an array of int4s.
  		 */
! 		if (get_attstatsslot(&sslot, vardata.statsTuple,
  			 STATISTIC_KIND_MCELEM, InvalidOid,
! 			 ATTSTATSSLOT_VALUES | ATTSTATSSLOT_NUMBERS))
  		{
+ 			Assert(sslot.valuetype == INT4OID);
+ 
  	

Re: [HACKERS] eval_const_expresisions and ScalarArrayOpExpr

[Tom Lane]

Heikki Linnakangas  writes:
> On 05/11/2017 06:21 PM, Tom Lane wrote:
>>> On a side-note, I find it a bit awkward that ScalarArrayOpExpr uses a
>>> 2-element List to hold the scalar and array arguments. Wouldn't it be
>>> much more straightforward to have explicit "Expr *scalararg" and "Expr
>>> *arrayarg" fields?

>> I think it's modeled on OpExpr, which also uses a list though you could
>> argue for separate leftarg and rightarg fields instead.

> Yeah, I think that would be better for OpExpr, too. (For an unary 
> operator, rightarg would be unused.)

I should think leftarg is the one that would go unused, for a normal
prefix unary operator.

But it seems like changing this would be way more invasive than it's
really worth.  We'd save a couple of List cells per operator, which
is certainly something, but I'm afraid you'd be touching a heck of
a lot of code.  And there are a nontrivial number of places that
share argument-processing with FuncExprs, which such a change would
break.

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] export import bytea from psql

[Pavel Stehule]

2017-05-11 17:16 GMT+02:00 Daniel Verite :

> Pavel Stehule wrote:
>
> > It is similar to my first or second proposal - rejected by Tom :(
>
> Doesn't it differ? ISTM that in the patches/discussion related to:
> https://commitfest.postgresql.org/11/787/
> it was proposed to change \set in one way or another,
> and also in the point #1 of this present thread:
>
> > > 1. using psql variables - we just need to write commands \setfrom
> > > \setfrombf - this should be very simple implementation. The value can
> be
> > > used more times. On second hand - the loaded variable can hold lot of
> > > memory (and it can be invisible for user).
>
>
> My comment is: let's not change \set or even create a variant
> of it just for importing verbatim file contents, in text or binary,
> because interpolating psql variables differently in the query itself
> is all we need.
>

My memory is wrong - Tom didn't reject this idea


http://www.postgresql-archive.org/proposal-psql-setfileref-td5918727i20.html

There was 100% agreement on format and then discussion finished without any
result.

Regards

Pavel


>
>
> Best regards,
> --
> Daniel Vérité
> PostgreSQL-powered mailer: http://www.manitou-mail.org
> Twitter: @DanielVerite
>

Re: [HACKERS] Safer and faster get_attstatsslot()

[Simon Riggs]

On 11 May 2017 at 17:41, Tom Lane  wrote:
> ...because code that worked fine for Peter and me failed
> erratically in the buildfarm.

I think its always a little bit too exciting for me also.

I suggest we have a commit tree and a main tree, with automatic
copying from commit -> main either
1. 24 hours after commit
2. or earlier if we have a full set of green results from people
running the full suite on the commit tree

That way we don't have to polute the main tree with all this jumping around.

Many alternate ideas possible.

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


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

Re: [HACKERS] WITH clause in CREATE STATISTICS

[Alvaro Herrera]

Tom Lane wrote:

> Hmm ... I'm not sure that I buy that particular argument.  If you're
> concerned that the grammar could not handle "FROM x JOIN y USING (z)",
> wouldn't it also have a problem with "FROM x JOIN y ON (z)"?
> 
> It might work anyway, since the grammar should know whether ON or USING
> is needed to complete the JOIN clause.  But I think you'd better check
> whether the complete join syntax works there, even if we're not going
> to support it now.

Tomas spent some time trying to shoehorn the whole join syntax into the
FROM clause, but stopped once he realized that the joined_table
production uses table_ref, which allow things like TABLESAMPLE, SRFs,
LATERAL, etc which presumably we don't want to accept in CREATE STATS.
I didn't look into it any further.  But because of the other
considerations, I did end up changing the ON to FOR.

So the attached is the final version which I intend to push shortly.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
*** a/doc/src/sgml/perform.sgml
--- b/doc/src/sgml/perform.sgml
***
*** 1132,1138  WHERE tablename = 'road';
   To inspect functional dependencies on a statistics
   stts, you may do this:
  
! CREATE STATISTICS stts WITH (dependencies)
 ON (zip, city) FROM zipcodes;
  ANALYZE zipcodes;
  SELECT stxname, stxkeys, stxdependencies
--- 1132,1138 
   To inspect functional dependencies on a statistics
   stts, you may do this:
  
! CREATE STATISTICS stts (dependencies)
 ON (zip, city) FROM zipcodes;
  ANALYZE zipcodes;
  SELECT stxname, stxkeys, stxdependencies
***
*** 1219,1225  EXPLAIN (ANALYZE, TIMING OFF) SELECT * FROM t WHERE a = 1 
AND b = 10;
   Continuing the above example, the n-distinct coefficients in a ZIP
   code table may look like the following:
  
! CREATE STATISTICS stts2 WITH (ndistinct)
 ON (zip, state, city) FROM zipcodes;
  ANALYZE zipcodes;
  SELECT stxkeys AS k, stxndistinct AS nd
--- 1219,1225 
   Continuing the above example, the n-distinct coefficients in a ZIP
   code table may look like the following:
  
! CREATE STATISTICS stts2 (ndistinct)
 ON (zip, state, city) FROM zipcodes;
  ANALYZE zipcodes;
  SELECT stxkeys AS k, stxndistinct AS nd
*** a/doc/src/sgml/planstats.sgml
--- b/doc/src/sgml/planstats.sgml
***
*** 526,532  EXPLAIN (ANALYZE, TIMING OFF) SELECT * FROM t WHERE a = 1 AND 
b = 1;
  multivariate statistics on the two columns:
  
  
! CREATE STATISTICS stts WITH (dependencies) ON (a, b) FROM t;
  ANALYZE t;
  EXPLAIN (ANALYZE, TIMING OFF) SELECT * FROM t WHERE a = 1 AND b = 1;
QUERY PLAN  
 
--- 526,532 
  multivariate statistics on the two columns:
  
  
! CREATE STATISTICS stts (dependencies) ON (a, b) FROM t;
  ANALYZE t;
  EXPLAIN (ANALYZE, TIMING OFF) SELECT * FROM t WHERE a = 1 AND b = 1;
QUERY PLAN  
 
***
*** 569,575  EXPLAIN (ANALYZE, TIMING OFF) SELECT COUNT(*) FROM t GROUP BY 
a, b;
  calculation, the estimate is much improved:
  
  DROP STATISTICS stts;
! CREATE STATISTICS stts WITH (dependencies, ndistinct) ON (a, b) FROM t;
  ANALYZE t;
  EXPLAIN (ANALYZE, TIMING OFF) SELECT COUNT(*) FROM t GROUP BY a, b;
 QUERY PLAN 
   
--- 569,575 
  calculation, the estimate is much improved:
  
  DROP STATISTICS stts;
! CREATE STATISTICS stts (dependencies, ndistinct) ON (a, b) FROM t;
  ANALYZE t;
  EXPLAIN (ANALYZE, TIMING OFF) SELECT COUNT(*) FROM t GROUP BY a, b;
 QUERY PLAN 
   
*** a/doc/src/sgml/ref/create_statistics.sgml
--- b/doc/src/sgml/ref/create_statistics.sgml
***
*** 22,29  PostgreSQL documentation
   
  
  CREATE STATISTICS [ IF NOT EXISTS ] statistics_name
! WITH ( option [= 
value] [, ... ] )
! ON ( column_name, 
column_name [, ...])
  FROM table_name
  
  
--- 22,29 
   
  
  CREATE STATISTICS [ IF NOT EXISTS ] statistics_name
! [ ( statistic_type [, ... ] 
) ]
! FOR ( column_name, 
column_name [, ...])
  FROM table_name
  
  
***
*** 75,80  CREATE STATISTICS [ IF NOT EXISTS ] statistics_na
--- 75,93 
 
  
 
+ statistic_type
+ 
+  
+   A statistic type to be enabled for this statistics.  Currently
+   supported types are ndistinct, which enables
+   n-distinct coefficient tracking,
+   and dependencies, which enables functional
+   dependencies.
+  
+ 
+
+ 
+
  column_name
  
   
***
*** 94,135  CREATE STATISTICS [ IF NOT EXISTS ] statistics_na
 
  

- 
-   
-Parameters
- 
-  
-   st

Re: [HACKERS] Time based lag tracking for logical replication

[Andres Freund]

On May 11, 2017 8:08:11 AM PDT, Simon Riggs  wrote:
>On 11 May 2017 at 14:12, Petr Jelinek 
>wrote:
>
>>> Attached patch is Petr's patch, slightly rebased with added pacing
>>> delay, similar to that used by HSFeedback.
>>>
>>
>> This looks reasonable. I would perhaps change:
>>> +   /*
>>> +* Track lag no more than once per
>WALSND_LOGICAL_LAG_TRACK_INTERVAL_MS
>>> +*/
>>
>> to something like this for extra clarity:
>>> +   /*
>>> +* Track lag no more than once per
>WALSND_LOGICAL_LAG_TRACK_INTERVAL_MS
>>> +* to avoid flooding the lag tracker on busy servers.
>>> +*/
>
>New patch, v3.
>
>Applying in 90 minutes, barring objections.

Could you please wait till tomorrow?  I've bigger pending fixes for related 
code pending/being tested that I plan to push today.  I'd also like to take a 
look before...

Thanks,

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] WITH clause in CREATE STATISTICS

[Tom Lane]

Alvaro Herrera  writes:
> Tom Lane wrote:
>> Hmm ... I'm not sure that I buy that particular argument.  If you're
>> concerned that the grammar could not handle "FROM x JOIN y USING (z)",
>> wouldn't it also have a problem with "FROM x JOIN y ON (z)"?

> Tomas spent some time trying to shoehorn the whole join syntax into the
> FROM clause, but stopped once he realized that the joined_table
> production uses table_ref, which allow things like TABLESAMPLE, SRFs,
> LATERAL, etc which presumably we don't want to accept in CREATE STATS.
> I didn't look into it any further.  But because of the other
> considerations, I did end up changing the ON to FOR.

Have you thought further about the upthread suggestion to just borrow
SELECT's syntax lock stock and barrel?  That is, it'd look something
like

CREATE STATISTICS name [(list of stats types)] expression-list FROM ...
[ WHERE ... ] [ WITH (options) ]

This would certainly support any FROM stuff we might want later.
Yeah, you'd need to reject any features you weren't supporting at
execution, but doing that would allow issuing a friendlier message than
"syntax error" anyway.

If you don't have the cycles for that, I'd be willing to look into it.

regards, tom lane


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

Re: [HACKERS] Safer and faster get_attstatsslot()

[Tom Lane]

Simon Riggs  writes:
> On 11 May 2017 at 17:41, Tom Lane  wrote:
>> ...because code that worked fine for Peter and me failed
>> erratically in the buildfarm.

> I think its always a little bit too exciting for me also.

> I suggest we have a commit tree and a main tree, with automatic
> copying from commit -> main either
> 1. 24 hours after commit
> 2. or earlier if we have a full set of green results from people
> running the full suite on the commit tree

Meh.  We don't really need that in normal development, and for security
patches there's still a problem: we don't want to wait around 24 hours
after the code is public.

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] WITH clause in CREATE STATISTICS

[Alvaro Herrera]

Tom Lane wrote:

> Have you thought further about the upthread suggestion to just borrow
> SELECT's syntax lock stock and barrel?  That is, it'd look something
> like
> 
> CREATE STATISTICS name [(list of stats types)] expression-list FROM ...
> [ WHERE ... ] [ WITH (options) ]
> 
> This would certainly support any FROM stuff we might want later.
> Yeah, you'd need to reject any features you weren't supporting at
> execution, but doing that would allow issuing a friendlier message than
> "syntax error" anyway.

Hmm, no, I hadn't looked at this angle.

> If you don't have the cycles for that, I'd be willing to look into it.

I'll give it a try today, and pass the baton if I'm unable to.

-- 
Á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] Time based lag tracking for logical replication

[Simon Riggs]

On 11 May 2017 at 18:13, Andres Freund  wrote:

>>New patch, v3.
>>
>>Applying in 90 minutes, barring objections.
>
> Could you please wait till tomorrow?  I've bigger pending fixes for related 
> code pending/being tested that I plan to push today.  I'd also like to take a 
> look before...

Sure.

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


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

Re: [HACKERS] Cached plans and statement generalization

[Douglas Doole]

>
> One interesting idea from Doug Doole was to do it between the tokenizer
> and parser.  I think they are glued together so you would need a way to run
> the tokenizer separately and compare that to the tokens you stored for the
> cached plan.
>

When I did this, we had the same problem that the tokenizer and parser were
tightly coupled. Fortunately, I was able to do as you suggest and run the
tokenizer separately to do my analysis.

So my model was to do statement generalization before entering the compiler
at all. I would tokenize the statement to find the literals and generate a
new statement string with placeholders. The new string would the be passed
to the compiler which would then tokenize and parse the reworked statement.

This means we incurred the cost of tokenizing twice, but the tokenizer was
lightweight enough that it wasn't a problem. In exchange I was able to do
statement generalization without touching the compiler - the compiler saw
the generalized statement text as any other statement and handled it in the
exact same way. (There was just a bit of new code around variable binding.)

Re: [HACKERS] [PATCH v2] Progress command to monitor progression of long running SQL queries

[David Fetter]

On Thu, May 11, 2017 at 05:24:16PM +0200, Remi Colinet wrote:
> 2017-05-10 21:52 GMT+02:00 David Fetter :
> 
> > On Wed, May 10, 2017 at 06:40:31PM +0200, Remi Colinet wrote:
> > > Hello,
> > >
> > > This is version 2 of the new command name PROGRESS which I wrote in
> > > order to monitor long running SQL queries in a Postgres backend
> > > process.
> >
> > Perhaps I missed something important in the discussion, but was there
> > a good reason that this isn't a function callable from SQL, i.e. not
> > restricted to the psql client?

Please don't top post. http://www.caliburn.nl/topposting.html

> That's good point.
> 
> I will probably convert the new command to a SQL function.

Great!

> I was fumbling between a table or a SQL function. Oracle uses
> v$session_longops to track progression of long running SQL queries
> which have run for more than 6 seconds.  But a function is much
> better to provide parameters such as the backend pid and a format
> specification for the output.

Once it's a function, it can very easily be called in a system (or
user-defined) view.

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] Cached plans and statement generalization

[Bruce Momjian]

On Thu, May 11, 2017 at 05:39:58PM +, Douglas Doole wrote:
> One interesting idea from Doug Doole was to do it between the tokenizer 
> and
> parser.  I think they are glued together so you would need a way to run 
> the
> tokenizer separately and compare that to the tokens you stored for the
> cached plan.
> 
> 
> When I did this, we had the same problem that the tokenizer and parser were
> tightly coupled. Fortunately, I was able to do as you suggest and run the
> tokenizer separately to do my analysis. 
> 
> So my model was to do statement generalization before entering the compiler at
> all. I would tokenize the statement to find the literals and generate a new
> statement string with placeholders. The new string would the be passed to the
> compiler which would then tokenize and parse the reworked statement.
> 
> This means we incurred the cost of tokenizing twice, but the tokenizer was
> lightweight enough that it wasn't a problem. In exchange I was able to do
> statement generalization without touching the compiler - the compiler saw the
> generalized statement text as any other statement and handled it in the exact
> same way. (There was just a bit of new code around variable binding.)

Good point.  I think we need to do some measurements to see if the
parser-only stage is actually significant.  I have a hunch that
commercial databases have much heavier parsers than we do.

This split would also not work if the scanner feeds changes back into
the parser.  I know C does that for typedefs but I don't think we do.

Ideally I would like to see percentage-of-execution numbers for typical
queries for scan, parse, parse-analysis, plan, and execute to see where
the wins are.

-- 
  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] Logical decoding truncate

[Euler Taveira]

2017-05-11 4:23 GMT-03:00 Friedrich, Steffen <
steffen.friedr...@dieboldnixdorf.com>:

> I am writing a logical decoding output plugin decoding WAL to SQL which is
> finally applied to target database.
>
>
>
> Is it possible to decode a TRUNCATE statement and the tables involved?
>
>
Yes, use event triggers. You can decode whatever DDL command you want.


> Assuming the SQL statement "TRUNCATE x, y;",  I am interested in decoding
> the operation TRUNCATE and the corresponding tables x and y so that I can
> reconstruct the SQL statement/transaction.
>
> Is that possible?
>
> If so, can you please provide an example or point me into the right
> direction?
>
>
>
Take a look at BDR code (bdr_queue_ddl_commands and
bdr_queue_dropped_objects) [1]. It implements DDL replication too.


[1] https://github.com/2ndQuadrant/bdr


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

Re: [HACKERS] Cached plans and statement generalization

[Tom Lane]

Bruce Momjian  writes:
> Good point.  I think we need to do some measurements to see if the
> parser-only stage is actually significant.  I have a hunch that
> commercial databases have much heavier parsers than we do.

FWIW, gram.y does show up as significant in many of the profiles I take.
I speculate that this is not so much that it eats many CPU cycles, as that
the constant tables are so large as to incur lots of cache misses.  scan.l
is not quite as big a deal for some reason, even though it's also large.

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] Cached plans and statement generalization

[Andres Freund]

On May 11, 2017 11:31:02 AM PDT, Tom Lane  wrote:
>Bruce Momjian  writes:
>> Good point.  I think we need to do some measurements to see if the
>> parser-only stage is actually significant.  I have a hunch that
>> commercial databases have much heavier parsers than we do.
>
>FWIW, gram.y does show up as significant in many of the profiles I
>take.
>I speculate that this is not so much that it eats many CPU cycles, as
>that
>the constant tables are so large as to incur lots of cache misses. 
>scan.l
>is not quite as big a deal for some reason, even though it's also
>large.

And that there's a lot of unpredictable branches, leading to a lot if pipeline 
stalls.

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] WITH clause in CREATE STATISTICS

[Alvaro Herrera]

Tom Lane wrote:

> Have you thought further about the upthread suggestion to just borrow
> SELECT's syntax lock stock and barrel?  That is, it'd look something
> like
> 
> CREATE STATISTICS name [(list of stats types)] expression-list FROM ...
> [ WHERE ... ] [ WITH (options) ]
> 
> This would certainly support any FROM stuff we might want later.
> Yeah, you'd need to reject any features you weren't supporting at
> execution, but doing that would allow issuing a friendlier message than
> "syntax error" anyway.
> 
> If you don't have the cycles for that, I'd be willing to look into it.

Bison seems to like the productions below.  Is this what you had in
mind?  These mostly mimic joined_table and table_ref, stripping out the
rules that we don't need.

Note that I had to put back the FOR keyword in there; without the FOR,
that is "CREATE STATS any_name opt_name_list expr_list FROM" there was
one shift/reduce conflict, which I tried to solve by splitting
opt_name_list using two rules (one with "'(' name_list ')'" and one
without), but that gave rise to two reduce/reduce conflicts, and I didn't
see any way to deal with them.

(Note that I ended up realizing that stats_type_list is unnecessary
since we can use name_list.)


CreateStatsStmt:
CREATE opt_if_not_exists STATISTICS any_name
opt_name_list FOR expr_list FROM stats_joined_table
{
CreateStatsStmt *n = 
makeNode(CreateStatsStmt);
n->defnames = $4;
n->stat_types = $6;
n->if_not_exists = $2;

stmt->relation = $9;
stmt->keys = $7;
$$ = (Node *)n;
}
;

stats_joined_table:
  '(' stats_joined_table ')'
{
}
| stats_table_ref CROSS JOIN stats_table_ref
{
}
| stats_table_ref join_type JOIN stats_table_ref 
join_qual
{
}
| stats_table_ref JOIN stats_table_ref join_qual
{
}
| stats_table_ref NATURAL join_type JOIN stats_table_ref
{
}
| stats_table_ref NATURAL JOIN table_ref
{
}
;

stats_table_ref:
relation_expr opt_alias_clause
{
}
| stats_joined_table
{
}
| '(' stats_joined_table ')' alias_clause
{
}
;



-- 
Á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] Cached plans and statement generalization

[Konstantin Knizhnik]

On 05/11/2017 06:12 PM, Bruce Momjian wrote:

On Wed, May 10, 2017 at 07:11:07PM +0300, Konstantin Knizhnik wrote:

I am going to continue work on this patch I will be glad to receive any
feedback and suggestions for its improvement.
In most cases, applications are not accessing Postgres directly, but using
some connection pooling layer and so them are not able to use prepared
statements.
But at simple OLTP Postgres spent more time on building query plan than on
execution itself. And it is possible to speedup Postgres about two times at
such workload!
Another alternative is true shared plan cache.  May be it is even more
perspective approach, but definitely much more invasive and harder to
implement.

Can we back up and get an overview of what you are doing and how you are
doing it?  Our TODO list suggests this order for successful patches:

Desirability -> Design -> Implement -> Test -> Review -> Commit

You kind of started at the Implementation/patch level, which makes it
hard to evaluate.

I think everyone agrees on the Desirability of the feature, but the
Design is the tricky part.  I think the design questions are:

*  What information is stored about cached plans?
*  How are the cached plans invalidated?
*  How is a query matched against a cached plan?

Looking at the options, ideally the plan would be cached at the same
query stage as the stage where the incoming query is checked against the
cache.  However, caching and checking at the same level offers no
benefit, so they are going to be different.  For example, caching a
parse tree at the time it is created, then checking at the same point if
the incoming query is the same doesn't help you because you already had
to create the parse tree get to that point.

A more concrete example is prepared statements.  They are stored at the
end of planning and matched in the parser.  However, you can easily do
that since the incoming query specifies the name of the prepared query,
so there is no trick to matching.

The desire is to cache as late as possible so you cache more work and
you have more detail about the referenced objects, which helps with
cache invalidation.  However, you also want to do cache matching as
early as possible to improve performance.

So, let's look at some options.  One interesting idea from Doug Doole
was to do it between the tokenizer and parser.  I think they are glued
together so you would need a way to run the tokenizer separately and
compare that to the tokens you stored for the cached plan.  The larger
issue is that prepared plans already are checked after parsing, and we
know they are a win, so matching any earlier than that just seems like
overkill and likely to lead to lots of problems.

So, you could do it after parsing but before parse-analysis, which is
kind of what prepared queries do.  One tricky problem is that we don't
bind the query string tokens to database objects until after parse
analysis.

Doing matching before parse-analysis is going to be tricky, which is why
there are so many comments about the approach.  Changing search_path can
certainly affect it, but creating objects in earlier-mentioned schemas
can also change how an object reference in a query is resolved.  Even
obscure things like the creation of a new operator that has higher
precedence in the query could change the plan, though am not sure if
our prepared query system even handles that properly.

Anyway, that is my feedback.  I would like to get an overview of what
you are trying to do and the costs/benefits of each option so we can
best guide you.


Sorry, for luck of overview.
I have started with small prototype just to investigate if such optimization 
makes sense or not.
When I get more than two time advantage in performance on standard pgbench, I 
come to conclusion that this
optimization can be really very useful and now try to find the best way of its 
implementation.

I have started with simplest approach when string literals are replaced with 
parameters. It is done before parsing.
And can be done very fast - just need to locate data in quotes.
But this approach is not safe and universal: you will not be able to speedup 
most of the existed queries without rewriting them.

This is why I have provided second implementation which replace literals with 
parameters after raw parsing.
Certainly it is slower than first approach. But still provide significant 
advantage in performance: more than two times at pgbench.
Then I tried to run regression tests and find several situations where type 
analysis is not correctly performed in case of replacing literals with 
parameters.

So my third attempt is to replace constant nodes with parameters in already 
analyzed tree.

Now answering your questions:

*  What information is stored about cached plans?

Key to locate cached plan is raw parse tree. Value is saved CachedPlanSource.

*  How are the cached plans invalidated?

In the same way as plans for explicitly prepared statements.

*  How is a 

Re: [HACKERS] Cached plans and statement generalization

[Konstantin Knizhnik]

On 05/11/2017 09:31 PM, Tom Lane wrote:

Bruce Momjian  writes:

Good point.  I think we need to do some measurements to see if the
parser-only stage is actually significant.  I have a hunch that
commercial databases have much heavier parsers than we do.

FWIW, gram.y does show up as significant in many of the profiles I take.
I speculate that this is not so much that it eats many CPU cycles, as that
the constant tables are so large as to incur lots of cache misses.  scan.l
is not quite as big a deal for some reason, even though it's also large.

regards, tom lane

Yes, my results shows that pg_parse_query adds not so much overhead:
206k TPS for my first variant with string literal substitution and modified 
query text used as hash key vs.
181k. TPS for version with patching raw parse tree constructed by 
pg_parse_query.


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



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

Re: [HACKERS] WITH clause in CREATE STATISTICS

[Tom Lane]

Alvaro Herrera  writes:
> Tom Lane wrote:
>> Have you thought further about the upthread suggestion to just borrow
>> SELECT's syntax lock stock and barrel?

> Bison seems to like the productions below.  Is this what you had in
> mind?  These mostly mimic joined_table and table_ref, stripping out the
> rules that we don't need.

I'd suggest just using the from_list production and then complaining
at runtime if what you get is too complicated.  Otherwise, you have
to maintain a duplicate set of productions, and you're going to be
unable to throw anything more informative than "syntax error" when
somebody tries to exceed the implementation limits.

> Note that I had to put back the FOR keyword in there; without the FOR,
> that is "CREATE STATS any_name opt_name_list expr_list FROM" there was
> one shift/reduce conflict, which I tried to solve by splitting
> opt_name_list using two rules (one with "'(' name_list ')'" and one
> without), but that gave rise to two reduce/reduce conflicts, and I didn't
> see any way to deal with them.

That's fine.  I was going to suggest using ON after the stats type list
just because it seemed to read better.  FOR is about as good.

> (Note that I ended up realizing that stats_type_list is unnecessary
> since we can use name_list.)

Check.

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] Cached plans and statement generalization

[Andres Freund]

On 2017-05-11 22:48:26 +0300, Konstantin Knizhnik wrote:
> On 05/11/2017 09:31 PM, Tom Lane wrote:
> > Bruce Momjian  writes:
> > > Good point.  I think we need to do some measurements to see if the
> > > parser-only stage is actually significant.  I have a hunch that
> > > commercial databases have much heavier parsers than we do.
> > FWIW, gram.y does show up as significant in many of the profiles I take.
> > I speculate that this is not so much that it eats many CPU cycles, as that
> > the constant tables are so large as to incur lots of cache misses.  scan.l
> > is not quite as big a deal for some reason, even though it's also large.
> > 
> > regards, tom lane
> Yes, my results shows that pg_parse_query adds not so much overhead:
> 206k TPS for my first variant with string literal substitution and modified 
> query text used as hash key vs.
> 181k. TPS for version with patching raw parse tree constructed by 
> pg_parse_query.

Those numbers and your statement seem to contradict each other?

- 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] [BUGS] Concurrent ALTER SEQUENCE RESTART Regression

[Peter Eisentraut]

On 5/10/17 12:24, Andres Freund wrote:
> Upthread I theorized whether
> that's actually still meaningful given fastpath locking and such, but I
> guess we'll have to evaluate that.

I did some testing.

I ran this script

CREATE SEQUENCE seq1;

DO LANGUAGE plpythonu $$
plan = plpy.prepare("SELECT nextval('seq1')")
for i in range(0, 1000):
plpy.execute(plan)
$$;

and timed the "DO".

I compared 9.1 (pre fast-path locking) with 9.2 (with fast-path locking).

I compared the stock releases with a patched version that replaces the
body of open_share_lock() with just

return relation_open(seq->relid, AccessShareLock);

First, a single session:

9.1 unpatched
Time: 57634.098 ms

9.1 patched
Time: 58674.655 ms

(These were within each other's variance over several runs.)

9.2 unpatched
Time: 64868.305 ms

9.2 patched
Time: 60585.317 ms

(So without contention fast-path locking beats the extra dance that
open_share_lock() does.)

Then, with four concurrent sessions (one timed, three just running):

9.1 unpatched
Time: 73123.661 ms

9.1 patched
Time: 78019.079 ms

So the "hack" avoids the slowdown from contention here.

9.2 unpatched
Time: 73308.873 ms

9.2 patched
Time: 70353.971 ms

Here, fast-path locking beats out the "hack".

If I add more concurrent sessions, everything gets much slower but the
advantage of the patched version stays about the same.  But I can't
reliably test that on this hardware.

(One wonders whether these differences remain relevant if this is run
within the context of an INSERT or COPY instead of just a tight loop.)

-- 
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] Cached plans and statement generalization

[Konstantin Knizhnik]

On 05/11/2017 10:52 PM, Andres Freund wrote:

On 2017-05-11 22:48:26 +0300, Konstantin Knizhnik wrote:

On 05/11/2017 09:31 PM, Tom Lane wrote:

Bruce Momjian  writes:

Good point.  I think we need to do some measurements to see if the
parser-only stage is actually significant.  I have a hunch that
commercial databases have much heavier parsers than we do.

FWIW, gram.y does show up as significant in many of the profiles I take.
I speculate that this is not so much that it eats many CPU cycles, as that
the constant tables are so large as to incur lots of cache misses.  scan.l
is not quite as big a deal for some reason, even though it's also large.

regards, tom lane

Yes, my results shows that pg_parse_query adds not so much overhead:
206k TPS for my first variant with string literal substitution and modified 
query text used as hash key vs.
181k. TPS for version with patching raw parse tree constructed by 
pg_parse_query.

Those numbers and your statement seem to contradict each other?


Oops, my parse error:( I incorrectly read Tom's statement.
Actually, I also was afraid that price of parsing is large enough and this is 
why my first attempt was to avoid parsing.
But then I find out that most of the time is spent in analyze and planning (see 
attached profile):

pg_parse_query: 4.23%
pg_analyze_and_rewrite 8.45%
pg_plan_queries: 15.49%



- Andres



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


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

Re: [HACKERS] [BUGS] Concurrent ALTER SEQUENCE RESTART Regression

[Andres Freund]

On 2017-05-11 11:35:22 -0400, Peter Eisentraut wrote:
> On 5/10/17 12:24, Andres Freund wrote:
> > The issue isn't the strength, but that we currently have this weird
> > hackery around open_share_lock():
> > /*
> >  * Open the sequence and acquire AccessShareLock if needed
> >  *
> >  * If we haven't touched the sequence already in this transaction,
> >  * we need to acquire AccessShareLock.  We arrange for the lock to
> >  * be owned by the top transaction, so that we don't need to do it
> >  * more than once per xact.
> >  */
> > 
> > This'd probably need to be removed, as we'd otherwise would get very
> > weird semantics around aborted subxacts.
> 
> Can you explain in more detail what you mean by this?

Well, right now we don't do proper lock-tracking for sequences, always
assigning them to the toplevel transaction.  But that doesn't seem
proper when nextval() would conflict with ALTER SEQUENCE et al, because
then locks would continue to be held by aborted savepoints.

- 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] [BUGS] Concurrent ALTER SEQUENCE RESTART Regression

[Tom Lane]

Peter Eisentraut  writes:
> On 5/10/17 12:24, Andres Freund wrote:
>> Upthread I theorized whether
>> that's actually still meaningful given fastpath locking and such, but I
>> guess we'll have to evaluate that.

> [ with or without contention, fast-path locking beats the extra dance that
> open_share_lock() does. ]

That is pretty cool.  It would be good to verify the same on master,
but assuming it holds up, I think it's ok to remove open_share_lock().

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] [BUGS] Concurrent ALTER SEQUENCE RESTART Regression

[Andres Freund]

Hi,


On 2017-05-11 16:27:48 -0400, Peter Eisentraut wrote:
> On 5/10/17 12:24, Andres Freund wrote:
> > Upthread I theorized whether
> > that's actually still meaningful given fastpath locking and such, but I
> > guess we'll have to evaluate that.
> 
> I did some testing.

That's with the open_share_lock stuff ripped out entirely, replaced by a
plain lock acquisition within the current subxact?


> (These were within each other's variance over several runs.)
> 
> 9.2 unpatched
> Time: 64868.305 ms
> 
> 9.2 patched
> Time: 60585.317 ms
> 
> (So without contention fast-path locking beats the extra dance that
> open_share_lock() does.)

That's kind of surprising, I really wouldn't have thought it'd be faster
without.  I guess it's the overhead of sigsetjmp().  Cool.


- 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] [BUGS] Concurrent ALTER SEQUENCE RESTART Regression

[Tom Lane]

Peter Eisentraut  writes:
> I ran this script

> CREATE SEQUENCE seq1;

> DO LANGUAGE plpythonu $$
> plan = plpy.prepare("SELECT nextval('seq1')")
> for i in range(0, 1000):
> plpy.execute(plan)
> $$;

> and timed the "DO".

It occurred to me that plpy.execute is going to run a subtransaction for
each call, which makes this kind of a dubious test case, both because of
the extra subtransaction overhead and because subtransaction lock
ownership effects will possibly skew the results.  So I tried to
reproduce the test using plain plpgsql,

CREATE SEQUENCE IF NOT EXISTS seq1;

\timing on

do $$
declare x int;
begin
  for i in 0 .. 1000 loop
x := nextval('seq1');
  end loop;
end
$$;

On HEAD, building without cassert, I got a fairly reproducible result
of about 10.6 seconds.  I doubt my machine is 6X faster than yours,
so this indicates that the subtransaction overhead is pretty real.

> I compared the stock releases with a patched version that replaces the
> body of open_share_lock() with just
> return relation_open(seq->relid, AccessShareLock);

Hm.  I don't think that's a sufficient code change, because if you do it
like that then the lock remains held after nextval() returns.  This means
that (a) subsequent calls aren't hitting the shared lock manager at all,
they're just incrementing a local lock counter, and whether it would be
fastpath or not is irrelevant; and (b) this means that the semantic issues
Andres is worried about remain in place, because we will hold the lock
till transaction end.

I experimented with something similar, just replacing open_share_lock
as above, and I got runtimes of just about 12 seconds, which surprised
me a bit.  I'd have thought the locallock-already-exists code path
would be faster than that.

I then further changed the code so that nextval_internal ends with
"relation_close(seqrel, AccessShareLock);" rather than NoLock,
so that the lock is actually released between calls.  This boosted
the runtime up to 15.5 seconds, or a 50% penalty over HEAD.

My conclusion is that in low-overhead cases, such as using a sequence
to assign default values during COPY IN, the percentage overhead from
acquiring and releasing the lock could be pretty dire.  Still, we might
not have much choice if we want nice semantics.

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] [BUGS] Concurrent ALTER SEQUENCE RESTART Regression

[Tom Lane]

Andres Freund  writes:
> On 2017-05-11 16:27:48 -0400, Peter Eisentraut wrote:
>> (So without contention fast-path locking beats the extra dance that
>> open_share_lock() does.)

> That's kind of surprising, I really wouldn't have thought it'd be faster
> without.  I guess it's the overhead of sigsetjmp().  Cool.

My results (posted nearby) lead me to suspect that the improvement
Peter sees from 9.1 to 9.2 has little to do with fastpath locking
and a lot to do with some improvement or other in subtransaction
lock management.

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] [BUGS] Concurrent ALTER SEQUENCE RESTART Regression

[Andres Freund]

On 2017-05-11 17:21:18 -0400, Tom Lane wrote:
> Peter Eisentraut  writes:
> > I ran this script
> 
> > CREATE SEQUENCE seq1;
> 
> > DO LANGUAGE plpythonu $$
> > plan = plpy.prepare("SELECT nextval('seq1')")
> > for i in range(0, 1000):
> > plpy.execute(plan)
> > $$;
> 
> > and timed the "DO".
> 
> It occurred to me that plpy.execute is going to run a subtransaction for
> each call, which makes this kind of a dubious test case, both because of
> the extra subtransaction overhead and because subtransaction lock
> ownership effects will possibly skew the results.  So I tried to
> reproduce the test using plain plpgsql,
> 
> CREATE SEQUENCE IF NOT EXISTS seq1;
> 
> \timing on
> 
> do $$
> declare x int;
> begin
>   for i in 0 .. 1000 loop
> x := nextval('seq1');
>   end loop;
> end
> $$;
> 
> On HEAD, building without cassert, I got a fairly reproducible result
> of about 10.6 seconds.  I doubt my machine is 6X faster than yours,
> so this indicates that the subtransaction overhead is pretty real.

Isn't that pretty much the point?  The whole open_share_lock()
optimization looks like it really only can make a difference with
subtransactions?


> > I compared the stock releases with a patched version that replaces the
> > body of open_share_lock() with just
> > return relation_open(seq->relid, AccessShareLock);
> 
> Hm.  I don't think that's a sufficient code change, because if you do it
> like that then the lock remains held after nextval() returns.

Hm?  That's not new, is it?  We previously did a LockRelationOid(seq->relid) and
then relation_open(seq->relid, NoLock)?


> This means
> that (a) subsequent calls aren't hitting the shared lock manager at all,
> they're just incrementing a local lock counter, and whether it would be
> fastpath or not is irrelevant; and (b) this means that the semantic issues
> Andres is worried about remain in place, because we will hold the lock
> till transaction end.

My concern was aborted subtransactions, and those should release their
lock via resowner stuff at abort?


> I experimented with something similar, just replacing open_share_lock
> as above, and I got runtimes of just about 12 seconds, which surprised
> me a bit.

Yea, I suspect we'd need something like the current open_share_lock,
except with the resowner trickery removed.


> I'd have thought the locallock-already-exists code path would be
> faster than that.

It's a hash-table lookup, via dynahash no less, that's definitley going
to be more expensive than a single seq->lxid != thislxid...


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] CTE inlining

[Yaroslav]

Ilya Shkuratov wrote
> First of all, to such replacement to be valid, the CTE must be 
> 1. non-writable (e.g. be of form: SELECT ...),
> 2. do not use VOLATILE or STABLE functions,
> 3. ... (maybe there must be more restrictions?) 

What about simple things like this?

CREATE OR REPLACE FUNCTION z(numeric) RETURNS boolean AS $$
BEGIN
RETURN $1 <> 0;
END;
$$ LANGUAGE plpgSQL IMMUTABLE COST 1000;

-- This one works:
WITH T AS (
SELECT 1.0 AS v1, 0.0 AS v2
UNION ALL
SELECT 3.0, 1.0
UNION ALL
SELECT 2.0, 0.0
), a AS (
SELECT *
  FROM t
 WHERE z(v2)
)
SELECT *
  FROM a
 WHERE v1/v2 > 1.5;

-- This one gives 'division by zero':
WITH T AS (
SELECT 1.0 AS v1, 0.0 AS v2
UNION ALL
SELECT 3.0, 1.0
UNION ALL
SELECT 2.0, 0.0
)
SELECT *
  FROM (
   SELECT *
 FROM t
WHERE z(v2)
   ) AS a
 WHERE v1/v2 > 1.5;




-
WBR, Yaroslav Schekin.
--
View this message in context: 
http://www.postgresql-archive.org/CTE-inlining-tp5958992p5961086.html
Sent from the PostgreSQL - hackers mailing list archive at Nabble.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] snapbuild woes

[Andres Freund]

On 2017-05-08 00:10:12 -0700, Andres Freund wrote:
> I plan to commit the next pending patch after the back branch releases
> are cut - it's an invasive fix and the issue doesn't cause corruption
> "just" slow slot creation. So it seems better to wait for a few days,
> rather than hurry it into the release.

Now that that's done, here's an updated version of that patch.  Note the
new logic to trigger xl_running_xact's to be logged at the right spot.
Works well in my testing.

I plan to commit this fairly soon, unless somebody wants a bit more time
to look into it.

Unless somebody protests, I'd like to slightly revise how the on-disk
snapshots are stored on master - given the issues this bug/commit showed
with the current method - but I can see one could argue that that should
rather be done next release.

- 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] snapbuild woes

[Peter Geoghegan]

On Thu, May 11, 2017 at 2:51 PM, Andres Freund  wrote:
> Now that that's done, here's an updated version of that patch.  Note the
> new logic to trigger xl_running_xact's to be logged at the right spot.
> Works well in my testing.

You forgot the patch. :-)


-- 
Peter Geoghegan

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] snapbuild woes

[Andres Freund]

On 2017-05-11 14:51:55 -0700,  wrote:
> On 2017-05-08 00:10:12 -0700, Andres Freund wrote:
> > I plan to commit the next pending patch after the back branch releases
> > are cut - it's an invasive fix and the issue doesn't cause corruption
> > "just" slow slot creation. So it seems better to wait for a few days,
> > rather than hurry it into the release.
> 
> Now that that's done, here's an updated version of that patch.  Note the
> new logic to trigger xl_running_xact's to be logged at the right spot.
> Works well in my testing.
> 
> I plan to commit this fairly soon, unless somebody wants a bit more time
> to look into it.
> 
> Unless somebody protests, I'd like to slightly revise how the on-disk
> snapshots are stored on master - given the issues this bug/commit showed
> with the current method - but I can see one could argue that that should
> rather be done next release.

As usual I forgot my attachement.

- Andres
>From 3ecec368e3015f5082f120b1a428d1108203a356 Mon Sep 17 00:00:00 2001
From: Andres Freund 
Date: Thu, 4 May 2017 16:48:00 -0700
Subject: [PATCH] Fix race condition leading to hanging logical slot creation.

The snapshot assembly during the creation of logical slots relied
waiting for transactions in xl_running_xacts to end, by checking for
their commit/abort records.  Unfortunately, despite locking, it is
possible to see an xl_running_xact record listing transactions as
ready, that have already WAL-logged an commit/abort record, as the
locking just prevents the ProcArray to be adjusted, and the commit
record has to be logged first.

That lead to either delayed or hanging snapshot creation, because
snapbuild.c would wait "forever" to see commit/abort records for some
transactions.  That hang resolved only if a xl_running_xacts record
without any running transactions happened to be logged, far from
certain on a busy server.

It's impractical to prevent that via more heavyweight locking, the
likelihood of deadlocks and significantly increased contention would
be too big.

Instead change the initial snapshot creation to be solely based on
tracking the oldest running transaction via
xl_running_xacts->oldestRunningXid - that actually ends up
significantly simplifying the code.  That has two disadvantages:
1) Because we cannot fully "trust" the contents of xl_running_xacts,
   we cannot use it to build the initial snapshot.  Instead we have to
   wait twice for all running transactions to finish.
2) Previously a slot, unless the race occurred, could be created when
   the all transaction perceived as running based on commit/abort
   records, now we have to wait for the next xl_running_xacts record.
To address that, trigger logging new xl_running_xacts record from
within snapbuild.c exactly when necessary.

Unfortunately snabuild.c's SnapBuild is stored on disk, one of the
stupider ideas of a certain Mr Freund, so we can't change it in a
minor release.  As this is going to be backpatched, we have to play
around a bit to keep on-disk compatibility.  A later commit will
rejigger that on master.

Author: Andres Freund, based on a quite different patch from Petr Jelinek
Analyzed-By: Petr Jelinek
Reviewed-By: Petr Jelinek
Discussion: https://postgr.es/m/f37e975c-908f-858e-707f-058d3b1eb...@2ndquadrant.com
Backpatch: 9.4-, where logical decoding has been introduced
---
 contrib/test_decoding/expected/ondisk_startup.out |  15 +-
 contrib/test_decoding/specs/ondisk_startup.spec   |   8 +-
 src/backend/replication/logical/decode.c  |   3 -
 src/backend/replication/logical/reorderbuffer.c   |   2 +-
 src/backend/replication/logical/snapbuild.c   | 416 ++
 src/include/replication/snapbuild.h   |  25 +-
 6 files changed, 220 insertions(+), 249 deletions(-)

diff --git a/contrib/test_decoding/expected/ondisk_startup.out b/contrib/test_decoding/expected/ondisk_startup.out
index 65115c830a..c7b1f45b46 100644
--- a/contrib/test_decoding/expected/ondisk_startup.out
+++ b/contrib/test_decoding/expected/ondisk_startup.out
@@ -1,21 +1,30 @@
 Parsed test spec with 3 sessions
 
-starting permutation: s2txid s1init s3txid s2alter s2c s1insert s1checkpoint s1start s1insert s1alter s1insert s1start
-step s2txid: BEGIN ISOLATION LEVEL REPEATABLE READ; SELECT txid_current() IS NULL;
+starting permutation: s2b s2txid s1init s3b s3txid s2alter s2c s2b s2txid s3c s2c s1insert s1checkpoint s1start s1insert s1alter s1insert s1start
+step s2b: BEGIN;
+step s2txid: SELECT txid_current() IS NULL;
 ?column?   
 
 f  
 step s1init: SELECT 'init' FROM pg_create_logical_replication_slot('isolation_slot', 'test_decoding'); 
-step s3txid: BEGIN ISOLATION LEVEL REPEATABLE READ; SELECT txid_current() IS NULL;
+step s3b: BEGIN;
+step s3txid: SELECT txid_current() IS NULL;
 ?column?   
 
 f  
 step s2alter: ALTER TABLE do_write ADD COLUMN addedbys2 int;
 step s2c: COMMIT;
+step s2b: BEGIN;
+step s2txid: SELECT txid_current() IS NULL;
+?column?   
+
+f   

Re: [HACKERS] snapbuild woes

[Andres Freund]

On 2017-04-15 05:18:49 +0200, Petr Jelinek wrote:
> >>> From 3318a929e691870f3c1ca665bec3bfa8ea2af2a8 Mon Sep 17 00:00:00 2001
> >>> From: Petr Jelinek 
> >>> Date: Sun, 26 Feb 2017 01:07:33 +0100
> >>> Subject: [PATCH 3/5] Prevent snapshot builder xmin from going backwards
> >>
> >> A bit more commentary would be good. What does that protect us against?
> >>
> > 
> > I think I explained that in the email. We might export snapshot with
> > xmin smaller than global xmin otherwise.
> > 
> 
> Updated commit message with explanation as well.


> From ae60b52ae0ca96bc14169cd507f101fbb5dfdf52 Mon Sep 17 00:00:00 2001
> From: Petr Jelinek 
> Date: Sun, 26 Feb 2017 01:07:33 +0100
> Subject: [PATCH 3/5] Prevent snapshot builder xmin from going backwards
> 
> Logical decoding snapshot builder may encounter xl_running_xacts with
> older xmin than the xmin of the builder. This can happen because
> LogStandbySnapshot() sometimes sees already committed transactions as
> running (there is difference between "running" in terms for WAL and in
> terms of ProcArray). When this happens we must make sure that the xmin
> of snapshot builder won't go back otherwise the resulting snapshot would
> show some transaction as running even though they have already
> committed.
> ---
>  src/backend/replication/logical/snapbuild.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/src/backend/replication/logical/snapbuild.c 
> b/src/backend/replication/logical/snapbuild.c
> index ada618d..3e34f75 100644
> --- a/src/backend/replication/logical/snapbuild.c
> +++ b/src/backend/replication/logical/snapbuild.c
> @@ -1165,7 +1165,8 @@ SnapBuildProcessRunningXacts(SnapBuild *builder, 
> XLogRecPtr lsn, xl_running_xact
>* looking, it's correct and actually more efficient this way since we 
> hit
>* fast paths in tqual.c.
>*/
> - builder->xmin = running->oldestRunningXid;
> + if (TransactionIdFollowsOrEquals(running->oldestRunningXid, 
> builder->xmin))
> + builder->xmin = running->oldestRunningXid;
>  
>   /* Remove transactions we don't need to keep track off anymore */
>   SnapBuildPurgeCommittedTxn(builder);
> -- 
> 2.7.4

I still don't understand.  The snapshot's xmin is solely managed via
xl_running_xacts, so I don't see how the WAL/procarray difference can
play a role here.  ->committed isn't pruned before xl_running_xacts
indicates it can be done, so I don't understand your explanation above.

I'd be ok with adding this as paranoia check, but I still don't
understand when it could practically be hit.

- 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] WITH clause in CREATE STATISTICS

[Alvaro Herrera]

Tom Lane wrote:
> Alvaro Herrera  writes:
> > Tom Lane wrote:
> >> Have you thought further about the upthread suggestion to just borrow
> >> SELECT's syntax lock stock and barrel?
> 
> > Bison seems to like the productions below.  Is this what you had in
> > mind?  These mostly mimic joined_table and table_ref, stripping out the
> > rules that we don't need.
> 
> I'd suggest just using the from_list production and then complaining
> at runtime if what you get is too complicated.  Otherwise, you have
> to maintain a duplicate set of productions, and you're going to be
> unable to throw anything more informative than "syntax error" when
> somebody tries to exceed the implementation limits.

Hmm, yeah, makes sense.  Here's a patch for this approach.  I ended up
using on ON again for the list of columns.  I suppose the checks in
CreateStatistics() could still be improved, but I'd go ahead and push
this tomorrow morning and we can hammer those details later on, if it's
still needed.  Better avoid shipping beta with outdated grammar ...

BTW the new castNode() family of macros don't work with Value nodes
(because the tags are different depending on what's stored, but each
type does not have its own struct.  Oh well.)

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
diff --git a/doc/src/sgml/perform.sgml b/doc/src/sgml/perform.sgml
index b10b734b90..32e17ee5f8 100644
--- a/doc/src/sgml/perform.sgml
+++ b/doc/src/sgml/perform.sgml
@@ -1132,8 +1132,8 @@ WHERE tablename = 'road';
  To inspect functional dependencies on a statistics
  stts, you may do this:
 
-CREATE STATISTICS stts WITH (dependencies)
-   ON (zip, city) FROM zipcodes;
+CREATE STATISTICS stts (dependencies)
+   ON zip, city FROM zipcodes;
 ANALYZE zipcodes;
 SELECT stxname, stxkeys, stxdependencies
   FROM pg_statistic_ext
@@ -1219,8 +1219,8 @@ EXPLAIN (ANALYZE, TIMING OFF) SELECT * FROM t WHERE a = 1 
AND b = 10;
  Continuing the above example, the n-distinct coefficients in a ZIP
  code table may look like the following:
 
-CREATE STATISTICS stts2 WITH (ndistinct)
-   ON (zip, state, city) FROM zipcodes;
+CREATE STATISTICS stts2 (ndistinct)
+   ON zip, state, city FROM zipcodes;
 ANALYZE zipcodes;
 SELECT stxkeys AS k, stxndistinct AS nd
   FROM pg_statistic_ext
diff --git a/doc/src/sgml/planstats.sgml b/doc/src/sgml/planstats.sgml
index 11580bfd22..ef847b9633 100644
--- a/doc/src/sgml/planstats.sgml
+++ b/doc/src/sgml/planstats.sgml
@@ -526,7 +526,7 @@ EXPLAIN (ANALYZE, TIMING OFF) SELECT * FROM t WHERE a = 1 
AND b = 1;
 multivariate statistics on the two columns:
 
 
-CREATE STATISTICS stts WITH (dependencies) ON (a, b) FROM t;
+CREATE STATISTICS stts (dependencies) ON a, b FROM t;
 ANALYZE t;
 EXPLAIN (ANALYZE, TIMING OFF) SELECT * FROM t WHERE a = 1 AND b = 1;
   QUERY PLAN   
@@ -569,7 +569,7 @@ EXPLAIN (ANALYZE, TIMING OFF) SELECT COUNT(*) FROM t GROUP 
BY a, b;
 calculation, the estimate is much improved:
 
 DROP STATISTICS stts;
-CREATE STATISTICS stts WITH (dependencies, ndistinct) ON (a, b) FROM t;
+CREATE STATISTICS stts (dependencies, ndistinct) ON a, b FROM t;
 ANALYZE t;
 EXPLAIN (ANALYZE, TIMING OFF) SELECT COUNT(*) FROM t GROUP BY a, b;
QUERY PLAN  
  
diff --git a/doc/src/sgml/ref/create_statistics.sgml 
b/doc/src/sgml/ref/create_statistics.sgml
index edbcf5840b..84ff52df04 100644
--- a/doc/src/sgml/ref/create_statistics.sgml
+++ b/doc/src/sgml/ref/create_statistics.sgml
@@ -22,8 +22,8 @@ PostgreSQL documentation
  
 
 CREATE STATISTICS [ IF NOT EXISTS ] statistics_name
-WITH ( option [= value] [, ... ] )
-ON ( column_name, 
column_name [, ...])
+[ ( statistic_type [, ... ] ) 
]
+ON column_name, column_name [, ...]
 FROM table_name
 
 
@@ -75,6 +75,19 @@ CREATE STATISTICS [ IF NOT EXISTS ] statistics_na

 

+statistic_type
+
+ 
+  A statistic type to be enabled for this statistics.  Currently
+  supported types are ndistinct, which enables
+  n-distinct coefficient tracking,
+  and dependencies, which enables functional
+  dependencies.
+ 
+
+   
+
+   
 column_name
 
  
@@ -94,42 +107,6 @@ CREATE STATISTICS [ IF NOT EXISTS ] statistics_na

 
   
-
-  
-   Parameters
-
- 
-  statistics parameters
- 
-
-   
-The WITH clause can specify options
-for the statistics. Available options are listed below.
-   
-
-   
-
-   
-dependencies (boolean)
-
- 
-  Enables functional dependencies for the statistics.
- 
-
-   
-
-   
-ndistinct (boolean)
-
- 
-  Enables ndistinct coefficients for the statistics.
- 
-
-   
-
-   
-
-  
  
 
  
@@ -158,7 +135,7 @@ CREATE TABLE t1 (
 INSERT INTO t1 SELECT i/100, i/500
   

Re: [HACKERS] WITH clause in CREATE STATISTICS

[Tom Lane]

Alvaro Herrera  writes:
> BTW the new castNode() family of macros don't work with Value nodes
> (because the tags are different depending on what's stored, but each
> type does not have its own struct.  Oh well.)

Yeah.  Value nodes are pretty much of a wart --- it's not clear to me
that they add anything compared to just having a separate node type
for each of the possible subclasses.  It's never bothered anyone enough
to try to replace them, though.

I'll take a look at the patch later.

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] A design for amcheck heapam verification

[Peter Geoghegan]

On Mon, May 1, 2017 at 6:39 PM, Peter Geoghegan  wrote:
> On Mon, May 1, 2017 at 6:20 PM, Tom Lane  wrote:
>> Maybe you can fix this by assuming that your own session's advertised xmin
>> is a safe upper bound on everybody else's RecentGlobalXmin.  But I'm not
>> sure if that rule does what you want.
>
> That's what you might ultimately need to fall back on (that, or
> perhaps repeated calls to GetOldestXmin() to recheck, in the style of
> pg_visibility).

I spent only a few hours writing a rough prototype, and came up with
something that does an IndexBuildHeapScan() scan following the
existing index verification steps. Its amcheck callback does an
index_form_tuple() call, hashes the resulting IndexTuple (heap TID and
all), and tests it for membership of a bloom filter generated as part
of the main B-Tree verification phase. The IndexTuple memory is freed
immediately following hashing.

The general idea here is that whatever IndexTuples ought to be in the
index following a fresh REINDEX also ought to have been found in the
index already. IndexBuildHeapScan() takes care of almost all of the
details for us.

I think I can do this correctly when only an AccessShareLock is
acquired on heap + index, provided I also do a separate
GetOldestXmin() before even the index scan begins, and do a final
recheck of the saved GetOldestXmin() value against heap tuple xmin
within the new IndexBuildHeapScan() callback (if we still think that
it should have been found by the index scan, then actually throw a
corruption related error). When there is only a ShareLock (for
bt_parent_index_check() calls), the recheck isn't necessary. I think I
should probably also make the IndexBuildHeapScan()-passed indexInfo
structure "ii_Unique = false", since waiting for the outcome of a
concurrent conflicting unique index insertion isn't useful, and can
cause deadlocks.

While I haven't really made my mind up, this design is extremely
simple, and effectively tests IndexBuildHeapScan() at the same time as
everything else. The addition of the bloom filter itself isn't
trivial, but the code added to verify_nbtree.c is.

The downside of going this way is that I cannot piggyback other types
of heap verification on the IndexBuildHeapScan() scan. Still, perhaps
it's worth it. Perhaps I should implement this bloom-filter-index-heap
verification step as one extra option for the existing B-Tree
verification functions. I may later add new verification functions
that examine and verify the heap and related SLRUs alone.

-- 
Peter Geoghegan

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] Row Level Security Documentation

[Rod Taylor]

I think the biggest piece missing is something to summarize the giant
blocks of text.

Attached is a table that has commands and policy types, and a "yes" if it
applies.

-- 
Rod Taylor
diff --git a/doc/src/sgml/ref/create_policy.sgml b/doc/src/sgml/ref/create_policy.sgml
index 3b24e5e95e..c737f9e884 100644
--- a/doc/src/sgml/ref/create_policy.sgml
+++ b/doc/src/sgml/ref/create_policy.sgml
@@ -389,6 +389,96 @@ CREATE POLICY name ON 
 

+
+   Policies Applied During Statement
+
+ 
+  
+   Policy
+   SELECT
+   INSERT
+   INSERT RETURNING
+   UPDATE WHERE
+   UPDATE RETURNING
+   DELETE WHERE
+   DELETE RETURNING
+  
+ 
+ 
+  
+   FOR ALL ... USING
+   yes
+   
+   
+   yes
+   yes
+   yes
+   yes
+  
+  
+   FOR ALL ... WITH CHECK
+   
+   yes
+   yes
+   yes
+   yes
+   
+   
+  
+  
+   FOR SELECT ... USING
+   yes
+   
+   yes
+   yes
+   yes
+   yes
+   yes
+  
+  
+   FOR INSERT ... WITH CHECK
+   
+   yes
+   yes
+   
+   
+   
+   
+  
+  
+   FOR UPDATE ... USING
+   
+   
+   
+   yes
+   yes
+   
+   
+  
+  
+   FOR UPDATE ... WITH CHECK
+   
+   
+   
+   yes
+   yes
+   
+   
+  
+  
+   FOR DELETE ... USING
+   
+   
+   
+   
+   
+   yes
+   yes
+  
+ 
+
+   
+
   
  
 

-- 
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] Cached plans and statement generalization

[Bruce Momjian]

On Thu, May 11, 2017 at 10:41:45PM +0300, Konstantin Knizhnik wrote:
> This is why I have provided second implementation which replace
> literals with parameters after raw parsing.  Certainly it is slower
> than first approach. But still provide significant advantage in
> performance: more than two times at pgbench.  Then I tried to run
> regression tests and find several situations where type analysis is
> not correctly performed in case of replacing literals with parameters.

So the issue is that per-command output from the parser, SelectStmt,
only has strings for identifers, e.g. table and column names, so you
can't be sure it is the same as the cached entry you matched.  I suppose
if you cleared the cache every time someone created an object or changed
search_path, it might work.

-- 
  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] snapbuild woes

[Andres Freund]

On 2017-05-11 14:54:26 -0700, Andres Freund wrote:
> On 2017-05-11 14:51:55 -0700,  wrote:
> > On 2017-05-08 00:10:12 -0700, Andres Freund wrote:
> > > I plan to commit the next pending patch after the back branch releases
> > > are cut - it's an invasive fix and the issue doesn't cause corruption
> > > "just" slow slot creation. So it seems better to wait for a few days,
> > > rather than hurry it into the release.
> > 
> > Now that that's done, here's an updated version of that patch.  Note the
> > new logic to trigger xl_running_xact's to be logged at the right spot.
> > Works well in my testing.
> > 
> > I plan to commit this fairly soon, unless somebody wants a bit more time
> > to look into it.
> > 
> > Unless somebody protests, I'd like to slightly revise how the on-disk
> > snapshots are stored on master - given the issues this bug/commit showed
> > with the current method - but I can see one could argue that that should
> > rather be done next release.
> 
> As usual I forgot my attachement.

This patch also seems to offer a way to do your 0005 in, possibly, more
efficient manner.  We don't ever need to assume a transaction is
timetravelling anymore.  Could you check whether the attached, hasty,
patch resolves the performance problems you measured?  Also, do you have
a "reference" workload for that?

Regards,

Andres
diff --git a/src/backend/replication/logical/snapbuild.c b/src/backend/replication/logical/snapbuild.c
index 0f2dcb84be..4ddd10fcf0 100644
--- a/src/backend/replication/logical/snapbuild.c
+++ b/src/backend/replication/logical/snapbuild.c
@@ -929,21 +929,31 @@ SnapBuildCommitTxn(SnapBuild *builder, XLogRecPtr lsn, TransactionId xid,
 {
 	int			nxact;
 
-	bool		forced_timetravel = false;
+	bool		needs_snapshot = false;
+	bool		needs_timetravel = false;
+
 	bool		sub_needs_timetravel = false;
-	bool		top_needs_timetravel = false;
 
 	TransactionId xmax = xid;
 
+	if (builder->state == SNAPBUILD_START)
+		return;
+
+
 	/*
-	 * If we couldn't observe every change of a transaction because it was
-	 * already running at the point we started to observe we have to assume it
-	 * made catalog changes.
-	 *
-	 * This has the positive benefit that we afterwards have enough
-	 * information to build an exportable snapshot that's usable by pg_dump et
-	 * al.
+	 * Transactions preceding BUILDING_SNAPSHOT will neither be decoded, nor
+	 * will it be part of a snapshot.  So we don't even need to record
+	 * anything.
 	 */
+	if (builder->state == SNAPBUILD_BUILDING_SNAPSHOT &&
+		TransactionIdPrecedes(xid, SnapBuildNextPhaseAt(builder)))
+	{
+		/* ensure that only commits after this are getting replayed */
+		if (builder->start_decoding_at <= lsn)
+			builder->start_decoding_at = lsn + 1;
+		return;
+	}
+
 	if (builder->state < SNAPBUILD_CONSISTENT)
 	{
 		/* ensure that only commits after this are getting replayed */
@@ -951,12 +961,13 @@ SnapBuildCommitTxn(SnapBuild *builder, XLogRecPtr lsn, TransactionId xid,
 			builder->start_decoding_at = lsn + 1;
 
 		/*
-		 * We could avoid treating !SnapBuildTxnIsRunning transactions as
-		 * timetravel ones, but we want to be able to export a snapshot when
-		 * we reached consistency.
+		 * If we're building an exportable snapshot, force recording of the
+		 * xid, even if the transaction doesn't modify the catalog.
 		 */
-		forced_timetravel = true;
-		elog(DEBUG1, "forced to assume catalog changes for xid %u because it was running too early", xid);
+		if (builder->building_full_snapshot)
+		{
+			needs_timetravel = true;
+		}
 	}
 
 	for (nxact = 0; nxact < nsubxacts; nxact++)
@@ -964,23 +975,13 @@ SnapBuildCommitTxn(SnapBuild *builder, XLogRecPtr lsn, TransactionId xid,
 		TransactionId subxid = subxacts[nxact];
 
 		/*
-		 * If we're forcing timetravel we also need visibility information
-		 * about subtransaction, so keep track of subtransaction's state.
-		 */
-		if (forced_timetravel)
-		{
-			SnapBuildAddCommittedTxn(builder, subxid);
-			if (NormalTransactionIdFollows(subxid, xmax))
-xmax = subxid;
-		}
-
-		/*
 		 * Add subtransaction to base snapshot if it DDL, we don't distinguish
 		 * to toplevel transactions there.
 		 */
-		else if (ReorderBufferXidHasCatalogChanges(builder->reorder, subxid))
+		if (ReorderBufferXidHasCatalogChanges(builder->reorder, subxid))
 		{
 			sub_needs_timetravel = true;
+			needs_snapshot = true;
 
 			elog(DEBUG1, "found subtransaction %u:%u with catalog changes.",
  xid, subxid);
@@ -990,21 +991,26 @@ SnapBuildCommitTxn(SnapBuild *builder, XLogRecPtr lsn, TransactionId xid,
 			if (NormalTransactionIdFollows(subxid, xmax))
 xmax = subxid;
 		}
+		/*
+		 * If we're forcing timetravel we also need visibility information
+		 * about subtransaction, so keep track of subtransaction's state, even
+		 * if not catalog modifying.
+		 */
+		else if (needs_timetravel)
+		{
+			SnapBuildAddCommittedTxn(builder, subxid);
+			if (NormalTransactionIdFollows(subxid, xmax))
+xmax = subxid;
+		}
 	}
 
-	if (forc

Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

[Tom Lane]

"Bossart, Nathan"  writes:
> Looking forward to any feedback that you have.

You probably won't get much in the near term, because we're in
stabilize-the-release mode not develop-new-features mode.
Please add your patch to the pending commitfest
https://commitfest.postgresql.org/14/
so that we remember to look at it when the time comes.

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] Row Level Security Documentation

[Rod Taylor]

Of course, better thoughts appear immediately after hitting the send button.

This version of the table attempts to stipulate which section of the
process the rule applies to.

On Thu, May 11, 2017 at 8:40 PM, Rod Taylor  wrote:

> I think the biggest piece missing is something to summarize the giant
> blocks of text.
>
> Attached is a table that has commands and policy types, and a "yes" if it
> applies.
>
> --
> Rod Taylor
>



-- 
Rod Taylor
diff --git a/doc/src/sgml/ref/create_policy.sgml b/doc/src/sgml/ref/create_policy.sgml
index 3b24e5e95e..4c997a956d 100644
--- a/doc/src/sgml/ref/create_policy.sgml
+++ b/doc/src/sgml/ref/create_policy.sgml
@@ -389,6 +389,72 @@ CREATE POLICY name ON 
 

+
+   Policies Applied During Statement
+
+ 
+  
+   Policy
+   SELECT
+   INSERT
+   UPDATE
+   DELETE
+  
+ 
+ 
+  
+   FOR ALL ... USING
+   WHERE clause
+   RETURNING clause
+   WHERE and RETURNING clause
+   WHERE and RETURNING clause
+  
+  
+   FOR ALL ... WITH CHECK
+   
+   new tuple
+   new tuple
+   new tuple
+  
+  
+   FOR SELECT ... USING
+   WHERE clause
+   RETURNING clause
+   WHERE and RETURNING clause
+   WHERE and RETURNING clause
+  
+  
+   FOR INSERT ... WITH CHECK
+   
+   new tuple
+   
+   
+  
+  
+   FOR UPDATE ... USING
+   
+   
+   WHERE clause
+   
+  
+  
+   FOR UPDATE ... WITH CHECK
+   
+   
+   new tuple
+   
+  
+  
+   FOR DELETE ... USING
+   
+   
+   
+   WHERE clause
+  
+ 
+
+   
+
   
  
 

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

Re: [HACKERS] [POC] hash partitioning

[Robert Haas]

On Thu, May 11, 2017 at 12:02 PM, Dilip Kumar  wrote:
> We need to add PARTITION_STRATEGY_HASH as well, we don't support NULL
> for hash also, right?

I think it should.

Actually, I think that not supporting nulls for range partitioning may
have been a fairly bad decision.

-- 
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] [Proposal] Allow users to specify multiple tables in VACUUM commands

[Bossart, Nathan]

On 5/11/17, 6:32 PM, "Tom Lane"  wrote:
> You probably won't get much in the near term, because we're in
> stabilize-the-release mode not develop-new-features mode.
> Please add your patch to the pending commitfest
> https://commitfest.postgresql.org/14/
> so that we remember to look at it when the time comes.

Understood.  I’ve added it to the commitfest.

Nathan


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

Re: [HACKERS] Bug in pg_dump --table and --exclude-table for declarative partition table handling.

[Robert Haas]

On Thu, May 11, 2017 at 9:33 AM, Tom Lane  wrote:
> Uh ... what in that is creating the already-extant parent?

/me looks embarrassed.

Never mind.  I didn't read what you wrote carefully enough.

>> I think one answer to the original complaint might be to add a new
>> flag to pg_dump, something like --recursive-selection, maybe -r for
>> short, which makes --table, --exclude-table, and --exclude-table-data
>> cascade to inheritance descendents.
>
> Yeah, you could do it like that.  Another way to do it would be to
> create variants of all the selection switches, along the lines of
> "--table-all=foo" meaning "foo plus its children".  Then you could
> have some switches recursing and others not within the same command.
> But maybe that's more flexibility than needed ... and I'm having a
> hard time coming up with nice switch names, anyway.

I don't think that's as good.  It's a lot more typing than what I
proposed and I don't think anyone is really going to want the
flexibility.

> Anyway, I'm still of the opinion that it's fine to leave this as a
> future feature.  If we've gotten away without it this long for
> inherited tables, it's unlikely to be critical for partitioned tables.

+1.

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


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

Re: [HACKERS] Moving relation extension locks out of heavyweight lock manager

[Robert Haas]

On Wed, May 10, 2017 at 8:39 PM, Masahiko Sawada  wrote:
> Currently, the relation extension lock is implemented using
> heavyweight lock manager and almost functions (except for
> brin_page_cleanup) using LockRelationForExntesion use it with
> ExclusiveLock mode. But actually it doesn't need multiple lock modes
> or deadlock detection or any of the other functionality that the
> heavyweight lock manager provides. I think It's enough to use
> something like LWLock. So I'd like to propose to change relation
> extension lock management so that it works using LWLock instead.

That's not a good idea because it'll make the code that executes while
holding that lock noninterruptible.

Possibly something based on condition variables would work better.

-- 
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] Addition of pg_dump --no-publications

[Michael Paquier]

On Thu, May 11, 2017 at 3:19 PM, Michael Paquier
 wrote:
> I imagine that pg_dump -s would be the basic operation that users
> would do first before creating a subcription on a secondary node, but
> what I find surprising is that publications are dumped by default. I
> don't find confusing that those are actually included by default to be
> consistent with the way subcriptions are handled, what I find
> confusing is that there are no options to not dump them, and no
> options to bypass their restore.
>
> So, any opinions about having pg_dump/pg_restore --no-publications?

And that's really a boring patch, giving the attached.
-- 
Michael


pgdump-no-pubs.patch
Description: Binary data

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

Re: [HACKERS] [POC] hash partitioning

[Amit Langote]

On 2017/05/12 10:42, Robert Haas wrote:
> On Thu, May 11, 2017 at 12:02 PM, Dilip Kumar  wrote:
>> We need to add PARTITION_STRATEGY_HASH as well, we don't support NULL
>> for hash also, right?
> 
> I think it should.
> 
> Actually, I think that not supporting nulls for range partitioning may
> have been a fairly bad decision.

I think the relevant discussion concluded [1] that way, because we
couldn't decide which interface to provide for specifying where NULLs are
placed or because we decided to think about it later.

Thanks,
Amit

[1]
https://www.postgresql.org/message-id/CA%2BTgmoZN_Zf7MBb48O66FAJgFe0S9_NkLVeQNBz6hsxb6Og93w%40mail.gmail.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] [Proposal] Allow users to specify multiple tables in VACUUM commands

[Michael Paquier]

On Fri, May 12, 2017 at 9:47 AM, Bossart, Nathan  wrote:
> Attached is a more complete first attempt at adding this functionality.  I 
> added two node types: one for parsing the “relation and columns” list in the 
> grammar, and one for holding the relation information we need for each call 
> to vacuum_rel(…)/analyze_rel(…).  I also added assertions and comments for 
> some undocumented assumptions that we currently rely upon.
>
> Adjustments to the documentation for VACUUM/ANALYZE and new checks in the 
> VACUUM regression test are included in this patch as well.
>
> Looking forward to any feedback that you have.

Browsing the code

 
-ANALYZE [ VERBOSE ] [ table_name [ ( column_name [, ...] ) ] ]
+ANALYZE [ VERBOSE ] [ table_name [ [ ( column_name [, ...] ) ] [, ...] ]
 
It seems to me that you don't need those extra square brackets around
the column list. Same remark for vacuum.sgml.

 
  
-  The name (possibly schema-qualified) of a specific table to
+  The name (possibly schema-qualified) of the specific tables to
   analyze.  If omitted, all regular tables, partitioned tables, and
   materialized views in the current database are analyzed (but not
-  foreign tables).  If the specified table is a partitioned table, both the
+  foreign tables).  If a specified table is a partitioned table, both the
   inheritance statistics of the partitioned table as a whole and
   statistics of the individual partitions are updated.
  
Don't think that's needed. table_name is still referencing a single
table name. And similar remark for vacuum.sgml.

In short for all that, it is enough to have "[, ... ]" to document
that a list is accepted.

/* Now go through the common routine */
-   vacuum(vacstmt->options, vacstmt->relation, InvalidOid, ¶ms,
-  vacstmt->va_cols, NULL, isTopLevel);
+   vacuum(vacstmt->options, vacstmt->rels, InvalidOid, ¶ms,
+  NULL, isTopLevel);
 }
It seems to me that it would have been less invasive to loop through
vacuum() for each relation. Do you foresee advantages in allowing
vacuum() to handle multiple? I am not sure if is is worth complicating
the current logic more considering that you have as well so extra
logic to carry on option values.

+ * used for error messages.  In the case that relid is valid, rels
+ * must have exactly one element corresponding to the specified relid.
s/rels/relations/ as variable name?
-- 
Michael


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