date:20170417

On 2017/04/18 15:31, Kang Yuzhe wrote:
> My question is why is that there is a lot of hands-on about PG application
> development(eg. connecting to PG using JAVA/JDBC) but almost nothing about
> PG hacking hands-on lessons. For example, I wanna add the keyword
> "Encrypted" in CREATE TABLE t1(a int, b int encrypted) or CREATE TABLE t1(a
> int, b int) encrypted. Alas, its not easy task.

Regarding this part, at one of the links shared above [1], you can find
presentations with hands-on instructions about how to implement a new SQL
functionality by modifying various parts of the source code.  See these:

Implementing a TABLESAMPLE clause (by Neil Conway)
http://www.neilconway.org/talks/hacking/ottawa/ottawa_slides.pdf

Add support for the WHEN clause to the CREATE TRIGGER statement (by Neil
Conway)
http://www.neilconway.org/talks/hacking/hack_slides.pdf

(by Gavin Sherry)
https://linux.org.au/conf/2007/att_data/Miniconfs(2f)PostgreSQL/attachments/hacking_intro.pdf

Handout: The Implementation of TABLESAMPLE
http://www.neilconway.org/talks/hacking/ottawa/ottawa_handout.pdf

Handout: Adding WHEN clause to CREATE TRIGGER
http://www.neilconway.org/talks/hacking/hack_handout.pdf

Some of the details might be dated, because they were written more than 10
years ago, but will definitely get you motivated to dive more into the
source code.

Thanks,
Amit

[1] http://www.neilconway.org/talks/hacking/



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

Re: [HACKERS] Parallel Append implementation

2017-04-17 Thread Amit Khandekar

On 7 April 2017 at 20:35, Andres Freund  wrote:
>> But for costs such as (4, 4, 4,   20 times), the logic would give
>> us 20 workers because we want to finish the Append in 4 time units;
>> and this what we want to avoid when we go with
>> don't-allocate-too-many-workers approach.
>
> I guess, my problem is that I don't agree with that as a goal in an of
> itself.  If you actually want to run your query quickly, you *want* 20
> workers here.  The issues of backend startup overhead (already via
> parallel_setup_cost), concurrency and such cost should be modelled, not
> burried in a formula the user can't change.  If we want to make it less
> and less likely to start more workers we should make that configurable,
> not the default.
> I think there's some precedent taken from the parallel seqscan case,
> that's not actually applicable here.  Parallel seqscans have a good
> amount of shared state, both on the kernel and pg side, and that shared
> state reduces gains of increasing the number of workers.  But with
> non-partial scans such shared state largely doesn't exist.

After searching through earlier mails about parallel scan, I am not
sure whether the shared state was considered to be a potential factor
that might reduce parallel query gains, when deciding the calculation
for number of workers for a parallel seq scan. I mean even today if we
allocate 10 workers as against a calculated 4 workers count for a
parallel seq scan, they might help. I think it's just that we don't
know if they would *always* help or it would regress sometimes.


-- 
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] On How To Shorten the Steep Learning Curve Towards PG Hacking...

2017-04-17 Thread Kang Yuzhe

Thanks Kevin for taking your time and justifying the real difficult of
finding ones space/way in PG development.And thanks for your genuine advice
which I have taken it AS IS.
My question is why is that there is a lot of hands-on about PG application
development(eg. connecting to PG using JAVA/JDBC) but almost nothing about
PG hacking hands-on lessons. For example, I wanna add the keyword
"Encrypted" in "CREATE TABLE t1(a int, b int encrypted)" or "CREATE TABLE
t1(a int, b int) encrypted". Alas, its not easy task.

Lastly, I have come to understand that PG community is not harsh to newbies
and thus, I am feeling at home.

Regards,
Zeray

On Mon, Apr 17, 2017 at 6:53 PM, Kevin Grittner  wrote:

> On Tue, Mar 28, 2017 at 10:36 PM, Craig Ringer 
> wrote:
>
> > Personally I have to agree that the learning curve is very steep. Some
> > of the docs and presentations help, but there's a LOT to understand.
>
> Some small patches can be kept to a fairly narrow set of areas, and
> if you can find a similar capability to can crib technique for
> handling some of the more mysterious areas it might brush up
> against.  When I started working on my first *big* patch that was
> bound to touch many areas (around the start of development for 9.1)
> I counted lines of code and found over a million lines just in .c
> and .h files.  We're now closing in on 1.5 million lines.  That's
> not counting over 376,000 lines of documentation in .sgml files,
> over 12,000 lines of text in README* files, over 26,000 lines of
> perl code, over 103,000 lines of .sql code (60% of which is in
> regression tests), over 38,000 lines of .y code (for flex/bison
> parsing), about 9,000 lines of various type of code just for
> generating the configure file, and over 439,000 lines of .po files
> (for message translations).  I'm sure I missed a lot of important
> stuff there, but it gives some idea the challenge it is to get your
> head around it all.
>
> My first advice is to try to identify which areas of the code you
> will need to touch, and read those over.  Several times.  Try to
> infer the API to areas *that* code needs to reference from looking
> at other code (as similar to what you want to work on as you can
> find), reading code comments and README  files, and asking
> questions.  Secondly, there is a lot that is considered to be
> "coding rules" that is, as far as I've been able to tell, only
> contained inside the heads of veteran PostgreSQL coders, with
> occasional references in the discussion list archives.  Asking
> questions, proposing approaches before coding, and showing work in
> progress early and often will help a lot in terms of discovering
> these issues and allowing you to rearrange things to fit these
> conventions.  If someone with the "gift of gab" is able to capture
> these and put them into a readily available form, that would be
> fantastic.
>
> > * SSI (haven't gone there yet myself)
>
> For anyone wanting to approach this area, there is a fair amount to
> look at.  There is some overlap, but in rough order of "practical"
> to "theoretical foundation", you might want to look at:
>
> https://www.postgresql.org/docs/current/static/transaction-iso.html
>
> https://wiki.postgresql.org/wiki/SSI
>
> The SQL standard
>
> https://git.postgresql.org/gitweb/?p=postgresql.git;a=
> blob_plain;f=src/backend/storage/lmgr/README-SSI;hb=refs/heads/master
>
> http://www.vldb.org/pvldb/vol5.html
>
> http://hdl.handle.net/2123/5353
>
> Papers cited in these last two.  I have found papers authored by
> Alan Fekete or Adul Adya particularly enlightening.
>
> If any of the other areas that Craig listed have similar work
> available, maybe we should start a Wiki page where we list areas of
> code (starting with the list Craig included) as section headers, and
> put links to useful reading below each?
>
> --
> Kevin Grittner
> VMware vCenter Server
> https://www.vmware.com/
>

Re: [HACKERS] Failed recovery with new faster 2PC code

2017-04-17 Thread Nikhil Sontakke

On 17 April 2017 at 15:02, Nikhil Sontakke  wrote:

>
>
>> >> commit 728bd991c3c4389fb39c45dcb0fe57e4a1dccd71
>> >> Author: Simon Riggs 
>> >> Date:   Tue Apr 4 15:56:56 2017 -0400
>> >>
>> >>Speedup 2PC recovery by skipping two phase state files in normal
>> path
>> >
>> > Thanks Jeff for your tests.
>> >
>> > So that's now two crash bugs in as many days and lack of clarity about
>> > how to fix it.
>> >
>>
>
> The issue seems to be that a prepared transaction is yet to be committed.
But autovacuum comes in and causes the clog to be truncated beyond this
prepared transaction ID in one of the runs.

We only add the corresponding pgproc entry for a surviving 2PC transaction
on completion of recovery. So could be a race condition here. Digging in
further.

Regards,
Nikhils
-- 
 Nikhil Sontakke   http://www.2ndQuadrant.com/
 PostgreSQL/Postgres-XL Development, 24x7 Support, Training & Services

Re: [HACKERS] Possible problem in Custom Scan API

2017-04-17 Thread Dmitry Ivanov


Tom Lane wrote:

I'm unimpressed by this part --- we couldn't back-patch such a change, and
I think it's unnecessary anyway in 9.6+ because the scan provider could
generate a nondefault pathtarget if it wants this to happen.


You're right, of course. Thank you very much!


--
Dmitry Ivanov
Postgres Professional: http://www.postgrespro.com
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] On How To Shorten the Steep Learning Curve Towards PG Hacking...

2017-04-17 Thread Kang Yuzhe

Thanks Alvaro for taking your time and pointing me to "Developer_FAQ". I
knew this web page and there is good stuff int it.
The most important about "Developer_FAQ" which I believe is that it lists
vital books for PG developers.

Comparing the real challenge I am facing in finding my way in the rabbit
role(the PG source code), "Developer_FAQ" is indeed less useful.

Of course, I am a beginner and I am just beginning and one day I hope with
your support I will figure out to find my space in PG development.

My question is why is that there is a lot of hands-on about PG application
development(eg. connecting to PG using JAVA/JDBC) but almost nothing about
PG hacking hands-on lessons. For example, I wanna add the keyword
"Encrypted" in CREATE TABLE t1(a int, b int encrypted) or CREATE TABLE t1(a
int, b int) encrypted. Alas, its not easy task.

Regards,
Zeray

On Mon, Apr 17, 2017 at 8:29 PM, Alvaro Herrera 
wrote:

> Craig Ringer wrote:
>
> > Personally I have to agree that the learning curve is very steep. Some
> > of the docs and presentations help, but there's a LOT to understand.
>
> There is a wiki page "Developer_FAQ" which is supposed to help answer
> these questions.  It is currently not very useful, because people
> stopped adding to it very early and is now mostly unmaintained, but
> I'm sure it could become a very useful central resource for this kind of
> information.
>
> --
> Álvaro Herrerahttps://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>

Re: [HACKERS] identity columns

2017-04-17 Thread Noah Misch

On Fri, Apr 14, 2017 at 05:56:44AM +, Noah Misch wrote:
> On Thu, Apr 06, 2017 at 10:26:16PM -0700, Vitaly Burovoy wrote:
> > On 4/6/17, Peter Eisentraut  wrote:
> > > On 4/4/17 22:53, Vitaly Burovoy wrote:
> > >> The next nitpickings to the last patch. I try to get places with
> > >> lacking of variables' initialization.
> > >> All other things seem good for me now. I'll continue to review the
> > >> patch while you're fixing the current notes.
> > >
> > > Committed with your changes (see below) as well as executor fix.
> > 
> > Thank you very much!
> > 
> > 
> > > As I tried to mention earlier, it is very difficult to implement the IF
> > > NOT EXISTS behavior here, because we need to run the commands the create
> > > the sequence before we know whether we will need it.
> > 
> > In fact with the function "get_attidentity" it is not so hard. Please,
> > find the patch attached.
> > I've implement SET GENERATED ... IF NOT EXISTS. It must be placed
> > before other SET options but fortunately it conforms with the
> > standard.
> > Since that form always changes the sequence behind the column, I
> > decided to explicitly write "[NO] CACHE" in pg_dump.
> > 
> > As a plus now it is possible to rename the sequence behind the column
> > by specifying SEQUENCE NAME in SET GENERATED.
> > 
> > I hope it is still possible to get rid of the "ADD GENERATED" syntax.
> 
> [Action required within three days.  This is a generic notification.]
> 
> The above-described topic is currently a PostgreSQL 10 open item.  Peter,
> since you committed the patch believed to have created it, you own this open
> item.  If some other commit is more relevant or if this does not belong as a
> v10 open item, please let us know.  Otherwise, please observe the policy on
> open item ownership[1] and send a status update within three calendar days of
> this message.  Include a date for your subsequent status update.  Testers may
> discover new open items at any time, and I want to plan to get them all fixed
> well in advance of shipping v10.  Consequently, I will appreciate your efforts
> toward speedy resolution.  Thanks.
> 
> [1] 
> https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com

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


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

Re: [HACKERS] Other formats in pset like markdown, rst, mediawiki

2017-04-17 Thread Jan Michálek

2017-04-18 2:27 GMT+02:00 Fabien COELHO :

>
> Hello Jan,
>
> Corrected problem with \pset linestyle when format is set to markdown, or
>> rst.
>>
>> Corrected tuples only for markdown and rst (normal and expanded)
>>
>
> It seems that the patch does not apply anymore on head due to changes in
> psql non regression tests. Could you rebase?
>

Hi
This patch willl not be in 10, because i haven`t it already completed on
commit freeze.

Should I update it to current master? Im afraid, that some changes will be
in conflict with this.

Je;



>
> --
> Fabien.
>



-- 
Jelen
Starší čeledín datovýho chlíva

Re: [HACKERS] Quorum commit for multiple synchronous replication.

On Tue, Apr 18, 2017 at 3:04 AM, Fujii Masao  wrote:
> On Wed, Apr 12, 2017 at 2:36 AM, Masahiko Sawada  
> wrote:
>> On Thu, Apr 6, 2017 at 4:17 PM, Masahiko Sawada  
>> wrote:
>>> On Thu, Apr 6, 2017 at 10:51 AM, Noah Misch  wrote:
 On Thu, Apr 06, 2017 at 12:48:56AM +0900, Fujii Masao wrote:
> On Wed, Apr 5, 2017 at 3:45 PM, Noah Misch  wrote:
> > On Mon, Dec 19, 2016 at 09:49:58PM +0900, Fujii Masao wrote:
> >> Regarding this feature, there are some loose ends. We should work on
> >> and complete them until the release.
> >>
> >> (1)
> >> Which synchronous replication method, priority or quorum, should be
> >> chosen when neither FIRST nor ANY is specified in s_s_names? Right now,
> >> a priority-based sync replication is chosen for keeping backward
> >> compatibility. However some hackers argued to change this decision
> >> so that a quorum commit is chosen because they think that most users
> >> prefer to a quorum.
> >>
> >> (2)
> >> There will be still many source comments and documentations that
> >> we need to update, for example, in high-availability.sgml. We need to
> >> check and update them throughly.
> >>
> >> (3)
> >> The priority value is assigned to each standby listed in s_s_names
> >> even in quorum commit though those priority values are not used at all.
> >> Users can see those priority values in pg_stat_replication.
> >> Isn't this confusing? If yes, it might be better to always assign 1 as
> >> the priority, for example.
> >
> > [Action required within three days.  This is a generic notification.]
> >
> > The above-described topic is currently a PostgreSQL 10 open item.  
> > Fujii,
> > 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
>
> Thanks for the notice!
>
> Regarding the item (2), Sawada-san told me that he will work on it after
> this CommitFest finishes. So we would receive the patch for the item from
> him next week. If there will be no patch even after the end of next week
> (i.e., April 14th), I will. Let's wait for Sawada-san's action at first.

 Sounds reasonable; I will look for your update on 14Apr or earlier.

> The items (1) and (3) are not bugs. So I don't think that they need to be
> resolved before the beta release. After the feature freeze, many users
> will try and play with many new features including quorum-based syncrep.
> Then if many of them complain about (1) and (3), we can change the code
> at that timing. So we need more time that users can try the feature.

 I've moved (1) to a new section for things to revisit during beta.  If 
 someone
 feels strongly that the current behavior is Wrong and must change, speak 
 up as
 soon as you reach that conclusion.  Absent such arguments, the behavior 
 won't
 change.

> BTW, IMO (3) should be fixed so that pg_stat_replication reports NULL
> as the priority if quorum-based sync rep is chosen. It's less confusing.

 Since you do want (3) to change, please own it like any other open item,
 including the mandatory status updates.
>>>
>>> I agree to report NULL as the priority. I'll send a patch for this as well.
>>>
>>> Regards,
>>>
>>
>> Attached two draft patches. The one makes pg_stat_replication.sync
>> priority report NULL if in quorum-based sync replication. To prevent
>> extra change I don't change so far the code of setting standby
>> priority. The another one improves the comment and documentation. If
>> there is more thing what we need to mention in documentation please
>> give me feedback.
>
> Attached is the modified version of the doc improvement patch.
> Barring any objection, I will commit this version.

Thank you for updating the patch.

>
> +In term of performance there is difference between two synchronous
> +replication method. Generally quorum-based synchronous replication
> +tends to be higher performance than priority-based synchronous
> +replication. Because in quorum-based synchronous replication, the
> +transaction can resume as soon as received the specified number of
> +acknowledge

Re: [HACKERS] Logical replication and inheritance

On 2017/04/14 21:44, Petr Jelinek wrote:
> On 14/04/17 06:14, Amit Langote wrote:
>> On 2017/04/14 10:57, Petr Jelinek wrote:
>>> For me the current behavior with inherited tables is correct.
>>
>> By the way, what do you think about the pg_dump example/issue I mentioned?
>>  Is that a pg_dump problem or backend?  To reiterate, if you add an
>> inheritance parent to a publication, dump the database, and restore it
>> into another, an error occurs.  Why?  Because every child table is added
>> *twice* because of the way publication tables are dumped.  Once by itself
>> and again via inheritance expansion when the parent is added.  Adding a
>> table again to the same publication is currently an error, which I was
>> wondering if it could be made a no-op instead.
>>
> 
> That's good question. I think it's fair to treat membership of table in
> publication is "soft object" or "property" rather than real object where
> we would enforce error on ADD of something that's already there. So I am
> not against changing it to no-op (like doing alter sequence owned by to
> column which is the current owner already).

The patch committed by Peter should be enough for the pg_dump issue I was
talking about (a pg_dump fix after all).  It seems that at least Tom and
Robert are not too excited about making duplicate membership addition a
no-op, so I don't intend to push for it.

>>> What I would like partitioned tables support to look like is that if we
>>> add partitioned table, the data decoded from any of the partitions would
>>> be sent as if the change was for that partitioned table so that the
>>> partitioning scheme on subscriber does not need to be same as on
>>> publisher. That's nontrivial to do though probably.
>>
>> I agree that it'd be nontrivial.  I'm not sure if you're also implying
>> that a row decoded from a partition is *never* published as having been
>> inserted into the partition itself.  A row can end up in a partition via
>> both the inserts into the partitioned table and the partition itself.
>> Also, AFAICT, obviously the output pluggin would have to have a dedicated
>> logic to determine which table to publish a given row as coming from
>> (possibly the root partitioned table), since nothing decode-able from WAL
>> is going to have that information.
>>
> 
> Well, yes that what I mean by nontrivial, IMHO the hard part is defining
> behavior (the coding is the easy part here). I think there are more or
> less 3 options, a) either partitions can't be added to publication
> individually or b) they will always publish their data as their main
> partitioned table (which for output plugin means same thing, ie we'll
> only see the rows as changes to partitioned tables) or alternatively c)
> if partition is added and partitioned table is added we publish changes
> twice, but that seems like quite bad option to me.

Note that (a) will amount to reversing what v10 will support, that is, can
publish individual leaf partitions, but not the partitioned tables.

About (b): sounds good, but not sure of the interface.

About (c): agreed that a bad option

> This was BTW the reason why I was saying in the original partitioning
> thread that it's unclear to me from documentation what is general
> guiding principle in terms of threating partitions as individual objects
> or not. Currently it's mixed bag, structure is treated as global for
> whole partitioned tree, but things like indexes can be defined
> separately on individual partitions. Also existing tables retain some of
> their differences when they are being added as partitions but other
> differences are strictly checked and will result in error. I don't quite
> understand if this is current implementation limitation and we'll
> eventually "lock down" the differences (when we have global indexes and
> such) or if it's intended long term to allow differences between
> partitions and what will be the rules for what's allowed and what not.
> 
>> Also, with the current state of partitioning, if a row decoded and
>> published as coming from the partitioned table had no corresponding
>> partition defined on the destination server, an error will occur in the
>> subscription worker I'd guess.  Or may be we don't assume anything about
>> whether the table on the subscription end is partitioned or not.
>>
>> Anyway, that perhaps also means that for time being, we might need to go
>> with the following option that Robert mentioned (I suppose strictly in the
>> context of partitioned tables, not general inheritance):
>>
>> (1) That's an error; the user should publish the partitions instead.
>>
> 
> Yes I agree with Robert's statement and that's how it should behave already.

Yes, it does.

> 
>> That is, we should make adding a partitioned table to a publication a user
>> error (feature not supported).
>>
> 
> It already should be error no? (Unless some change was committed that I
> missed, I definitely wrote it as error in original patch).

Oh, you're right.  Although, th

Re: [HACKERS] Comments not allowed on partitioned table columns

On 2017/04/18 14:50, Amit Langote wrote:
> Attached patch fixes the oversight that COMMENT ON COLUMN is not allowed
> on partitioned tables columns.

Forgot to mention that I added this to the open items list.

Thanks,
Amit



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

[HACKERS] Comments not allowed on partitioned table columns

Attached patch fixes the oversight that COMMENT ON COLUMN is not allowed
on partitioned tables columns.

Thanks,
Amit
>From cfc0717478f2a73087c85d67d73e557aaccb4f78 Mon Sep 17 00:00:00 2001
From: amit 
Date: Tue, 18 Apr 2017 14:48:37 +0900
Subject: [PATCH] Allow COMMENT ON partitioned table columns

---
 src/backend/commands/comment.c |  3 ++-
 src/test/regress/expected/create_table.out | 19 +++
 src/test/regress/sql/create_table.sql  |  8 
 3 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/src/backend/commands/comment.c b/src/backend/commands/comment.c
index b5569bddaf..1c17927c49 100644
--- a/src/backend/commands/comment.c
+++ b/src/backend/commands/comment.c
@@ -94,7 +94,8 @@ CommentObject(CommentStmt *stmt)
 relation->rd_rel->relkind != RELKIND_VIEW &&
 relation->rd_rel->relkind != RELKIND_MATVIEW &&
 relation->rd_rel->relkind != RELKIND_COMPOSITE_TYPE &&
-relation->rd_rel->relkind != RELKIND_FOREIGN_TABLE)
+relation->rd_rel->relkind != RELKIND_FOREIGN_TABLE &&
+relation->rd_rel->relkind != RELKIND_PARTITIONED_TABLE)
 ereport(ERROR,
 		(errcode(ERRCODE_WRONG_OBJECT_TYPE),
 		 errmsg("\"%s\" is not a table, view, materialized view, composite type, or foreign table",
diff --git a/src/test/regress/expected/create_table.out b/src/test/regress/expected/create_table.out
index 6f8645ddbd..b6c75d2e81 100644
--- a/src/test/regress/expected/create_table.out
+++ b/src/test/regress/expected/create_table.out
@@ -669,3 +669,22 @@ Number of partitions: 3 (Use \d+ to list them.)
 
 -- cleanup
 DROP TABLE parted, list_parted, range_parted, list_parted2, range_parted2, range_parted3;
+-- comments on partitioned tables columns
+CREATE TABLE parted_col_comment (a int, b text) PARTITION BY LIST (a);
+COMMENT ON TABLE parted_col_comment IS 'Am partitioned table';
+COMMENT ON COLUMN parted_col_comment.a IS 'Partition key';
+SELECT obj_description('parted_col_comment'::regclass);
+   obj_description
+--
+ Am partitioned table
+(1 row)
+
+\d+ parted_col_comment
+  Table "public.parted_col_comment"
+ Column |  Type   | Collation | Nullable | Default | Storage  | Stats target |  Description  
++-+---+--+-+--+--+---
+ a  | integer |   |  | | plain|  | Partition key
+ b  | text|   |  | | extended |  | 
+Partition key: LIST (a)
+
+DROP TABLE parted_col_comment;
diff --git a/src/test/regress/sql/create_table.sql b/src/test/regress/sql/create_table.sql
index 1f0fa8e16d..b00d9e87b8 100644
--- a/src/test/regress/sql/create_table.sql
+++ b/src/test/regress/sql/create_table.sql
@@ -597,3 +597,11 @@ CREATE TABLE part_c_1_10 PARTITION OF part_c FOR VALUES FROM (1) TO (10);
 
 -- cleanup
 DROP TABLE parted, list_parted, range_parted, list_parted2, range_parted2, range_parted3;
+
+-- comments on partitioned tables columns
+CREATE TABLE parted_col_comment (a int, b text) PARTITION BY LIST (a);
+COMMENT ON TABLE parted_col_comment IS 'Am partitioned table';
+COMMENT ON COLUMN parted_col_comment.a IS 'Partition key';
+SELECT obj_description('parted_col_comment'::regclass);
+\d+ parted_col_comment
+DROP TABLE parted_col_comment;
-- 
2.11.0


-- 
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 and \password

2017-04-17 Thread Noah Misch

On Tue, Apr 11, 2017 at 10:07:12PM +0300, Heikki Linnakangas wrote:
> On 04/10/2017 08:42 AM, Michael Paquier wrote:
> >As there have been some conflicts because of the commit of SASLprep,
> >here is a rebased set of patches. A couple of things worth noting:
> >- SASLprep does an allocation of the prepared password string. It is
> >definitely better to do all the ground work in pg_saslprep but this
> >costs a free() call with a #ifdef FRONTEND at the end of
> >scram_build_verifier().
> >- Patch 0005 does that:
> >+   /*
> >+* Hash password using SCRAM-SHA-256 when connecting to servers
> >+* newer than Postgres 10, and hash with MD5 otherwise.
> >+*/
> >+   if (pset.sversion < 10)
> >+   encrypted_password = PQencryptPassword(pw1, user, "md5");
> >+   else
> >+   encrypted_password = PQencryptPassword(pw1, user, "scram");
> >Actually I am thinking that guessing the hashing function according to
> >the value of password_encryption would make the most sense. Thoughts?
> 
> Thanks! I've been busy on the other thread on future-proofing the protocol
> with negotiating the SASL mechanism, I'll come back to this once we get that
> settled. By the end of the week, I presume.

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] Re: Query fails when SRFs are part of FROM clause (Commit id: 69f4b9c85f)

On 2017-04-17 19:26:11 -0400, Tom Lane wrote:
> I wrote:
> > I'm a bit inclined to agree with the idea of explicitly requiring SRFs
> > in FROM to appear only at the top level of the expression.
> 
> If we are going to go down this road, I think it would be a good idea
> to try to provide a cursor position for the "can't accept a set" error
> message, because otherwise it will be really unclear what's wrong with
> something like

That sounds like a good plan.


> Now that this infrastructure exists, anything that has access to a
> PlanState or ExprContext, plus a parse node containing a location, is able
> to report an error cursor.

I've wished for that ability for a while...


> It took a considerable effort of will not to
> start plastering error position reports on a lot of other executor errors.
> I think we should do that, but it smells new-feature-y, and hence
> something to tackle for v11 not now.  But if v10 is moving the goalposts
> on where you can put an SRF call, I think we should do this much in v10.

FWIW, I'd be ok with relaxing things a bit around this, but let's get
the minimal thing in first.


> Naming note: a name like ExecErrPosition() would have been more consistent
> with the other stuff that execUtils.c exports.  But since this is meant
> to be used in ereport() calls, whose support functions generally aren't
> camel-cased, I thought executor_errposition() was a better choice.
> I'm not particularly set on that though.

Seems slightly better this way.


Looks good to me.


I'm working on a patch that adds a few more tests around the current
behaviour and that updates the code to match what we now know.

I couldn't find any place in the docs that actually documents our SRF
behaviour in any sort of detail ([1], [2]), particularly not documenting
where SRFs are legal, and how nested SRFs are supposed to behave.  I'm
not sure in how much detail we want to go?  If we want do document that,
where?  The closest seems to be "4.2.14. Expression Evaluation Rules"
[3].

Greetings,

Andres Freund

[1] https://www.postgresql.org/docs/devel/static/sql-select.html#sql-from
[2] 
https://www.postgresql.org/docs/devel/static/queries-table-expressions.html#queries-from
[3] 
https://www.postgresql.org/docs/devel/static/sql-expressions.html#syntax-express-eval


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

Re: [HACKERS] pg_dump emits ALTER TABLE ONLY partitioned_table

Hi Stephen,

On 2017/04/18 1:43, Stephen Frost wrote:
> Amit,
> 
> * Amit Langote (langote_amit...@lab.ntt.co.jp) wrote:
>> OK, I agree.  I tweaked the existing bullet point about differences from
>> traditional inheritance when using ONLY with partitioned tables.
> 
> Please take a look at the attached and let me know your thoughts on it.
> I changed the code to complain again regarding TRUNCATE ONLY, since that
> never actually makes sense on a partitioned table, unlike ALTER TABLE
> ONLY, and adjusted the error messages and added hints.

Thanks for polishing the patch.  It looks mostly good to me.

+ Once
+   partitions exist, we do not support using ONLY to
+   add or drop constraints on only the partitioned table.

I wonder if the following sounds a bit more informative: Once partitions
exist, using ONLY will result in an error, because the
constraint being added or dropped must be added or dropped from the
partitions too.

>> Updated patches attached (0002 and 0003 unchanged).
> 
> Regarding these, 0002 changes pg_dump to handle partitions much more
> like inheritance, which at least seems like a decent idea but makes me a
> bit concerned about making sure we're doing everything correctly.

Are you concerned about the correctness of the code in pg_dump or backend?

Rules of inheritance enforced by flagInhAttrs() are equally applicable to
the partitioning case, so actions performed by it for the partitioning
cases will not emit incorrect text.

The rule that backend follows is this: if a constraint is added to the
partitioned table (either a named check constraint or a NOT NULL
constraint), that constraint becomes an inherited *non-local* constraint
in all the partitions no matter if it was present in the partitions before
or not.

> In
> particular, we should really have regression tests for non-inherited
> CHECK (and NOT NULL) constraints on partitions.  Perhaps there are tests
> covering those cases in the standard regression suite, but it's not
> obvious from the SQL.

There is one in create_table.sql that looks like this:

CREATE TABLE part_b PARTITION OF parted (
b NOT NULL DEFAULT 1 CHECK (b >= 0),
CONSTRAINT check_a CHECK (length(a) > 0)
) FOR VALUES IN ('b');
-- conislocal should be false for any merged constraints
SELECT conislocal, coninhcount FROM pg_constraint WHERE conrelid =
'part_b'::regclass AND conname = 'check_a';

But it doesn't really look like it's testing non-inherited constraints a
whole lot (CHECK (b >= 0) above is a local non-inherited constraint).

I added a few more tests right below the above test so that there is a bit
more coverage.  Keep the rule I mentioned above when looking at these.  I
also added a test in the pg_dump suite.  That's in patch 0001 (since you
posted your version of what was 0001 before, I'm no longer including it here).

By the way, 0003 is a bug-fix.  WITH OPTIONS that is emitted currently
like below is not the acceptable syntax:

CREATE TABLE a_partition OF a_partitioned_table (
a WITH OPTIONS NOT NULL DEFAULT 1
) FOR VALUES blah

But looking at the pg_dump code closely and also considering our
discussion upthread, I don't think this form is ever emitted.  Because we
never emit a partition's column-level constraints and column defaults in
the CREATE TABLE, but rather separately using ALTER TABLE.  In any case,
applying 0003 seems to be the right thing to do anyway, in case we might
rejigger things so that whatever can be emitted within the CREATE TABLE
text will be.

Thanks,
Amit
>From eba30bebbd29f79af778c9526e2d205e721f38c0 Mon Sep 17 00:00:00 2001
From: amit 
Date: Tue, 18 Apr 2017 10:59:35 +0900
Subject: [PATCH 1/3] Improve test coverage of partition constraints

Better exercise the code manipulating partition constraints a bit
differently from the traditional inheritance children.
---
 src/bin/pg_dump/t/002_pg_dump.pl   | 10 +---
 src/test/regress/expected/create_table.out | 41 ++
 src/test/regress/sql/create_table.sql  | 21 ---
 3 files changed, 55 insertions(+), 17 deletions(-)

diff --git a/src/bin/pg_dump/t/002_pg_dump.pl b/src/bin/pg_dump/t/002_pg_dump.pl
index ccd0ed6a53..bebb2f4f5d 100644
--- a/src/bin/pg_dump/t/002_pg_dump.pl
+++ b/src/bin/pg_dump/t/002_pg_dump.pl
@@ -4757,12 +4757,16 @@ qr/CREATE TRANSFORM FOR integer LANGUAGE sql \(FROM SQL WITH FUNCTION pg_catalog
 		catch_all=> 'CREATE ... commands',
 		create_order => 91,
 		create_sql => 'CREATE TABLE dump_test_second_schema.measurement_y2006m2
-	   PARTITION OF dump_test.measurement FOR VALUES
-	   FROM (\'2006-02-01\') TO (\'2006-03-01\');',
+	   PARTITION OF dump_test.measurement (
+	  unitsales DEFAULT 0 CHECK (unitsales >= 0)
+	   )
+	   FOR VALUES FROM (\'2006-02-01\') TO (\'2006-03-01\');',
 		regexp => qr/^
 			\Q-- Name: measurement_y2006m2;\E.*\n
 			\Q--\E\n\n
-			\QCREATE TABLE measurement_y2006m2 PARTITION OF dump_test.measurement\E\n
+			\QCREATE TABLE measurem

Re: [HACKERS] snapbuild woes

On 2017-04-16 22:04:04 -0400, Noah Misch wrote:
> On Thu, Apr 13, 2017 at 12:58:12AM -0400, Noah Misch wrote:
> > On Wed, Apr 12, 2017 at 10:21:51AM -0700, Andres Freund wrote:
> > > On 2017-04-12 11:03:57 -0400, Peter Eisentraut wrote:
> > > > On 4/12/17 02:31, Noah Misch wrote:
> > > > >>> But I hope you mean to commit these snapbuild patches before the 
> > > > >>> postgres 10
> > > > >>> release?  As far as I know, logical replication is still very 
> > > > >>> broken without
> > > > >>> them (or at least some of that set of 5 patches - I don't know 
> > > > >>> which ones
> > > > >>> are essential and which may not be).
> > > > >>
> > > > >> Yes, these should go into 10 *and* earlier releases, and I don't 
> > > > >> plan to
> > > > >> wait for 2017-07.
> > > > > 
> > > > > [Action required within three days.  This is a generic notification.]
> > > > 
> > > > I'm hoping for a word from Andres on this.
> > > 
> > > Feel free to reassign to me.
> > 
> > Thanks for volunteering; I'll do that shortly.  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.
> > 
> > [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.

I've since the previous update reviewed Petr's patch, which he since has
updated over the weekend.  I'll do another round tomorrow, and will see
how it looks.  I think we might need some more tests for this to be
committable, so it might not become committable tomorrow.  I hope we'll
have something in tree by end of this week, if not I'll send an update.

- 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] Optimization for updating foreign tables in Postgres FDW

At Tue, 18 Apr 2017 09:12:07 +0530, Ashutosh Bapat 
 wrote in 

> On Tue, Apr 18, 2017 at 8:12 AM, Kyotaro HORIGUCHI
>  wrote:
> > At Mon, 17 Apr 2017 17:50:58 +0530, Ashutosh Bapat 
> >  wrote in 
> > 
> >> On Mon, Apr 17, 2017 at 1:53 PM, Kyotaro HORIGUCHI
> >>  wrote:
> >> > At Thu, 13 Apr 2017 13:04:12 -0400, Robert Haas  
> >> > wrote in 
> >> > 
> >> >> On Thu, Apr 21, 2016 at 10:53 AM, Robert Haas  
> >> >> wrote:
> >> >> > On Thu, Apr 21, 2016 at 8:44 AM, Michael Paquier
> >> >> >  wrote:
> >> >> >> On Thu, Apr 21, 2016 at 5:22 PM, Etsuro Fujita
> >> >> >>  wrote:
> >> >> >>> Attached is an updated version of the patch, which modified 
> >> >> >>> Michael's
> >> >> >>> version of the patch, as I proposed in [1] (see "Other changes:").  
> >> >> >>> I
> >> >> >>> modified comments for pgfdw_get_result/pgfdw_exec_query also, 
> >> >> >>> mainly because
> >> >> >>> words like "non-blocking mode" there seems confusing (note that we 
> >> >> >>> have
> >> >> >>> PQsetnonbloking).
> >> >> >>
> >> >> >> OK, so that is what I sent except that the comments mentioning PG_TRY
> >> >> >> are moved to their correct places. That's fine for me. Thanks for
> >> >> >> gathering everything in a single patch and correcting it.
> >> >> >
> >> >> > I have committed this patch.  Thanks for working on this.  Sorry for 
> >> >> > the delay.
> >> >>
> >> >> This 9.6-era patch, as it turns out, has a problem, which is that we
> >> >> now respond to an interrupt by sending a cancel request and a
> >> >> NON-interruptible ABORT TRANSACTION command to the remote side.  If
> >> >> the reason that the user is trying to interrupt is that the network
> >> >> connection has been cut, they interrupt the original query only to get
> >> >> stuck in a non-interruptible wait for ABORT TRANSACTION.  That is
> >> >> non-optimal.
> >> >
> >> > Agreed.
> >> >
> >> >> It is not exactly clear to me how to fix this.  Could we get by with
> >> >> just slamming the remote connection shut, instead of sending an
> >> >> explicit ABORT TRANSACTION?  The remote side ought to treat a
> >> >> disconnect as equivalent to an ABORT anyway, I think, but maybe our
> >> >> local state would get confused.  (I have not checked.)
> >> >>
> >> >> Thoughts?
> >> >
> >> > Perhaps we will get stuck at query cancellation before ABORT
> >> > TRANSACTION in the case. A connection will be shut down when
> >> > anything wrong (PQstatus(conn) != CONNECTION_OK and so) on
> >> > aborting local transactoin . So I don't think fdw gets confused
> >> > or sholdn't be confused by shutting down there.
> >> >
> >> > The most significant issue I can see is that the same thing
> >> > happens in the case of graceful ABORT TRANSACTION. It could be a
> >> > performance issue.
> >> >
> >> > We could set timeout here but maybe we can just slamming the
> >> > connection down instead of sending a query cancellation. It is
> >> > caused only by timeout or interrupts so I suppose it is not a
> >> > problem *with a few connections*.
> >> >
> >> >
> >> > Things are a bit diffent with hundreds of connections. The
> >> > penalty of reconnection would be very high in the case.
> >> >
> >> > If we are not willing to pay such high penalty, maybe we are to
> >> > manage busy-idle time of each connection and trying graceful
> >> > abort if it is short enough, maybe having a shoft timeout.
> >> >
> >> > Furthermore, if most or all of the hundreds of connections get
> >> > stuck, such timeout will accumulate up like a mountain...
> >>
> >> Even when the transaction is aborted because a user cancels a query,
> >> we do want to preserve the connection, if possible, to avoid
> >
> > Yes.
> >
> >> reconnection. If the request to cancel the query itself fails, we
> >> should certainly drop the connection. Here's the patch to do that.
> >
> > A problem I think on this would be that we still try to make
> > another connection for canceling and it would stall for several
> > minutes per connection on a packet stall, which should be done in
> > a second on ordinary circumstances. Perhaps we might want here is
> > async canceling with timeout.
> 
> I am not sure what do you mean "make another connection for
> cancelling.". Do you mean to say that PQcancel would make another
> connection?

Yes. It will take about 3 minutes on standard settings when no
ACK returned. I thought that this discussion is based on such
situation.

> The patch proposed simply improves the condition when PQcancel has
> returned a failure. Right now we ignore that failure and try to ABORT
> the transaction, which is most probably going to get stalled or fail
> to be dispatched. With the patch such a connection will be reset.

Ah, I understand that. It is surely an improvement since it
avoids useless ABORT TRANSACTION that is known to stall.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center



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

Re: [HACKERS] Optimization for updating foreign tables in Postgres FDW

2017-04-17 Thread Ashutosh Bapat

On Tue, Apr 18, 2017 at 8:12 AM, Kyotaro HORIGUCHI
 wrote:
> At Mon, 17 Apr 2017 17:50:58 +0530, Ashutosh Bapat 
>  wrote in 
> 
>> On Mon, Apr 17, 2017 at 1:53 PM, Kyotaro HORIGUCHI
>>  wrote:
>> > At Thu, 13 Apr 2017 13:04:12 -0400, Robert Haas  
>> > wrote in 
>> > 
>> >> On Thu, Apr 21, 2016 at 10:53 AM, Robert Haas  
>> >> wrote:
>> >> > On Thu, Apr 21, 2016 at 8:44 AM, Michael Paquier
>> >> >  wrote:
>> >> >> On Thu, Apr 21, 2016 at 5:22 PM, Etsuro Fujita
>> >> >>  wrote:
>> >> >>> Attached is an updated version of the patch, which modified Michael's
>> >> >>> version of the patch, as I proposed in [1] (see "Other changes:").  I
>> >> >>> modified comments for pgfdw_get_result/pgfdw_exec_query also, mainly 
>> >> >>> because
>> >> >>> words like "non-blocking mode" there seems confusing (note that we 
>> >> >>> have
>> >> >>> PQsetnonbloking).
>> >> >>
>> >> >> OK, so that is what I sent except that the comments mentioning PG_TRY
>> >> >> are moved to their correct places. That's fine for me. Thanks for
>> >> >> gathering everything in a single patch and correcting it.
>> >> >
>> >> > I have committed this patch.  Thanks for working on this.  Sorry for 
>> >> > the delay.
>> >>
>> >> This 9.6-era patch, as it turns out, has a problem, which is that we
>> >> now respond to an interrupt by sending a cancel request and a
>> >> NON-interruptible ABORT TRANSACTION command to the remote side.  If
>> >> the reason that the user is trying to interrupt is that the network
>> >> connection has been cut, they interrupt the original query only to get
>> >> stuck in a non-interruptible wait for ABORT TRANSACTION.  That is
>> >> non-optimal.
>> >
>> > Agreed.
>> >
>> >> It is not exactly clear to me how to fix this.  Could we get by with
>> >> just slamming the remote connection shut, instead of sending an
>> >> explicit ABORT TRANSACTION?  The remote side ought to treat a
>> >> disconnect as equivalent to an ABORT anyway, I think, but maybe our
>> >> local state would get confused.  (I have not checked.)
>> >>
>> >> Thoughts?
>> >
>> > Perhaps we will get stuck at query cancellation before ABORT
>> > TRANSACTION in the case. A connection will be shut down when
>> > anything wrong (PQstatus(conn) != CONNECTION_OK and so) on
>> > aborting local transactoin . So I don't think fdw gets confused
>> > or sholdn't be confused by shutting down there.
>> >
>> > The most significant issue I can see is that the same thing
>> > happens in the case of graceful ABORT TRANSACTION. It could be a
>> > performance issue.
>> >
>> > We could set timeout here but maybe we can just slamming the
>> > connection down instead of sending a query cancellation. It is
>> > caused only by timeout or interrupts so I suppose it is not a
>> > problem *with a few connections*.
>> >
>> >
>> > Things are a bit diffent with hundreds of connections. The
>> > penalty of reconnection would be very high in the case.
>> >
>> > If we are not willing to pay such high penalty, maybe we are to
>> > manage busy-idle time of each connection and trying graceful
>> > abort if it is short enough, maybe having a shoft timeout.
>> >
>> > Furthermore, if most or all of the hundreds of connections get
>> > stuck, such timeout will accumulate up like a mountain...
>>
>> Even when the transaction is aborted because a user cancels a query,
>> we do want to preserve the connection, if possible, to avoid
>
> Yes.
>
>> reconnection. If the request to cancel the query itself fails, we
>> should certainly drop the connection. Here's the patch to do that.
>
> A problem I think on this would be that we still try to make
> another connection for canceling and it would stall for several
> minutes per connection on a packet stall, which should be done in
> a second on ordinary circumstances. Perhaps we might want here is
> async canceling with timeout.

I am not sure what do you mean "make another connection for
cancelling.". Do you mean to say that PQcancel would make another
connection?

The patch proposed simply improves the condition when PQcancel has
returned a failure. Right now we ignore that failure and try to ABORT
the transaction, which is most probably going to get stalled or fail
to be dispatched. With the patch such a connection will be reset.

-- 
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] Different table schema in logical replication crashes

On 4/17/17 08:47, Euler Taveira wrote:
> Patch works fine. However, I don't see any documentation about
> supporting different schemas for logical replication. Is it an oversight?

I have added more documentation about that.

-- 
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] Different table schema in logical replication crashes

On 4/14/17 21:36, Petr Jelinek wrote:
> I tried something bit different which seems cleaner to me - use the
> pstate->r_table instead of ad-hock locally made up range table and fill
> that using standard addRangeTableEntryForRelation. Both in tablesync and
> in DoCopy instead of the old coding.

committed

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


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

Re: [HACKERS] some review comments on logical rep code

Hi,

Thank you for the revised version.

At Mon, 17 Apr 2017 23:29:28 +0900, Masahiko Sawada  
wrote in 
> On Mon, Apr 17, 2017 at 9:13 PM, Masahiko Sawada  
> wrote:
> > On Mon, Apr 17, 2017 at 7:39 PM, Kyotaro HORIGUCHI
> >  wrote:
> >> At Mon, 17 Apr 2017 18:02:57 +0900, Masahiko Sawada 
> >>  wrote in 
> >> 
> >>> On Fri, Apr 14, 2017 at 4:47 AM, Fujii Masao  
> >>> wrote:
> >>> 1.
> >>> >
> >>> > In ApplyWorkerMain(), DatumGetInt32() should be used to get integer
> >>> > value from the argument, instead of DatumGetObjectId().
> >>>
> >>> Attached 001 patch fixes it.
> >>
> >> Hmm. I looked at the launcher side and found that it assigns bare
> >> integer to bgw_main_arg. It also should use Int32GetDatum.
> >
> > Yeah, I agreed. Will fix it.

Thanks.

> >>> 2.
> >>> >
> >>> > No one resets on_commit_launcher_wakeup flag to false.
> >>>
> >>> Attached 002 patch makes on_commit_launcher_wakeup turn off after woke
> >>> up the launcher process.
> >>
> >> It is omitting the abort case. Maybe it should be
> >> AtEOXact_ApplyLauncher(bool commit)?
> >
> > Should we wake up the launcher process even when abort?

No, I meant that on_commit_launcher_wakeup should be just reset
without waking up launcher on abort.

> >>> 3.
> >>> >
> >>> > ApplyLauncherWakeup() should be static function.
> >>>
> >>> Attached 003 patch fixes it (and also fixes #5 review comment).
> >>>
> >>> 4.
> >>> >
> >>> > Seems not only CREATE SUBSCRIPTION but also ALTER SUBSCRIPTION's
> >>> > subcommands (e.g., ENABLED one) should wake the launcher up on commit.
> >>>
> >>> This is also reported by Horiguchi-san on another thread[1], and patch 
> >>> exists.
> >>>
> >>> 5.
> >>> >
> >>> > Why is IsBackendPid() check necessary in ApplyLauncherWakeup()?
> >>>
> >>> I also guess it's necessary. This change is included in #3 patch.
> >>
> >> IsBackendPid() is not free, I suppose that just ignoring failure
> >> with ESRCH is enough.
> >
> > After thought, since LogicalRepCtx->launcher_pid could be 0 should, we
> > check if launcher_pid != 0?

Yes. For example, code to send a signal to autovacuum launcher
looks as the follows. I don't think there's a reason to do
different thing here.

|   avlauncher_needs_signal = false;
|   if (AutoVacPID != 0)
| kill(AutoVacPID, SIGUSR2);


> >>> 6.
> >>> >
> >>> > Normal statements that the walsender for logical rep runs are logged
> >>> > by log_replication_commands. So if log_statement is also set,
> >>> > those commands are logged twice.
> >>>
> >>> Attached 006 patch fixes it by adding  "-c log_statement = none" to
> >>> connection parameter of libpqrcv if logical = true.
> >>
> >> The control by log_replication_commands is performed on
> >> walsender, so this also shuld be done on the same place. Anyway
> >> log_statement is irrelevant to walsender.
> >
> > Patch 006 emits all logs executed by the apply worker as a log of
> > log_replication_command but perhaps you're right. It would be better
> > to leave the log of log_statement if the command executed by the apply
> > worker is a SQL command. Will fix.

Here, we can choose whether a SQL command is a part of
replication commands or not. The previous fix thought it is and
this doesn't. (They are emitted in different forms) Is this ok?

I'm not sure why you have moved the location of the code, but if
it should show all received commands, it might be better to be
placed before CHECK_FOR_INTERRUPTS..

The comment for the code seems should be more clearly.

- * compatibility. To prevent the command is logged doubly, we doesn't
- * log it when the command is a SQL command.
+ * compatibility. SQL command are logged later according
+ * to log_statement setting.

> >>> 7.
> >>> >
> >>> > LogicalRepCtx->launcher_pid isn't reset to 0 when the launcher emits
> >>> > an error. The callback function to reset it needs to be registered
> >>> > via on_shmem_exit().
> >>>
> >>> Attached 007 patch adds callback function to reset 
> >>> LogicalRepCtx->launcher_pid.
> >>>
> >>> 8.
> >>> >
> >>> > Typo: "an subscriber" should be "a subscriber" in some places.
> >>>
> >>> It seems that there is no longer these typo.
> >>>
> >>> 9.
> >>> > /* Get conninfo */
> >>> > datum = SysCacheGetAttr(SUBSCRIPTIONOID,
> >>> > tup,
> >>> > Anum_pg_subscription_subconninfo,
> >>> > &isnull);
> >>> > Assert(!isnull);
> >>> >
> >>> > This assertion makes me think that pg_subscription.subconnifo should
> >>> > have NOT NULL constraint. Also subslotname and subpublications
> >>> > should have NOT NULL constraint because of the same reason.
> >>>
> >>> Agreed. Attached 009 patch add NOT NULL constraint to all other
> >>> columns of pg_subscription. pg_subscription.subsynccommit should have
> >>> it as well.
> >>
> >> I'm not sure the policy here, but I suppose that the assertions
> >> there are still required irrelevantly from the nun-nullness of
> >> the attribute.
> >
> > You're right. Will fix it.
> >
> >>> 10.
> >>> >
> >>> > SpinLockAc

Re: [HACKERS] PATCH: psql show index with type info

2017-04-17 Thread Amos Bird


Ah, thanks for the suggestions. I'll revise this patch soon :)

Fabien COELHO  writes:

>> Done.
>
> Ok. The file should be named "v2".
>
>> Would you like to be the reviewer?
>
> Dunno. At least I wanted to have a look at it!
>
> My 0.02€:
>
> I think that the improvement provided is worthwhile.
>
> Two questions: Why no documentation update? Why no non-regressions 
> tests?
>
> As far as the output is concerned, ISTM that "btree index", "hash index" 
> and so would sound nicer than "index: sub-type".
>
> I'm wondering about the sub-query implementation: Maybe an outer join 
> could bring the same result with a more relational & elegant query.
>
> The "gettext_noop" call does not seem to make sense: the point is to 
> translate the string, and there is no way to translate "index:  query>". The result of the query should be translated if this is what is 
> wanted, and that can only be by hand:
>
>CASE amname || ' index'
>  WHEN 'hash index' THEN '%s' ...
>  WHEN ...
>  ELSE amname || ' index' # untranslated...
>END
>
> gettext_noop('hash index') # get translation for 'hash index'
> ...



-- 
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] Optimization for updating foreign tables in Postgres FDW

At Mon, 17 Apr 2017 17:50:58 +0530, Ashutosh Bapat 
 wrote in 

> On Mon, Apr 17, 2017 at 1:53 PM, Kyotaro HORIGUCHI
>  wrote:
> > At Thu, 13 Apr 2017 13:04:12 -0400, Robert Haas  
> > wrote in 
> > 
> >> On Thu, Apr 21, 2016 at 10:53 AM, Robert Haas  
> >> wrote:
> >> > On Thu, Apr 21, 2016 at 8:44 AM, Michael Paquier
> >> >  wrote:
> >> >> On Thu, Apr 21, 2016 at 5:22 PM, Etsuro Fujita
> >> >>  wrote:
> >> >>> Attached is an updated version of the patch, which modified Michael's
> >> >>> version of the patch, as I proposed in [1] (see "Other changes:").  I
> >> >>> modified comments for pgfdw_get_result/pgfdw_exec_query also, mainly 
> >> >>> because
> >> >>> words like "non-blocking mode" there seems confusing (note that we have
> >> >>> PQsetnonbloking).
> >> >>
> >> >> OK, so that is what I sent except that the comments mentioning PG_TRY
> >> >> are moved to their correct places. That's fine for me. Thanks for
> >> >> gathering everything in a single patch and correcting it.
> >> >
> >> > I have committed this patch.  Thanks for working on this.  Sorry for the 
> >> > delay.
> >>
> >> This 9.6-era patch, as it turns out, has a problem, which is that we
> >> now respond to an interrupt by sending a cancel request and a
> >> NON-interruptible ABORT TRANSACTION command to the remote side.  If
> >> the reason that the user is trying to interrupt is that the network
> >> connection has been cut, they interrupt the original query only to get
> >> stuck in a non-interruptible wait for ABORT TRANSACTION.  That is
> >> non-optimal.
> >
> > Agreed.
> >
> >> It is not exactly clear to me how to fix this.  Could we get by with
> >> just slamming the remote connection shut, instead of sending an
> >> explicit ABORT TRANSACTION?  The remote side ought to treat a
> >> disconnect as equivalent to an ABORT anyway, I think, but maybe our
> >> local state would get confused.  (I have not checked.)
> >>
> >> Thoughts?
> >
> > Perhaps we will get stuck at query cancellation before ABORT
> > TRANSACTION in the case. A connection will be shut down when
> > anything wrong (PQstatus(conn) != CONNECTION_OK and so) on
> > aborting local transactoin . So I don't think fdw gets confused
> > or sholdn't be confused by shutting down there.
> >
> > The most significant issue I can see is that the same thing
> > happens in the case of graceful ABORT TRANSACTION. It could be a
> > performance issue.
> >
> > We could set timeout here but maybe we can just slamming the
> > connection down instead of sending a query cancellation. It is
> > caused only by timeout or interrupts so I suppose it is not a
> > problem *with a few connections*.
> >
> >
> > Things are a bit diffent with hundreds of connections. The
> > penalty of reconnection would be very high in the case.
> >
> > If we are not willing to pay such high penalty, maybe we are to
> > manage busy-idle time of each connection and trying graceful
> > abort if it is short enough, maybe having a shoft timeout.
> >
> > Furthermore, if most or all of the hundreds of connections get
> > stuck, such timeout will accumulate up like a mountain...
> 
> Even when the transaction is aborted because a user cancels a query,
> we do want to preserve the connection, if possible, to avoid

Yes.

> reconnection. If the request to cancel the query itself fails, we
> should certainly drop the connection. Here's the patch to do that.

A problem I think on this would be that we still try to make
another connection for canceling and it would stall for several
minutes per connection on a packet stall, which should be done in
a second on ordinary circumstances. Perhaps we might want here is
async canceling with timeout.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center



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

Re: [HACKERS] PATCH: psql show index with type info




Done.


Ok. The file should be named "v2".


Would you like to be the reviewer?


Dunno. At least I wanted to have a look at it!

My 0.02€:

I think that the improvement provided is worthwhile.

Two questions: Why no documentation update? Why no non-regressions 
tests?


As far as the output is concerned, ISTM that "btree index", "hash index" 
and so would sound nicer than "index: sub-type".


I'm wondering about the sub-query implementation: Maybe an outer join 
could bring the same result with a more relational & elegant query.


The "gettext_noop" call does not seem to make sense: the point is to 
translate the string, and there is no way to translate "index: query>". The result of the query should be translated if this is what is 
wanted, and that can only be by hand:


  CASE amname || ' index'
WHEN 'hash index' THEN '%s' ...
WHEN ...
ELSE amname || ' index' # untranslated...
  END

   gettext_noop('hash index') # get translation for 'hash index'
   ...

--
Fabien.
--
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: psql show index with type info

2017-04-17 Thread Amos Bird


Done. Would you like to be the reviewer? Thanks! diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index 0f9f497..a6dc599 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -3284,7 +3284,7 @@ listTables(const char *tabtypes, const char *pattern, bool verbose, bool showSys
 	  " WHEN " CppAsString2(RELKIND_RELATION) " THEN '%s'"
 	  " WHEN " CppAsString2(RELKIND_VIEW) " THEN '%s'"
 	  " WHEN " CppAsString2(RELKIND_MATVIEW) " THEN '%s'"
-	  " WHEN " CppAsString2(RELKIND_INDEX) " THEN '%s'"
+	  " WHEN " CppAsString2(RELKIND_INDEX) " THEN %s"
 	  " WHEN " CppAsString2(RELKIND_SEQUENCE) " THEN '%s'"
 	  " WHEN 's' THEN '%s'"
 	  " WHEN " CppAsString2(RELKIND_FOREIGN_TABLE) " THEN '%s'"
@@ -3296,7 +3296,7 @@ listTables(const char *tabtypes, const char *pattern, bool verbose, bool showSys
 	  gettext_noop("table"),
 	  gettext_noop("view"),
 	  gettext_noop("materialized view"),
-	  gettext_noop("index"),
+	  gettext_noop("'index: '||(select amname from pg_am a where a.oid = c.relam)"),
 	  gettext_noop("sequence"),
 	  gettext_noop("special"),
 	  gettext_noop("foreign table"),
.

Fabien COELHO  writes:

>> I'm not sure where to add documentations about this patch or if needed one. 
>> Please help
>> me if you think this patch is useful.
>
> This patch does not apply anymore. Are you planning to update it?

-- 
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: psql show index with type info




I'm not sure where to add documentations about this patch or if needed one. 
Please help
me if you think this patch is useful.


This patch does not apply anymore. Are you planning to update it?

--
Fabien.


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

Re: [HACKERS] Logical replication and inheritance

On 2017/04/17 23:08, Peter Eisentraut wrote:
> On 4/16/17 23:00, Amit Langote wrote:
>>> To fix this, pg_dump should emit ADD TABLE ONLY foo.
>>
>> Yeah, that's one way.  Attached is a tiny patch for that.
>>
>> By the way, I noticed that although grammar accepts ONLY and * against a
>> table name to affect whether descendant tables are included, the same is
>> not mentioned in the CREATE PUBLICATION and ALTER PUBLICATION reference
>> pages.  I suspect it was probably not intentional, so attached a doc patch
>> for that too.
> 
> Committed those, with some extra tests.

Thanks.

Regards,
Amit



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

Re: [HACKERS] Other formats in pset like markdown, rst, mediawiki



Hello Jan,


Corrected problem with \pset linestyle when format is set to markdown, or
rst.

Corrected tuples only for markdown and rst (normal and expanded)


It seems that the patch does not apply anymore on head due to changes in 
psql non regression tests. Could you rebase?


--
Fabien.


--
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] PANIC in pg_commit_ts slru after crashes

2017-04-17 Thread Michael Paquier

On Tue, Apr 18, 2017 at 12:33 AM, Jeff Janes  wrote:
> On Sun, Apr 16, 2017 at 6:59 PM, Michael Paquier 
> wrote:
>>
>>
>>
>> Jeff, does this patch make the situation better? The fix is rather
>> simple as it just makes sure that the next XID never gets updated if
>> there are no 2PC files.
>
>
> Yes, that fixes the reported case when 2PC are not being used.

Thanks for the confirmation. I am able to run your test suite by the
way after a couple of tweaks, and I can see the failures. Still
investigating the 2nd report...
-- 
Michael
VMware vCenter Server
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] Re: Query fails when SRFs are part of FROM clause (Commit id: 69f4b9c85f)

I wrote:
> I'm a bit inclined to agree with the idea of explicitly requiring SRFs
> in FROM to appear only at the top level of the expression.

If we are going to go down this road, I think it would be a good idea
to try to provide a cursor position for the "can't accept a set" error
message, because otherwise it will be really unclear what's wrong with
something like

SELECT * FROM ROWS FROM (generate_series(1,10), abs(generate_series(1,10)));

I looked into what it would take to do that, and was pleasantly surprised
to find out that most of the infrastructure is already there since
commit 4c728f38.  Basically all we have to do is the attached.

Now that this infrastructure exists, anything that has access to a
PlanState or ExprContext, plus a parse node containing a location, is able
to report an error cursor.  It took a considerable effort of will not to
start plastering error position reports on a lot of other executor errors.
I think we should do that, but it smells new-feature-y, and hence
something to tackle for v11 not now.  But if v10 is moving the goalposts
on where you can put an SRF call, I think we should do this much in v10.

Naming note: a name like ExecErrPosition() would have been more consistent
with the other stuff that execUtils.c exports.  But since this is meant
to be used in ereport() calls, whose support functions generally aren't
camel-cased, I thought executor_errposition() was a better choice.
I'm not particularly set on that though.

regards, tom lane

diff --git a/src/backend/executor/execExpr.c b/src/backend/executor/execExpr.c
index 15d693f..5a34a46 100644
*** a/src/backend/executor/execExpr.c
--- b/src/backend/executor/execExpr.c
*** ExecInitFunc(ExprEvalStep *scratch, Expr
*** 2103,2109 
  	if (flinfo->fn_retset)
  		ereport(ERROR,
  (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
!  errmsg("set-valued function called in context that cannot accept a set")));

  	/* Build code to evaluate arguments directly into the fcinfo struct */
  	argno = 0;
--- 2103,2111 
  	if (flinfo->fn_retset)
  		ereport(ERROR,
  (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
!  errmsg("set-valued function called in context that cannot accept a set"),
!  parent ? executor_errposition(parent->state,
! 		  exprLocation((Node *) node)) : 0));

  	/* Build code to evaluate arguments directly into the fcinfo struct */
  	argno = 0;
diff --git a/src/backend/executor/execSRF.c b/src/backend/executor/execSRF.c
index 4badd5c..077ac20 100644
*** a/src/backend/executor/execSRF.c
--- b/src/backend/executor/execSRF.c
***
*** 34,40 

  /* static function decls */
! static void init_sexpr(Oid foid, Oid input_collation, SetExprState *sexpr,
  		   MemoryContext sexprCxt, bool allowSRF, bool needDescForSRF);
  static void ShutdownSetExpr(Datum arg);
  static void ExecEvalFuncArgs(FunctionCallInfo fcinfo,
--- 34,41 

  /* static function decls */
! static void init_sexpr(Oid foid, Oid input_collation, Expr *node,
! 		   SetExprState *sexpr, PlanState *parent,
  		   MemoryContext sexprCxt, bool allowSRF, bool needDescForSRF);
  static void ShutdownSetExpr(Datum arg);
  static void ExecEvalFuncArgs(FunctionCallInfo fcinfo,
*** ExecInitTableFunctionResult(Expr *expr,
*** 77,83 
  		state->funcReturnsSet = func->funcretset;
  		state->args = ExecInitExprList(func->args, parent);

! 		init_sexpr(func->funcid, func->inputcollid, state,
     econtext->ecxt_per_query_memory, func->funcretset, false);
  	}
  	else
--- 78,84 
  		state->funcReturnsSet = func->funcretset;
  		state->args = ExecInitExprList(func->args, parent);

! 		init_sexpr(func->funcid, func->inputcollid, expr, state, parent,
     econtext->ecxt_per_query_memory, func->funcretset, false);
  	}
  	else
*** ExecInitFunctionResultSet(Expr *expr,
*** 438,444 
  		FuncExpr   *func = (FuncExpr *) expr;

  		state->args = ExecInitExprList(func->args, parent);
! 		init_sexpr(func->funcid, func->inputcollid, state,
     econtext->ecxt_per_query_memory, true, true);
  	}
  	else if (IsA(expr, OpExpr))
--- 439,445 
  		FuncExpr   *func = (FuncExpr *) expr;

  		state->args = ExecInitExprList(func->args, parent);
! 		init_sexpr(func->funcid, func->inputcollid, expr, state, parent,
     econtext->ecxt_per_query_memory, true, true);
  	}
  	else if (IsA(expr, OpExpr))
*** ExecInitFunctionResultSet(Expr *expr,
*** 446,452 
  		OpExpr	   *op = (OpExpr *) expr;

  		state->args = ExecInitExprList(op->args, parent);
! 		init_sexpr(op->opfuncid, op->inputcollid, state,
     econtext->ecxt_per_query_memory, true, true);
  	}
  	else
--- 447,453 
  		OpExpr	   *op = (OpExpr *) expr;

  		state->args = ExecInitExprList(op->args, parent);
! 		init_sexpr(op->opfuncid, op->inputcollid, expr, state, parent,
     econtext->ecxt_per_query_memory, true, true);
  	}
  	else
*** restart:

Re: [HACKERS] extended stats not friendly towards ANALYZE with subset of columns

David Rowley wrote:
> On 18 April 2017 at 05:12, Alvaro Herrera  wrote:
> > Pushed, after tweaking so that a WARNING is still emitted, because it's
> > likely to be useful in pointing out a procedural mistake while extended
> > stats are being tested.
> 
> Thanks for pushing.
> 
> Seems you maintained most of my original patch and added to it a bit,
> but credits don't seem to reflect that. It's not the end of the world
> but just wanted to note that.

Yeah, sorry about that.  Maybe we should start mentioning the percentage
of the patch that comes from each author (just kidding).

-- 
Á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] extended stats not friendly towards ANALYZE with subset of columns

2017-04-17 Thread David Rowley

On 18 April 2017 at 05:12, Alvaro Herrera  wrote:
> Pushed, after tweaking so that a WARNING is still emitted, because it's
> likely to be useful in pointing out a procedural mistake while extended
> stats are being tested.

Thanks for pushing.

Seems you maintained most of my original patch and added to it a bit,
but credits don't seem to reflect that. It's not the end of the world
but just wanted to note that.


-- 
 David Rowley   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] Allowing extended stats on foreign and partitioned tables

2017-04-17 Thread David Rowley

On 18 April 2017 at 09:01, Alvaro Herrera  wrote:
> David Rowley wrote:
>> While reviewing extended stats I noticed that it was possible to
>> create extended stats on many object types, including sequences. I
>> mentioned that this should be disallowed. Statistics were then changed
>> to be only allowed on plain tables and materialized views.
>>
>> This should be relaxed again to allow anything ANALYZE is allowed on.
>>
>> The attached does this.
>
> The error message was inconsistent in the case of indexes, because of
> using heap_open instead of relation_open.  That seemed gratuitous, so I
> flipped it, added test for the whole thing and pushed.

Thanks for changing that and pushing this.

-- 
 David Rowley   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] pg_statistic_ext.staenabled might not be the best column name

Alvaro Herrera wrote:
> Tom Lane wrote:
> > Peter Eisentraut  writes:
> > > On 4/13/17 08:37, Heikki Linnakangas wrote:
> > >> We have a bunch of > 3-character prefixes already: amop*, amproc*, 
> > >> enum*, cast*. But I think I nevertheless like "ste" better.
> > 
> > > "stx" perhaps?
> > 
> > > I would also be in favor of changing it to something other than "sta".
> > 
> > "stx" sounds pretty good to me --- it seems like e.g. "stxkind" is
> > more visibly distinct from "stakind" than "stekind" would be.
> > 
> > Any objections out there?
> 
> stx sounds good to me too.  I'll see about a patch this afternoon.

Took me a bit longer than I had hoped, but it's done now.

-- 
Á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] Allowing extended stats on foreign and partitioned tables

David Rowley wrote:
> While reviewing extended stats I noticed that it was possible to
> create extended stats on many object types, including sequences. I
> mentioned that this should be disallowed. Statistics were then changed
> to be only allowed on plain tables and materialized views.
> 
> This should be relaxed again to allow anything ANALYZE is allowed on.
> 
> The attached does this.

The error message was inconsistent in the case of indexes, because of
using heap_open instead of relation_open.  That seemed gratuitous, so I
flipped it, added test for the whole thing and pushed.

Thanks for reporting.

-- 
Á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] Self-signed certificate instructions

2017-04-17 Thread Andreas Karlsson


On 04/15/2017 03:58 PM, Andrew Dunstan wrote:

The instructions on how to create a self-signed certificate in s 18.9.3
of the docs seem unduly cumbersome.


+1, I see no reason for us to spread unnecessarily complicated instructions.

Andreas


--
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] Self-signed certificate instructions

2017-04-17 Thread Bruce Momjian

On Mon, Apr 17, 2017 at 04:27:30PM -0400, Andrew Dunstan wrote:
> > I would like to revisit these instructions, as well as document how to
> > create intermediate certificates.  I have scripts that do that.
> >
> 
> 
> OK.. Do you want to run with this?

Please go forward and I will work on the intermediate certificate issue
in a few months.

-- 
  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] Self-signed certificate instructions

2017-04-17 Thread Andrew Dunstan



On 04/17/2017 02:19 PM, Bruce Momjian wrote:
> On Sat, Apr 15, 2017 at 11:17:14AM -0400, Andrew Dunstan wrote:
>>
>> On 04/15/2017 09:58 AM, Andrew Dunstan wrote:
>>> The instructions on how to create a self-signed certificate in s 18.9.3
>>> of the docs seem unduly cumbersome. AFAICT we could replace all the
>>> commands (except the chmod) with something like this:
>>>
>>> |openssl req -new-x509 -days 365-nodes \ -text -outserver.crt\
>>> -keyout server.key\ -subj "/C=XY/CN=yourdomain.name"|
>>>
>>> Is there any reason for sticking with the current instructions?
>>>
>> Argh. Darn Thunderbird. This should of course be:
>>
>>
>> openssl req -new-x509 -days 365-nodes \
>   ^
>
> I think you meant "-days 365 -nodes" here.


yes.


>
> I think the reason we have those cumbersome instructions is that there
> is no way to create a non-expireable certificate using simpler
> instructions.


You can make it for a very large number of days.  should be plenty :-)

TBH very long lived keys are a bad idea. In fact, self-signed
certificates in any production or publicly visible instance are also a
bad idea.


>
> I would like to revisit these instructions, as well as document how to
> create intermediate certificates.  I have scripts that do that.
>


OK.. Do you want to run with this?

cheers

andrew


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



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

[HACKERS] Passing values to a dynamic background worker

2017-04-17 Thread Keith Fiske

So after reading a recent thread on the steep learning curve for PG
internals [1], I figured I'd share where I've gotten stuck with this in a
new thread vs hijacking that one.

One of the goals I had with pg_partman was to see if I could get the
partitioning python scripts redone as C functions using a dynamic
background worker to be able to commit in batches with a single call. My
thinking was to have a user-function that can accept arguments for things
like the interval value, batch size, and other arguments to the python
script, then start/stop a dynamic bgw up for each batch so it can commit
after each one. The dymanic bgw would essentially just have to call the
already existing partition_data() plpgsql function, but I have to be able
to pass the argument values that the user gave down into the dynamic bgw.

I've reached a roadblock in that bgw_main_arg can only accept a single
argument that must be passed by value for a dynamic bgw. I already worked
around this for passing the database name to the my existing use of a bgw
with doing partition maintenance (pass a simple integer to use as an index
array value). But I'm not sure how to do this for passing multiple values
in. I'm assuming this would be the place where I'd see about storing values
in shared memory to be able to re-use later? I'm not even sure if that's
the right approach, and if it is, where to even start to understand how to
do that. Let alone in the context of how that would interact with the
background worker system. If you look at my existing C code, you can see
it's very simple and doesn't do much more than the worker_spi example. I've
yet to have to interact with any memory contexts or such things, and as the
referenced thread below mentions, doing so is quite a steep learning curve.

Any guidance for a newer internals dev here would be great.

1.
https://www.postgresql.org/message-id/CAH%3Dt1kqwCBF7J1bP0RjgsTcp-SaJaHrF4Yhb1iiQZMe3W-FX2w%40mail.gmail.com

--
Keith Fiske
Database Administrator
OmniTI Computer Consulting, Inc.
http://www.keithf4.com

Re: [HACKERS] Self-signed certificate instructions

2017-04-17 Thread Bruce Momjian

On Mon, Apr 17, 2017 at 03:43:09PM -0400, Tom Lane wrote:
> Bruce Momjian  writes:
> > I think the reason we have those cumbersome instructions is that there
> > is no way to create a non-expireable certificate using simpler
> > instructions.
> 
> Um ... but the current instructions don't address that either.

Uh, I thought the instructions were needed for non-expiration, but I now
remember it was to allow for non-password keys, but now I see it is not
needed, so +1 for making the simplification.

> > I would like to revisit these instructions, as well as document how to
> > create intermediate certificates.  I have scripts that do that.
> 
> I don't think we should try to teach people how to use openssl.
> A quick example of setting up a dummy certificate for testing is fine,
> but going much beyond that is not our turf.

We had an open item for years about people complaining that the client
required the entire chain to the root (and our documention currently
mentions that requirement), but it turns out this is only necessary if
you don't create the intermediate certificates with the proper
certificate flag, e.g. -extensions v3_ca.  I will generate a patch that
at least mentions that requirement.

-- 
  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] SUBSCRIPTIONS and pg_upgrade

Peter,

* Peter Eisentraut (peter.eisentr...@2ndquadrant.com) wrote:
> On 4/13/17 12:11, Robert Haas wrote:
> > I wonder if we should have an --no-subscriptions option, now that they
> > are dumped by default, just like we have --no-blobs, --no-owner,
> > --no-password, --no-privileges, --no-acl, --no-tablespaces, and
> > --no-security-labels.  It seems like there is probably a fairly large
> > use case for excluding subscriptions even if you have sufficient
> > permissions to dump them.
> 
> What purpose would that serve in practice?  The subscriptions are now
> dumped disabled, so if they hang around, there is not much impact.

Will they be allowed to be created as a non-superuser when loading into
another cluster?  Even if you might have superuser rights on one
cluster, you may not on another.

> Perhaps an option that also omits publications would make sense?

Yes.

> Or a general filtering facility based on the object tag?

Perhaps..  I'd hate for it to end up being overly complicated though.
The general '--no-security-lables', '--no-tablespaces' and other options
are pretty straight-forward and I think that's a good thing.

Thanks!

Stephen

signature.asc
Description: Digital signature

Re: [HACKERS] Row Level Security UPDATE Confusion

Rod,

* Rod Taylor (rod.tay...@gmail.com) wrote:
> Yep. It's equivalent to a DELETE or DEACTIVATE. RLS may not be the right
> facility but it was very close to working exactly the way I wanted in FOR
> ALL mode.

Turning an UPDATE into, effectively, a DELETE, does seem like it's
beyond the mandate of RLS.  Using an on-delete trigger strikes me as a
good approach.

The base premise of not allowing rows to be 'given away', similar to how
we don't allow full objects to be given away, should be enforced for the
'ALL' policy case, just as it is for the individual-command case.  I'll
get that addressed before the next set of minor releases and will also
see about improving the documentation and code comments to make it more
clear.

Thanks!

Stephen

signature.asc
Description: Digital signature

Re: [HACKERS] Self-signed certificate instructions

Bruce Momjian  writes:
> I think the reason we have those cumbersome instructions is that there
> is no way to create a non-expireable certificate using simpler
> instructions.

Um ... but the current instructions don't address that either.

> I would like to revisit these instructions, as well as document how to
> create intermediate certificates.  I have scripts that do that.

I don't think we should try to teach people how to use openssl.
A quick example of setting up a dummy certificate for testing is fine,
but going much beyond that is not our turf.

regards, tom lane

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

Re: [HACKERS] pg_dump emits ALTER TABLE ONLY partitioned_table

Noah, all,

* Noah Misch (n...@leadboat.com) wrote:
> On Thu, Apr 13, 2017 at 11:38:08AM -0400, Robert Haas wrote:
> > On Thu, Apr 13, 2017 at 11:05 AM, Stephen Frost  wrote:
> > > Sure, though I won't be able to today and I've got some doubts about the
> > > other patches.  I'll have more time tomorrow though.
> > 
> > OK, cool.  I'll mark you down as the owner on the open items list.
> 
> [Action required within three days.]

I've put up a new patch for review on the thread and plan to commit
that tomorrow, assuming there isn't anything further.  That should
resolve the immediate issue, but I do think there's some merit to Amit's
follow-on patches and will continue to discuss those and may commit one
or both of those later this week.

Thanks!

Stephen

signature.asc
Description: Digital signature

Re: [HACKERS] Possible problem in Custom Scan API

Dmitry Ivanov  writes:
> Tom Lane wrote:
>> I'm coming around to the idea that it'd be better to disable physical
>> tlists for custom scans.
>> However, I'm hesitant to make such a change in the back branches; if
>> there's anyone using custom scans who's negatively affected, they'd be
>> rightfully unhappy.  Are you satisfied if we change this in HEAD?

After thinking about this over the weekend, I've come to the conclusion
that back-patching is probably the right thing anyway.  The upside of the
use_physical_tlist optimization is pretty marginal even when it applies,
while the downsides of applying it inappropriately can be very large,
as we've discussed.

> I've been thinking about this all along, and it seems that this is a decent 
> decision. However, I've made a tiny bugfix patch which allows CustomScans 
> to notify the core code that they can handle physical targetlists. 

I'm unimpressed by this part --- we couldn't back-patch such a change, and
I think it's unnecessary anyway in 9.6+ because the scan provider could
generate a nondefault pathtarget if it wants this to happen.

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] Logical replication - TRAP: FailedAssertion in pgstat.c

2017-04-17 Thread Erik Rijkers

On 2017-04-17 15:59, Stas Kelvich wrote:

On 17 Apr 2017, at 10:30, Erik Rijkers  wrote:

On 2017-04-16 20:41, Andres Freund wrote:

On 2017-04-16 10:46:21 +0200, Erik Rijkers wrote:

On 2017-04-15 04:47, Erik Rijkers wrote:
>
> 0001-Reserve-global-xmin-for-create-slot-snasphot-export.patch +
> 0002-Don-t-use-on-disk-snapshots-for-snapshot-export-in-l.patch+
> 0003-Prevent-snapshot-builder-xmin-from-going-backwards.patch  +
> 0004-Fix-xl_running_xacts-usage-in-snapshot-builder.patch  +
> 0005-Skip-unnecessary-snapshot-builds.patch
I am now using these newer patches:
https://www.postgresql.org/message-id/30242bc6-eca4-b7bb-670e-8d0458753a8c%402ndquadrant.com
> It builds fine, but when I run the old pbench-over-logical-replication
> test I get:
>
> TRAP: FailedAssertion("!(entry->trans == ((void *)0))", File:
> "pgstat.c", Line: 828)
To get that error:

I presume this is the fault of
http://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=139eb9673cb84c76f493af7e68301ae204199746
if you git revert that individual commit, do things work again?

Yes, compiled from 67c2def11d4 with the above 4 patches, it runs 
flawlessly again. (flawlessly= a few hours without any error)

I’ve reproduced failure, this happens under tablesync worker and 
putting

pgstat_report_stat() under the previous condition block should help.

However for me it took about an hour of running this script to catch
original assert.

Can you check with that patch applied?

Your patch on top of the 5 patches above seem to solve the matter too: 
no problems after running for 2 hours (previously it failed within half 
a minute).

Erik Rijkers

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

Re: [HACKERS] logical replication and PANIC during shutdown checkpoint in publisher

On 4/17/17 12:30, Andres Freund wrote:
 So I guess that CREATE_REPLICATION_SLOT code calls LogStandbySnapshot()
 and which generates WAL record about snapshot of running transactions.
>>>
>>> Erroring out in these cases sounds easy enough.  Wonder if there's not a
>>> bigger problem with WAL records generated e.g. by HOT pruning or such,
>>> during decoding.  Not super likely, but would probably hit exactly the
>>> same, no?
>>
>> Sounds possible, yes. Sounds like that's going to be nontrivial to fix
>> though.
>>
>> Another problem is that queries can run on walsender now. But that
>> should be possible to detect and shutdown just like backend.
> 
> This sounds like a case for s/PANIC/ERROR|FATAL/ to me...

I'd imagine the postmaster would tell the walsender that it has started
shutdown, and then the walsender would reject $certain_things.  But I
don't see an existing way for the walsender to know that shutdown has
been initiated.  SIGINT is still free ...

The alternative of shutting down logical walsenders earlier also doesn't
look straightforward, since the postmaster doesn't know directly what
kind of walsender a certain process is.  So you'd also need additional
signal types or something there.

-- 
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] Self-signed certificate instructions

2017-04-17 Thread Bruce Momjian

On Sat, Apr 15, 2017 at 11:17:14AM -0400, Andrew Dunstan wrote:
> 
> 
> On 04/15/2017 09:58 AM, Andrew Dunstan wrote:
> > The instructions on how to create a self-signed certificate in s 18.9.3
> > of the docs seem unduly cumbersome. AFAICT we could replace all the
> > commands (except the chmod) with something like this:
> >
> > |openssl req -new-x509 -days 365-nodes \ -text -outserver.crt\
> > -keyout server.key\ -subj "/C=XY/CN=yourdomain.name"|
> >
> > Is there any reason for sticking with the current instructions?
> >
> 
> Argh. Darn Thunderbird. This should of course be:
> 
> 
> openssl req -new-x509 -days 365-nodes \
  ^

I think you meant "-days 365 -nodes" here.

I think the reason we have those cumbersome instructions is that there
is no way to create a non-expireable certificate using simpler
instructions.

I would like to revisit these instructions, as well as document how to
create intermediate certificates.  I have scripts that do that.

-- 
  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] tablesync patch broke the assumption that logical rep depends on?

On 4/16/17 22:01, 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

I think we're not really sure yet what to do about this.  Discussion is
ongoing.  I'll report back on Wednesday.

-- 
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] Quorum commit for multiple synchronous replication.

2017-04-17 Thread Fujii Masao

On Wed, Apr 12, 2017 at 2:36 AM, Masahiko Sawada  wrote:
> On Thu, Apr 6, 2017 at 4:17 PM, Masahiko Sawada  wrote:
>> On Thu, Apr 6, 2017 at 10:51 AM, Noah Misch  wrote:
>>> On Thu, Apr 06, 2017 at 12:48:56AM +0900, Fujii Masao wrote:
 On Wed, Apr 5, 2017 at 3:45 PM, Noah Misch  wrote:
 > On Mon, Dec 19, 2016 at 09:49:58PM +0900, Fujii Masao wrote:
 >> Regarding this feature, there are some loose ends. We should work on
 >> and complete them until the release.
 >>
 >> (1)
 >> Which synchronous replication method, priority or quorum, should be
 >> chosen when neither FIRST nor ANY is specified in s_s_names? Right now,
 >> a priority-based sync replication is chosen for keeping backward
 >> compatibility. However some hackers argued to change this decision
 >> so that a quorum commit is chosen because they think that most users
 >> prefer to a quorum.
 >>
 >> (2)
 >> There will be still many source comments and documentations that
 >> we need to update, for example, in high-availability.sgml. We need to
 >> check and update them throughly.
 >>
 >> (3)
 >> The priority value is assigned to each standby listed in s_s_names
 >> even in quorum commit though those priority values are not used at all.
 >> Users can see those priority values in pg_stat_replication.
 >> Isn't this confusing? If yes, it might be better to always assign 1 as
 >> the priority, for example.
 >
 > [Action required within three days.  This is a generic notification.]
 >
 > The above-described topic is currently a PostgreSQL 10 open item.  Fujii,
 > 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

 Thanks for the notice!

 Regarding the item (2), Sawada-san told me that he will work on it after
 this CommitFest finishes. So we would receive the patch for the item from
 him next week. If there will be no patch even after the end of next week
 (i.e., April 14th), I will. Let's wait for Sawada-san's action at first.
>>>
>>> Sounds reasonable; I will look for your update on 14Apr or earlier.
>>>
 The items (1) and (3) are not bugs. So I don't think that they need to be
 resolved before the beta release. After the feature freeze, many users
 will try and play with many new features including quorum-based syncrep.
 Then if many of them complain about (1) and (3), we can change the code
 at that timing. So we need more time that users can try the feature.
>>>
>>> I've moved (1) to a new section for things to revisit during beta.  If 
>>> someone
>>> feels strongly that the current behavior is Wrong and must change, speak up 
>>> as
>>> soon as you reach that conclusion.  Absent such arguments, the behavior 
>>> won't
>>> change.
>>>
 BTW, IMO (3) should be fixed so that pg_stat_replication reports NULL
 as the priority if quorum-based sync rep is chosen. It's less confusing.
>>>
>>> Since you do want (3) to change, please own it like any other open item,
>>> including the mandatory status updates.
>>
>> I agree to report NULL as the priority. I'll send a patch for this as well.
>>
>> Regards,
>>
>
> Attached two draft patches. The one makes pg_stat_replication.sync
> priority report NULL if in quorum-based sync replication. To prevent
> extra change I don't change so far the code of setting standby
> priority. The another one improves the comment and documentation. If
> there is more thing what we need to mention in documentation please
> give me feedback.

Attached is the modified version of the doc improvement patch.
Barring any objection, I will commit this version.

+In term of performance there is difference between two synchronous
+replication method. Generally quorum-based synchronous replication
+tends to be higher performance than priority-based synchronous
+replication. Because in quorum-based synchronous replication, the
+transaction can resume as soon as received the specified number of
+acknowledgement from synchronous standby servers without distinction
+of standby servers. On the other hand in priority-based synchronous
+replication, the standby server that the primary server must wait for
+is f

Re: [HACKERS] On How To Shorten the Steep Learning Curve Towards PG Hacking...

Craig Ringer wrote:

> Personally I have to agree that the learning curve is very steep. Some
> of the docs and presentations help, but there's a LOT to understand.

There is a wiki page "Developer_FAQ" which is supposed to help answer
these questions.  It is currently not very useful, because people
stopped adding to it very early and is now mostly unmaintained, but
I'm sure it could become a very useful central resource for this kind of
information.

-- 
Á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] Cutting initdb's runtime (Perl question embedded)

Andres Freund  writes:
> Btw, I think Tom's "more that could be done" was referring more to doing
> more upfront work, checking, easier input format, whatnot in the genbki,
> not so much performance work...  Tom, correct me if I'm wrong.

Yeah, as per what we were just saying, performance of that code isn't
really an issue right now.  Supporting a nicer input format is probably
the biggest step forward that could be taken, but that will require some
major work :-(.  We've also talked about things like supporting
regprocedure rather than only regproc (ie disambiguating overloaded
functions), likewise regoperator, and so on, with an eye to reducing
the number of places where people have to write numeric OIDs in the
input.  There's some threads on this in the archives, but I'm too
lazy to look.

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] Cutting initdb's runtime (Perl question embedded)

On 2017-04-17 17:49:54 +0100, Dagfinn Ilmari Mannsåker wrote:
> Tom Lane  writes:
> 
> > There's certainly lots more that could be done in the genbki code,
> > but I think all we can justify at this stage of the development
> > cycle is to get the low-hanging fruit for testing speedups.
> 
> I threw Devel::NYTProf at it and picked some more low-hanging fruit.
> Attached are separate patches for each change, and here are the runtimes
> of genbki.pl and Gen_fmgrtab.pl, respectively, after each patch
> (averages of 5 runs, in millseconds):

Btw, I think Tom's "more that could be done" was referring more to doing
more upfront work, checking, easier input format, whatnot in the genbki,
not so much performance work...  Tom, correct me if I'm wrong.

- 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] extended stats not friendly towards ANALYZE with subset of columns

David Rowley wrote:

> ERROR:  extended statistics could not be collected for column "a" of
> relation public.ab1
> HINT:  Consider ALTER TABLE "public"."ab1" ALTER "a" SET STATISTICS -1
> 
> I don't think the error is useful here, as it means nothing gets done.
> Probably better to just not (re)build those stats.

Agreed, an error there is a bad idea.

> Another option would be to check for extended stats before deciding
> which rows to ANALYZE, then still gathering the columns required for
> MV stats, but I think if the user asks for a subset of columns to be
> analyzed, and that causes a column to be missing for an extended
> statistics, that it would be pretty surprising if we rebuild the
> extended stats.

That would be very surprising indeed.

> Perhaps the SET STATISTIC 0 for a column still needs to gather data
> for extended statistics, though. Perhaps a debate should ensue about
> how that should work exactly.

Hmm.  I'd rather throw a warning at either CREATE STATISTICS time or at
ALTER TABLE SET STATISTICS time, rather than magically changing the set
of columns at analyze time.

> I've attached a patch which fixes the problem above, but it does
> nothing to change the analyze behaviour for 0 statistics columns.

Pushed, after tweaking so that a WARNING is still emitted, because it's
likely to be useful in pointing out a procedural mistake while extended
stats are being tested.

-- 
Á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] Cutting initdb's runtime (Perl question embedded)

Andres Freund  writes:
> On 2017-04-17 17:49:54 +0100, Dagfinn Ilmari Mannsåker wrote:
>> I threw Devel::NYTProf at it and picked some more low-hanging fruit.

> I'm a bit doubtful about improving the performance of genbki at the cost
> of any sort of complication - it's only executed during the actual
> build, not during initdb...  I don't see much point in doing things like
> 3) and 4), it's just not worth it imo.

Yeah, it's only run once per build, or probably even less often than that
considering we only re-run it when the input files change.  I could get
interested in another 20% reduction in initdb's time, but not genbki's.

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] Cutting initdb's runtime (Perl question embedded)

On 2017-04-17 17:49:54 +0100, Dagfinn Ilmari Mannsåker wrote:
> Tom Lane  writes:
> 
> > There's certainly lots more that could be done in the genbki code,
> > but I think all we can justify at this stage of the development
> > cycle is to get the low-hanging fruit for testing speedups.
> 
> I threw Devel::NYTProf at it and picked some more low-hanging fruit.
> Attached are separate patches for each change, and here are the runtimes
> of genbki.pl and Gen_fmgrtab.pl, respectively, after each patch
> (averages of 5 runs, in millseconds):
> 
> master (b6dd1271): 355, 182
> 
> 1: Avoid unnecessary regex captures: 349, 183
> 2: Avoid repeated calls to SplitDataLine: 316, 158
> 3: Inline SplitDataLine: 291, 141
> 4: Inline check_natts: 287, 141
> 
> Together they shave 68ms or 19.2% off the runtime of genbki.pl and 41ms
> or 22.5% off the runtime of Gen_fmgrtab.pl

I'm a bit doubtful about improving the performance of genbki at the cost
of any sort of complication - it's only executed during the actual
build, not during initdb...  I don't see much point in doing things like
3) and 4), it's just not worth it imo.

- 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] Cutting initdb's runtime (Perl question embedded)

2017-04-17 Thread Dagfinn Ilmari Mannsåker

Tom Lane  writes:

> There's certainly lots more that could be done in the genbki code,
> but I think all we can justify at this stage of the development
> cycle is to get the low-hanging fruit for testing speedups.

I threw Devel::NYTProf at it and picked some more low-hanging fruit.
Attached are separate patches for each change, and here are the runtimes
of genbki.pl and Gen_fmgrtab.pl, respectively, after each patch
(averages of 5 runs, in millseconds):

master (b6dd1271): 355, 182

1: Avoid unnecessary regex captures: 349, 183
2: Avoid repeated calls to SplitDataLine: 316, 158
3: Inline SplitDataLine: 291, 141
4: Inline check_natts: 287, 141

Together they shave 68ms or 19.2% off the runtime of genbki.pl and 41ms
or 22.5% off the runtime of Gen_fmgrtab.pl

Finally, one non-performance patch, which just removes the use of
Exporter in Catalog.pm, since none of the users actually import anything
from it.

- ilmari
-- 
"I use RMS as a guide in the same way that a boat captain would use
 a lighthouse.  It's good to know where it is, but you generally
 don't want to find yourself in the same spot." - Tollef Fog Heen

>From 5d7f4b1e78243daa6939501b88b2644bfcf9bd77 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= 
Date: Mon, 17 Apr 2017 13:26:35 +0100
Subject: [PATCH 1/8] Avoid unnecessary regex captures in Catalog.pm

---
 src/backend/catalog/Catalog.pm | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/backend/catalog/Catalog.pm b/src/backend/catalog/Catalog.pm
index fa8de04..e7b647a 100644
--- a/src/backend/catalog/Catalog.pm
+++ b/src/backend/catalog/Catalog.pm
@@ -54,7 +54,7 @@ sub Catalogs
 		{
 
 			# Strip C-style comments.
-			s;/\*(.|\n)*\*/;;g;
+			s;/\*(?:.|\n)*\*/;;g;
 			if (m;/\*;)
 			{
 
@@ -80,12 +80,12 @@ sub Catalogs
 			{
 $catalog{natts} = $1;
 			}
-			elsif (/^DATA\(insert(\s+OID\s+=\s+(\d+))?\s+\(\s*(.*)\s*\)\s*\)$/)
+			elsif (/^DATA\(insert(?:\s+OID\s+=\s+(\d+))?\s+\(\s*(.*)\s*\)\s*\)$/)
 			{
-check_natts($filename, $catalog{natts}, $3,
+check_natts($filename, $catalog{natts}, $2,
 			$input_file, $input_line_number);
 
-push @{ $catalog{data} }, { oid => $2, bki_values => $3 };
+push @{ $catalog{data} }, { oid => $1, bki_values => $2 };
 			}
 			elsif (/^DESCR\(\"(.*)\"\)$/)
 			{
-- 
2.7.4

>From 92402e0306eda209b1a2811acc41325d75add0cb Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= 
Date: Mon, 17 Apr 2017 14:04:20 +0100
Subject: [PATCH 2/8] Avoid repeated calls to Catalog::SplitDataLine

Both check_natts and the callers of Catalog::Catalogs were calling
Catalog::SplitDataLines, the former discarding the result.

Instead, have Catalog::Catalogs store the split fields directly and pass
the count to check_natts
---
 src/backend/catalog/Catalog.pm   | 14 +++---
 src/backend/catalog/genbki.pl|  3 +--
 src/backend/utils/Gen_fmgrtab.pl |  3 +--
 3 files changed, 9 insertions(+), 11 deletions(-)

diff --git a/src/backend/catalog/Catalog.pm b/src/backend/catalog/Catalog.pm
index e7b647a..0c508b0 100644
--- a/src/backend/catalog/Catalog.pm
+++ b/src/backend/catalog/Catalog.pm
@@ -82,10 +82,12 @@ sub Catalogs
 			}
 			elsif (/^DATA\(insert(?:\s+OID\s+=\s+(\d+))?\s+\(\s*(.*)\s*\)\s*\)$/)
 			{
-check_natts($filename, $catalog{natts}, $2,
+my @bki_values = SplitDataLine($2);
+
+check_natts($filename, $catalog{natts}, scalar(@bki_values),
 			$input_file, $input_line_number);
 
-push @{ $catalog{data} }, { oid => $1, bki_values => $2 };
+push @{ $catalog{data} }, { oid => $1, bki_values => \@bki_values };
 			}
 			elsif (/^DESCR\(\"(.*)\"\)$/)
 			{
@@ -254,17 +256,15 @@ sub RenameTempFile
 # verify the number of fields in the passed-in DATA line
 sub check_natts
 {
-	my ($catname, $natts, $bki_val, $file, $line) = @_;
+	my ($catname, $natts, $bki_vals, $file, $line) = @_;
 
 	die "Could not find definition for Natts_${catname} before start of DATA() in $file\n"
 		unless defined $natts;
 
-	my $nfields = scalar(SplitDataLine($bki_val));
-
 	die sprintf
 		"Wrong number of attributes in DATA() entry at %s:%d (expected %d but got %d)\n",
-		$file, $line, $natts, $nfields
-	  unless $natts == $nfields;
+		$file, $line, $natts, $bki_vals
+	unless $natts == $bki_vals;
 }
 
 1;
diff --git a/src/backend/catalog/genbki.pl b/src/backend/catalog/genbki.pl
index 198e072..8875f6c 100644
--- a/src/backend/catalog/genbki.pl
+++ b/src/backend/catalog/genbki.pl
@@ -161,9 +161,8 @@ foreach my $catname (@{ $catalogs->{names} })
 		foreach my $row (@{ $catalog->{data} })
 		{
 
-			# Split line into tokens without interpreting their meaning.
 			my %bki_values;
-			@bki_values{@attnames} = Catalog::SplitDataLine($row->{bki_values});
+			@bki_values{@attnames} = @{$row->{bki_values}};
 
 			# Perform required substitutions on fields
 			foreach my $att (keys %bki_values)
diff --git a/src/backend/utils/Gen_fmgrtab.pl b/src/backend/utils/Gen_fmgrtab.pl
index 76bdf5c.

Re: [HACKERS] pg_dump emits ALTER TABLE ONLY partitioned_table

Amit,

* Amit Langote (langote_amit...@lab.ntt.co.jp) wrote:
> OK, I agree.  I tweaked the existing bullet point about differences from
> traditional inheritance when using ONLY with partitioned tables.

Please take a look at the attached and let me know your thoughts on it.
I changed the code to complain again regarding TRUNCATE ONLY, since that
never actually makes sense on a partitioned table, unlike ALTER TABLE
ONLY, and adjusted the error messages and added hints.

> Updated patches attached (0002 and 0003 unchanged).

Regarding these, 0002 changes pg_dump to handle partitions much more
like inheritance, which at least seems like a decent idea but makes me a
bit concerned about making sure we're doing everything correctly.  In
particular, we should really have regression tests for non-inherited
CHECK (and NOT NULL) constraints on partitions.  Perhaps there are tests
covering those cases in the standard regression suite, but it's not
obvious from the SQL.

Thanks!

Stephen
From ecea1d99bb1bce1e1625d4f0157af03274c82e17 Mon Sep 17 00:00:00 2001
From: Stephen Frost 
Date: Mon, 17 Apr 2017 12:06:12 -0400
Subject: [PATCH] Allow ALTER TABLE ONLY on partitioned tables

There is no need to forbid ALTER TABLE ONLY on partitioned tables,
when no partitions exist yet.  This can be handy for users who are
building up their partitioned table independently and will create actual
partitions later.

In addition, this is how pg_dump likes to operate in certain instances.

Author: Amit Langote, with some error message word-smithing by me
---
 doc/src/sgml/ddl.sgml | 17 +---
 src/backend/commands/tablecmds.c  | 66 +++
 src/test/regress/expected/alter_table.out | 36 +++--
 src/test/regress/expected/truncate.out|  8 
 src/test/regress/sql/alter_table.sql  | 24 ---
 src/test/regress/sql/truncate.sql |  4 ++
 6 files changed, 107 insertions(+), 48 deletions(-)

diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml
index 340c961..958a90a 100644
--- a/doc/src/sgml/ddl.sgml
+++ b/doc/src/sgml/ddl.sgml
@@ -2944,17 +2944,22 @@ VALUES ('Albany', NULL, NULL, 'NY');
Both CHECK and NOT NULL
constraints of a partitioned table are always inherited by all its
partitions.  CHECK constraints that are marked
-   NO INHERIT are not allowed.
+   NO INHERIT are not allowed to be created on
+   partitioned tables.
   
  
 
  
   
-   The ONLY notation used to exclude child tables
-   will cause an error for partitioned tables in the case of
-   schema-modifying commands such as most ALTER TABLE
-   commands.  For example, dropping a column from only the parent does
-   not make sense for partitioned tables.
+   Using ONLY to add or drop a constraint on only the
+   partitioned table is only supported when no partitions exist yet.  Once
+   partitions exist, we do not support using ONLY to
+   add or drop constraints on only the partitioned table.  Instead,
+   constraints can be added or dropped, when they are not present in the
+   parent table, directly on the partitions.  As a partitioned table does
+   not have any data directly, attempts to use TRUNCATE
+   ONLY on a partitioned table will always return an
+   error.
   
  
 
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index a02904c..a357130 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -1259,7 +1259,8 @@ ExecuteTruncate(TruncateStmt *stmt)
 		else if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
 			ereport(ERROR,
 	(errcode(ERRCODE_WRONG_OBJECT_TYPE),
-	 errmsg("must truncate child tables too")));
+	 errmsg("cannot truncate only a partitioned table"),
+	 errhint("Do not specify the ONLY keyword, or use truncate only on the partitions directly.")));
 	}
 
 	/*
@@ -5578,14 +5579,20 @@ static void
 ATPrepDropNotNull(Relation rel, bool recurse, bool recursing)
 {
 	/*
-	 * If the parent is a partitioned table, like check constraints, NOT NULL
-	 * constraints must be dropped from child tables.
+	 * If the parent is a partitioned table, like check constraints, we do
+	 * not support removing the NOT NULL while partitions exist.
 	 */
-	if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE &&
-		!recurse && !recursing)
-		ereport(ERROR,
-(errcode(ERRCODE_INVALID_TABLE_DEFINITION),
- errmsg("constraint must be dropped from child tables too")));
+	if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
+	{
+		PartitionDesc	partdesc = RelationGetPartitionDesc(rel);
+
+		Assert(partdesc != NULL);
+		if (partdesc->nparts > 0 && !recurse && !recursing)
+			ereport(ERROR,
+	(errcode(ERRCODE_INVALID_TABLE_DEFINITION),
+	 errmsg("cannot remove constraint from only the partitioned table when partitions exist"),
+	 errhint("Do not specify the ONLY keyword.")));
+	}
 }

Re: [HACKERS] On How To Shorten the Steep Learning Curve Towards PG Hacking...

2017-04-17 Thread Simon Riggs

On 27 March 2017 at 13:00, Kang Yuzhe  wrote:

> I have found PG source Code reading and hacking to be one the most
> frustrating experiences in my life.  I believe that PG hacking should not be
> a painful journey but an enjoyable one!
>
> It is my strong believe that out of my PG hacking frustrations, there may
> come insights for the PG experts on ways how to devise hands-on with PG
> internals so that new comers will be great coders as quickly as possible.

I'm here now because PostgreSQL has clear, well designed and
maintained code with accurate docs, great comments and a helpful team.

I'd love to see detailed cases where another project is better in a
measurable way; I am willing to learn from that.

Any journey to expertise takes 10,000 hours. There is no way to shorten that.

What aspect of your journey caused you pain?

-- 
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] logical replication and PANIC during shutdown checkpoint in publisher

On 2017-04-17 18:28:16 +0200, Petr Jelinek wrote:
> On 17/04/17 18:02, Andres Freund wrote:
> > On 2017-04-15 02:33:59 +0900, Fujii Masao wrote:
> >> On Fri, Apr 14, 2017 at 10:33 PM, Petr Jelinek
> >>  wrote:
> >>> On 12/04/17 15:55, Fujii Masao wrote:
>  Hi,
> 
>  When I shut down the publisher while I repeated creating and dropping
>  the subscription in the subscriber, the publisher emitted the following
>  PANIC error during shutdown checkpoint.
> 
>  PANIC:  concurrent transaction log activity while database system is
>  shutting down
> 
>  The cause of this problem is that walsender for logical replication can
>  generate WAL records even during shutdown checkpoint.
> 
>  Firstly walsender keeps running until shutdown checkpoint finishes
>  so that all the WAL including shutdown checkpoint record can be
>  replicated to the standby. This was safe because previously walsender
>  could not generate WAL records. However this assumption became
>  invalid because of logical replication. That is, currenty walsender for
>  logical replication can generate WAL records, for example, by executing
>  CREATE_REPLICATION_SLOT command. This is an oversight in
>  logical replication patch, I think.
> >>>
> >>> Hmm, but CREATE_REPLICATION_SLOT should not generate WAL afaik. I agree
> >>> that the issue with walsender still exist (since we now allow normal SQL
> >>> to run there) but I think it's important to identify what exactly causes
> >>> the WAL activity in your case
> >>
> >> At least in my case, the following CREATE_REPLICATION_SLOT command
> >> generated WAL record.
> >>
> >> BEGIN READ ONLY ISOLATION LEVEL REPEATABLE READ;
> >> CREATE_REPLICATION_SLOT testslot TEMPORARY LOGICAL pgoutput 
> >> USE_SNAPSHOT;
> >>
> >> Here is the pg_waldump output of the WAL record that 
> >> CREATE_REPLICATION_SLOT
> >> generated.
> >>
> >> rmgr: Standby len (rec/tot): 24/50, tx:  0,
> >> lsn: 0/01601438, prev 0/01601400, desc: RUNNING_XACTS nextXid 692
> >> latestCompletedXid 691 oldestRunningXid 692
> >>
> >> So I guess that CREATE_REPLICATION_SLOT code calls LogStandbySnapshot()
> >> and which generates WAL record about snapshot of running transactions.
> > 
> > Erroring out in these cases sounds easy enough.  Wonder if there's not a
> > bigger problem with WAL records generated e.g. by HOT pruning or such,
> > during decoding.  Not super likely, but would probably hit exactly the
> > same, no?
> > 
> 
> Sounds possible, yes. Sounds like that's going to be nontrivial to fix
> though.
> 
> Another problem is that queries can run on walsender now. But that
> should be possible to detect and shutdown just like backend.

This sounds like a case for s/PANIC/ERROR|FATAL/ to me...


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

Re: [HACKERS] logical replication and PANIC during shutdown checkpoint in publisher

2017-04-17 Thread Petr Jelinek

On 17/04/17 18:02, Andres Freund wrote:
> On 2017-04-15 02:33:59 +0900, Fujii Masao wrote:
>> On Fri, Apr 14, 2017 at 10:33 PM, Petr Jelinek
>>  wrote:
>>> On 12/04/17 15:55, Fujii Masao wrote:
 Hi,

 When I shut down the publisher while I repeated creating and dropping
 the subscription in the subscriber, the publisher emitted the following
 PANIC error during shutdown checkpoint.

 PANIC:  concurrent transaction log activity while database system is
 shutting down

 The cause of this problem is that walsender for logical replication can
 generate WAL records even during shutdown checkpoint.

 Firstly walsender keeps running until shutdown checkpoint finishes
 so that all the WAL including shutdown checkpoint record can be
 replicated to the standby. This was safe because previously walsender
 could not generate WAL records. However this assumption became
 invalid because of logical replication. That is, currenty walsender for
 logical replication can generate WAL records, for example, by executing
 CREATE_REPLICATION_SLOT command. This is an oversight in
 logical replication patch, I think.
>>>
>>> Hmm, but CREATE_REPLICATION_SLOT should not generate WAL afaik. I agree
>>> that the issue with walsender still exist (since we now allow normal SQL
>>> to run there) but I think it's important to identify what exactly causes
>>> the WAL activity in your case
>>
>> At least in my case, the following CREATE_REPLICATION_SLOT command
>> generated WAL record.
>>
>> BEGIN READ ONLY ISOLATION LEVEL REPEATABLE READ;
>> CREATE_REPLICATION_SLOT testslot TEMPORARY LOGICAL pgoutput USE_SNAPSHOT;
>>
>> Here is the pg_waldump output of the WAL record that CREATE_REPLICATION_SLOT
>> generated.
>>
>> rmgr: Standby len (rec/tot): 24/50, tx:  0,
>> lsn: 0/01601438, prev 0/01601400, desc: RUNNING_XACTS nextXid 692
>> latestCompletedXid 691 oldestRunningXid 692
>>
>> So I guess that CREATE_REPLICATION_SLOT code calls LogStandbySnapshot()
>> and which generates WAL record about snapshot of running transactions.
> 
> Erroring out in these cases sounds easy enough.  Wonder if there's not a
> bigger problem with WAL records generated e.g. by HOT pruning or such,
> during decoding.  Not super likely, but would probably hit exactly the
> same, no?
> 

Sounds possible, yes. Sounds like that's going to be nontrivial to fix
though.

Another problem is that queries can run on walsender now. But that
should be possible to detect and shutdown just like backend.

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


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

Re: [HACKERS] logical replication and PANIC during shutdown checkpoint in publisher

On 2017-04-15 02:33:59 +0900, Fujii Masao wrote:
> On Fri, Apr 14, 2017 at 10:33 PM, Petr Jelinek
>  wrote:
> > On 12/04/17 15:55, Fujii Masao wrote:
> >> Hi,
> >>
> >> When I shut down the publisher while I repeated creating and dropping
> >> the subscription in the subscriber, the publisher emitted the following
> >> PANIC error during shutdown checkpoint.
> >>
> >> PANIC:  concurrent transaction log activity while database system is
> >> shutting down
> >>
> >> The cause of this problem is that walsender for logical replication can
> >> generate WAL records even during shutdown checkpoint.
> >>
> >> Firstly walsender keeps running until shutdown checkpoint finishes
> >> so that all the WAL including shutdown checkpoint record can be
> >> replicated to the standby. This was safe because previously walsender
> >> could not generate WAL records. However this assumption became
> >> invalid because of logical replication. That is, currenty walsender for
> >> logical replication can generate WAL records, for example, by executing
> >> CREATE_REPLICATION_SLOT command. This is an oversight in
> >> logical replication patch, I think.
> >
> > Hmm, but CREATE_REPLICATION_SLOT should not generate WAL afaik. I agree
> > that the issue with walsender still exist (since we now allow normal SQL
> > to run there) but I think it's important to identify what exactly causes
> > the WAL activity in your case
> 
> At least in my case, the following CREATE_REPLICATION_SLOT command
> generated WAL record.
> 
> BEGIN READ ONLY ISOLATION LEVEL REPEATABLE READ;
> CREATE_REPLICATION_SLOT testslot TEMPORARY LOGICAL pgoutput USE_SNAPSHOT;
> 
> Here is the pg_waldump output of the WAL record that CREATE_REPLICATION_SLOT
> generated.
> 
> rmgr: Standby len (rec/tot): 24/50, tx:  0,
> lsn: 0/01601438, prev 0/01601400, desc: RUNNING_XACTS nextXid 692
> latestCompletedXid 691 oldestRunningXid 692
> 
> So I guess that CREATE_REPLICATION_SLOT code calls LogStandbySnapshot()
> and which generates WAL record about snapshot of running transactions.

Erroring out in these cases sounds easy enough.  Wonder if there's not a
bigger problem with WAL records generated e.g. by HOT pruning or such,
during decoding.  Not super likely, but would probably hit exactly the
same, no?

- 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] On How To Shorten the Steep Learning Curve Towards PG Hacking...

2017-04-17 Thread Kevin Grittner

On Tue, Mar 28, 2017 at 10:36 PM, Craig Ringer  wrote:

> Personally I have to agree that the learning curve is very steep. Some
> of the docs and presentations help, but there's a LOT to understand.

Some small patches can be kept to a fairly narrow set of areas, and
if you can find a similar capability to can crib technique for
handling some of the more mysterious areas it might brush up
against.  When I started working on my first *big* patch that was
bound to touch many areas (around the start of development for 9.1)
I counted lines of code and found over a million lines just in .c
and .h files.  We're now closing in on 1.5 million lines.  That's
not counting over 376,000 lines of documentation in .sgml files,
over 12,000 lines of text in README* files, over 26,000 lines of
perl code, over 103,000 lines of .sql code (60% of which is in
regression tests), over 38,000 lines of .y code (for flex/bison
parsing), about 9,000 lines of various type of code just for
generating the configure file, and over 439,000 lines of .po files
(for message translations).  I'm sure I missed a lot of important
stuff there, but it gives some idea the challenge it is to get your
head around it all.

My first advice is to try to identify which areas of the code you
will need to touch, and read those over.  Several times.  Try to
infer the API to areas *that* code needs to reference from looking
at other code (as similar to what you want to work on as you can
find), reading code comments and README  files, and asking
questions.  Secondly, there is a lot that is considered to be
"coding rules" that is, as far as I've been able to tell, only
contained inside the heads of veteran PostgreSQL coders, with
occasional references in the discussion list archives.  Asking
questions, proposing approaches before coding, and showing work in
progress early and often will help a lot in terms of discovering
these issues and allowing you to rearrange things to fit these
conventions.  If someone with the "gift of gab" is able to capture
these and put them into a readily available form, that would be
fantastic.

> * SSI (haven't gone there yet myself)

For anyone wanting to approach this area, there is a fair amount to
look at.  There is some overlap, but in rough order of "practical"
to "theoretical foundation", you might want to look at:

https://www.postgresql.org/docs/current/static/transaction-iso.html

https://wiki.postgresql.org/wiki/SSI

The SQL standard

https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob_plain;f=src/backend/storage/lmgr/README-SSI;hb=refs/heads/master

http://www.vldb.org/pvldb/vol5.html

http://hdl.handle.net/2123/5353

Papers cited in these last two.  I have found papers authored by
Alan Fekete or Adul Adya particularly enlightening.

If any of the other areas that Craig listed have similar work
available, maybe we should start a Wiki page where we list areas of
code (starting with the list Craig included) as section headers, and
put links to useful reading below each?

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

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

[HACKERS] PGCon 2017 registration now open

2017-04-17 Thread Dan Langille

Join us in Ottawa for the 11th annual PGCon.  On May 23-26, users and 
developers from 
around the world arrive for what has become a traditional gathering of the 
PostgreSQL
community.

There will be two days of tutorials on Tuesday and Wednesday.  The best of the 
best 
will be available to help you learn great things about PostgreSQL and its tools.
See http://www.pgcon.org/2017/schedule/track/Tutorial/index.en.html 


On Wednesday, there will be a Developer Unconference (non-developers are 
welcome 
too).  The Unconference first appeared at PGCon 2013 and was a instant success.
See https://wiki.postgresql.org/wiki/PgCon_2017_Developer_Unconference 


For Thursday and Friday, the submitted talks will be presented as everyone 
gathers in 
one location to learn, discuss, and collaborate.

Full list of talks: http://www.pgcon.org/2017/schedule/events.en.html 


In summary:

• Tutorials: 23-34 May 2017 (Tue & Wed)
• Unconference: 24 May 2016
• Talks: 25-26 May 2016 (Thu-Fri).

Registration is now open at http://www.pgcon.org/2017/registration.php 


-- 
Dan Langille - BSDCan / PGCon
d...@langille.org

Re: [HACKERS] OK, so culicidae is still broken

On 2017-04-15 14:34:28 -0700, Andres Freund wrote:
> On 2017-04-15 17:30:21 -0400, Tom Lane wrote:
> > Andres Freund  writes:
> > > On 2017-04-15 16:48:05 -0400, Tom Lane wrote:
> > >> Concretely, I propose the attached patch.  We'd have to put it into
> > >> all supported branches, since culicidae is showing intermittent
> > >> "could not reattach to shared memory" failures in all the branches.
> > 
> > > That seems quite reasonable.
> > 
> > Pushed, please update culicidae's settings.
> 
> Done, although there were already builds running by the time I got to
> it, so there'll be a few unaffected runs.  I've increased build
> frequency of all branches to be forced once-an-hour, so we can quickly
> see how reliable it is.  Will turn down Monday or such, if everything
> looks good.

Looking through all the branches, it seems to have done the trick - the
previous irregular failures seem to be gone.  Nice.

Unless somebody complains, I'll reduce the forced build frequency to
something more normal again.

- 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] PANIC in pg_commit_ts slru after crashes

2017-04-17 Thread Jeff Janes

On Sun, Apr 16, 2017 at 6:59 PM, Michael Paquier 
wrote:

>
>
> Jeff, does this patch make the situation better? The fix is rather
> simple as it just makes sure that the next XID never gets updated if
> there are no 2PC files.
>

Yes, that fixes the reported case when 2PC are not being used.

Thanks,

Jeff

Re: [HACKERS] Re: extended stats not friendly towards ANALYZE with subset of columns

Noah Misch wrote:
> On Wed, Apr 05, 2017 at 08:58:54AM -0300, Alvaro Herrera wrote:
> > Noah Misch wrote:
> > > The above-described topic is currently a PostgreSQL 10 open item.  Álvaro,
> > > since you committed the patch believed to have created it, you own this 
> > > open
> > > item.
> > 
> > I'll commit a fix for this problem no later than Friday 14th.
> 
> This PostgreSQL 10 open item is past due for your status update.  Kindly send
> a status update within 24 hours, and include a date for your subsequent status
> update.  Refer to the policy on open item ownership:
> https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com

I'm working on the patch currently and intend to get it pushed no later
than tomorrow 19:00 America/Santiago.

-- 
Á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] some review comments on logical rep code

On Mon, Apr 17, 2017 at 9:13 PM, Masahiko Sawada  wrote:
> On Mon, Apr 17, 2017 at 7:39 PM, Kyotaro HORIGUCHI
>  wrote:
>> At Mon, 17 Apr 2017 18:02:57 +0900, Masahiko Sawada  
>> wrote in 
>>> On Fri, Apr 14, 2017 at 4:47 AM, Fujii Masao  wrote:
>>> > Hi,
>>> >
>>> > Though I've read only a part of the logical rep code yet, I'd like to
>>> > share some (relatively minor) review comments that I got so far.
>>>
>>> It seems nobody is working on dealing with these review comments, so
>>> I've attached patches. Since there are lots of review comment I
>>> numbered each review comment. The prefix of patches I attached is
>>> corresponding to review comment number.
>>>
>
> Thank you for reviewing.
>
>>> 1.
>>> >
>>> > In ApplyWorkerMain(), DatumGetInt32() should be used to get integer
>>> > value from the argument, instead of DatumGetObjectId().
>>>
>>> Attached 001 patch fixes it.
>>
>> Hmm. I looked at the launcher side and found that it assigns bare
>> integer to bgw_main_arg. It also should use Int32GetDatum.
>
> Yeah, I agreed. Will fix it.
>
>>
>>> logicalrep_worker_launch(Oid dbid, Oid subid, const char *subname, Oid 
>>> userid,
>>>  Oid relid)
>>> {
>>> int  slot;
>> ...
>>> for (slot = 0; slot < max_logical_replication_workers; slot++)
>> ...
>>> bgw.bgw_main_arg = slot;
>>
>>
>>
>>> 2.
>>> >
>>> > No one resets on_commit_launcher_wakeup flag to false.
>>>
>>> Attached 002 patch makes on_commit_launcher_wakeup turn off after woke
>>> up the launcher process.
>>
>> It is omitting the abort case. Maybe it should be
>> AtEOXact_ApplyLauncher(bool commit)?
>
> Should we wake up the launcher process even when abort?
>
>>
>>> 3.
>>> >
>>> > ApplyLauncherWakeup() should be static function.
>>>
>>> Attached 003 patch fixes it (and also fixes #5 review comment).
>>>
>>> 4.
>>> >
>>> > Seems not only CREATE SUBSCRIPTION but also ALTER SUBSCRIPTION's
>>> > subcommands (e.g., ENABLED one) should wake the launcher up on commit.
>>>
>>> This is also reported by Horiguchi-san on another thread[1], and patch 
>>> exists.
>>>
>>> 5.
>>> >
>>> > Why is IsBackendPid() check necessary in ApplyLauncherWakeup()?
>>>
>>> I also guess it's necessary. This change is included in #3 patch.
>>
>> IsBackendPid() is not free, I suppose that just ignoring failure
>> with ESRCH is enough.
>
> After thought, since LogicalRepCtx->launcher_pid could be 0 should, we
> check if launcher_pid != 0?
>
>>
>>> 6.
>>> >
>>> > Normal statements that the walsender for logical rep runs are logged
>>> > by log_replication_commands. So if log_statement is also set,
>>> > those commands are logged twice.
>>>
>>> Attached 006 patch fixes it by adding  "-c log_statement = none" to
>>> connection parameter of libpqrcv if logical = true.
>>
>> The control by log_replication_commands is performed on
>> walsender, so this also shuld be done on the same place. Anyway
>> log_statement is irrelevant to walsender.
>
> Patch 006 emits all logs executed by the apply worker as a log of
> log_replication_command but perhaps you're right. It would be better
> to leave the log of log_statement if the command executed by the apply
> worker is a SQL command. Will fix.
>
>>
>>> 7.
>>> >
>>> > LogicalRepCtx->launcher_pid isn't reset to 0 when the launcher emits
>>> > an error. The callback function to reset it needs to be registered
>>> > via on_shmem_exit().
>>>
>>> Attached 007 patch adds callback function to reset 
>>> LogicalRepCtx->launcher_pid.
>>>
>>> 8.
>>> >
>>> > Typo: "an subscriber" should be "a subscriber" in some places.
>>>
>>> It seems that there is no longer these typo.
>>>
>>> 9.
>>> > /* Get conninfo */
>>> > datum = SysCacheGetAttr(SUBSCRIPTIONOID,
>>> > tup,
>>> > Anum_pg_subscription_subconninfo,
>>> > &isnull);
>>> > Assert(!isnull);
>>> >
>>> > This assertion makes me think that pg_subscription.subconnifo should
>>> > have NOT NULL constraint. Also subslotname and subpublications
>>> > should have NOT NULL constraint because of the same reason.
>>>
>>> Agreed. Attached 009 patch add NOT NULL constraint to all other
>>> columns of pg_subscription. pg_subscription.subsynccommit should have
>>> it as well.
>>
>> I'm not sure the policy here, but I suppose that the assertions
>> there are still required irrelevantly from the nun-nullness of
>> the attribute.
>
> You're right. Will fix it.
>
>>> 10.
>>> >
>>> > SpinLockAcquire(&MyLogicalRepWorker->relmutex);
>>> > MyLogicalRepWorker->relstate =
>>> >   GetSubscriptionRelState(MyLogicalRepWorker->subid,
>>> > MyLogicalRepWorker->relid,
>>> > &MyLogicalRepWorker->relstate_lsn,
>>> > false);
>>> > SpinLockRelease(&MyLogicalRepWorker->relmutex);
>>> >
>>> > Non-"short-term" function like GetSubscriptionRelState() should not
>>> > be called while spinlock is being held.
>>> >
>>>
>>> One option is adding new LWLock for the relation state change but this
>>> lock is used at many locations

Re: [HACKERS] Logical replication and inheritance

On 4/16/17 23:00, Amit Langote wrote:
>> To fix this, pg_dump should emit ADD TABLE ONLY foo.
> 
> Yeah, that's one way.  Attached is a tiny patch for that.
> 
> By the way, I noticed that although grammar accepts ONLY and * against a
> table name to affect whether descendant tables are included, the same is
> not mentioned in the CREATE PUBLICATION and ALTER PUBLICATION reference
> pages.  I suspect it was probably not intentional, so attached a doc patch
> for that too.

Committed those, with some extra tests.

-- 
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] Proposal: Local indexes for partitioned table

2017-04-17 Thread Maksim Milyutin


On 10.04.2017 14:20, Robert Haas wrote:

On Tue, Apr 4, 2017 at 12:10 PM, Maksim Milyutin
 wrote:

1. I have added a new relkind for local indexes named RELKIND_LOCAL_INDEX
(literal 'l').


Seems like it should maybe be RELKIND_PARTITIONED_INDEX.  There's
nothing particularly "local" about it.  I suppose what you're going
for is that it's not global, but in a way it *is* global to the
partitioning hierarchy.  That's the point.  It's just that it's
partitioned.



Ok, thanks for the note.

But I want to discuss the relevancy of introduction of a new relkind for 
partitioned index. I could to change the control flow in partitioned 
index creation (specify conditional statement in the 'index_create' 
routine in attached patch) and not enter to the 'heap_create' routine. 
This case releases us from integrating new relkind into different places 
of Postgres code. But we have to copy-paste some specific code from 
'heap_create' function, e.g., definition of relfilenode and tablespaceid 
for the new index and perhaps something more when 'heap_create' routine 
will be extended.


What do you think about this way?


--
Maksim Milyutin
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 2328b92..9c15bc9 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -41,6 +41,7 @@
 #include "catalog/pg_collation.h"
 #include "catalog/pg_constraint.h"
 #include "catalog/pg_constraint_fn.h"
+#include "catalog/pg_depend.h"
 #include "catalog/pg_operator.h"
 #include "catalog/pg_opclass.h"
 #include "catalog/pg_tablespace.h"
@@ -849,17 +850,33 @@ index_create(Relation heapRelation,
 	 * we fail further down, it's the smgr's responsibility to remove the disk
 	 * file again.)
 	 */
-	indexRelation = heap_create(indexRelationName,
-namespaceId,
-tableSpaceId,
-indexRelationId,
-relFileNode,
-indexTupDesc,
-RELKIND_INDEX,
-relpersistence,
-shared_relation,
-mapped_relation,
-allow_system_table_mods);
+	if (heapRelation->rd_rel->relkind != RELKIND_PARTITIONED_TABLE)
+		indexRelation = heap_create(indexRelationName,
+	namespaceId,
+	tableSpaceId,
+	indexRelationId,
+	relFileNode,
+	indexTupDesc,
+	RELKIND_INDEX,
+	relpersistence,
+	shared_relation,
+	mapped_relation,
+	allow_system_table_mods);
+	else
+		indexRelation =
+			RelationBuildLocalRelation(indexRelationName,
+	   namespaceId,
+	   indexTupDesc,
+	   indexRelationId,
+	   (OidIsValid(relFileNode)) ?
+		relFileNode : indexRelationId,
+	   (tableSpaceId == MyDatabaseTableSpace) ?
+		InvalidOid : tableSpaceId,
+	   shared_relation,
+	   mapped_relation,
+	   relpersistence,
+	   RELKIND_INDEX);
+
 
 	Assert(indexRelationId == RelationGetRelid(indexRelation));
 
@@ -1549,10 +1566,14 @@ index_drop(Oid indexId, bool concurrent)
 		TransferPredicateLocksToHeapRelation(userIndexRelation);
 	}
 
-	/*
-	 * Schedule physical removal of the files
-	 */
-	RelationDropStorage(userIndexRelation);
+	if (userHeapRelation->rd_rel->relkind != RELKIND_PARTITIONED_TABLE)
+	{
+
+		/*
+		 * Schedule physical removal of the files
+		 */
+		RelationDropStorage(userIndexRelation);
+	}
 
 	/*
 	 * Close and flush the index's relcache entry, to ensure relcache doesn't
@@ -3294,6 +3315,111 @@ IndexGetRelation(Oid indexId, bool missing_ok)
 }
 
 /*
+ * Find all leaf indexes included into local index with 'indexId' oid and lock
+ * all dependent indexes and respective relations.
+ *
+ * Search is performed in pg_depend table since all indexes belonging to child
+ * tables depends on index from parent table.
+ *
+ *	indexId: the oid of local index whose leaf indexes need to find
+ *	result: list of result leaf indexes
+ *	depRel: already opened pg_depend relation
+ *	indexLockmode: lockmode for indexes' locks
+ *	heapLockmode: lockmode for relations' locks
+ */
+static void
+findDepedentLeafIndexes(Oid indexId, List **result, Relation depRel,
+		LOCKMODE indexLockmode, LOCKMODE heapLockmode)
+{
+	ScanKeyData 		key[3];
+	int	nkeys;
+	SysScanDesc 		scan;
+	HeapTuple			tup;
+	List			   *localSubIndexIds = NIL;
+	ListCell		   *lc;
+
+	ScanKeyInit(&key[0],
+Anum_pg_depend_refclassid,
+BTEqualStrategyNumber, F_OIDEQ,
+ObjectIdGetDatum(RelationRelationId));
+	ScanKeyInit(&key[1],
+Anum_pg_depend_refobjid,
+BTEqualStrategyNumber, F_OIDEQ,
+ObjectIdGetDatum(indexId));
+	nkeys = 2;
+
+	scan = systable_beginscan(depRel, DependReferenceIndexId, true,
+			  NULL, nkeys, key);
+
+	while (HeapTupleIsValid(tup = systable_getnext(scan)))
+	{
+		Form_pg_depend 	foundDep = (Form_pg_depend) GETSTRUCT(tup);
+		Relation		index,
+		heap;
+
+		if (foundDep->classid != RelationRelationId)
+			continue;
+
+		/* Open and lock c

Re: [HACKERS] Logical replication - TRAP: FailedAssertion in pgstat.c

2017-04-17 Thread Stas Kelvich


> On 17 Apr 2017, at 10:30, Erik Rijkers  wrote:
> 
> On 2017-04-16 20:41, Andres Freund wrote:
>> On 2017-04-16 10:46:21 +0200, Erik Rijkers wrote:
>>> On 2017-04-15 04:47, Erik Rijkers wrote:
>>> >
>>> > 0001-Reserve-global-xmin-for-create-slot-snasphot-export.patch +
>>> > 0002-Don-t-use-on-disk-snapshots-for-snapshot-export-in-l.patch+
>>> > 0003-Prevent-snapshot-builder-xmin-from-going-backwards.patch  +
>>> > 0004-Fix-xl_running_xacts-usage-in-snapshot-builder.patch  +
>>> > 0005-Skip-unnecessary-snapshot-builds.patch
>>> I am now using these newer patches:
>>> https://www.postgresql.org/message-id/30242bc6-eca4-b7bb-670e-8d0458753a8c%402ndquadrant.com
>>> > It builds fine, but when I run the old pbench-over-logical-replication
>>> > test I get:
>>> >
>>> > TRAP: FailedAssertion("!(entry->trans == ((void *)0))", File:
>>> > "pgstat.c", Line: 828)
>>> To get that error:
>> I presume this is the fault of
>> http://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=139eb9673cb84c76f493af7e68301ae204199746
>> if you git revert that individual commit, do things work again?
> 
> Yes, compiled from 67c2def11d4 with the above 4 patches, it runs flawlessly 
> again. (flawlessly= a few hours without any error)
> 

I’ve reproduced failure, this happens under tablesync worker and putting
pgstat_report_stat() under the previous condition block should help.

However for me it took about an hour of running this script to catch original 
assert.

Can you check with that patch applied?




logical_worker.patch
Description: Binary data

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] Different table schema in logical replication crashes

2017-04-17 Thread Euler Taveira

2017-04-14 22:36 GMT-03:00 Petr Jelinek :

> I tried something bit different which seems cleaner to me - use the
> pstate->r_table instead of ad-hock locally made up range table and fill
> that using standard addRangeTableEntryForRelation. Both in tablesync and
> in DoCopy instead of the old coding.
>

Patch works fine. However, I don't see any documentation about supporting
different schemas for logical replication. Is it an oversight?


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

Re: [HACKERS] Re: logical replication and PANIC during shutdown checkpoint in publisher

2017-04-17 Thread Petr Jelinek

On 16/04/17 08:12, Noah Misch wrote:
> On Wed, Apr 12, 2017 at 10:55:08PM +0900, Fujii Masao wrote:
>> When I shut down the publisher while I repeated creating and dropping
>> the subscription in the subscriber, the publisher emitted the following
>> PANIC error during shutdown checkpoint.
>>
>> PANIC:  concurrent transaction log activity while database system is
>> shutting down
>>
>> The cause of this problem is that walsender for logical replication can
>> generate WAL records even during shutdown checkpoint.
>>
>> Firstly walsender keeps running until shutdown checkpoint finishes
>> so that all the WAL including shutdown checkpoint record can be
>> replicated to the standby. This was safe because previously walsender
>> could not generate WAL records. However this assumption became
>> invalid because of logical replication. That is, currenty walsender for
>> logical replication can generate WAL records, for example, by executing
>> CREATE_REPLICATION_SLOT command. This is an oversight in
>> logical replication patch, I think.
>>
>> To fix this issue, we should terminate walsender for logical replication
>> before shutdown checkpoint starts. Of course walsender for physical
>> replication still needs to keep running until shutdown checkpoint ends,
>> though.
> 
> [Action required within three days.  This is a generic notification.]
> 
> The above-described topic is currently a PostgreSQL 10 open item.  Peter,
> since you committed the patch believed to have created it, you own this open
> item.  If some other commit is more relevant or if this does not belong as a
> v10 open item, please let us know.  Otherwise, please observe the policy on
> open item ownership[1] and send a status update within three calendar days of
> this message.  Include a date for your subsequent status update.  Testers may
> discover new open items at any time, and I want to plan to get them all fixed
> well in advance of shipping v10.  Consequently, I will appreciate your efforts
> toward speedy resolution.  Thanks.
> 

Just FYI this is not new in 10, the issue exists since the 9.4
introduction of logical replication slots.

-- 
  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] Optimization for updating foreign tables in Postgres FDW

2017-04-17 Thread Ashutosh Bapat

On Mon, Apr 17, 2017 at 1:53 PM, Kyotaro HORIGUCHI
 wrote:
> At Thu, 13 Apr 2017 13:04:12 -0400, Robert Haas  wrote 
> in 
>> On Thu, Apr 21, 2016 at 10:53 AM, Robert Haas  wrote:
>> > On Thu, Apr 21, 2016 at 8:44 AM, Michael Paquier
>> >  wrote:
>> >> On Thu, Apr 21, 2016 at 5:22 PM, Etsuro Fujita
>> >>  wrote:
>> >>> Attached is an updated version of the patch, which modified Michael's
>> >>> version of the patch, as I proposed in [1] (see "Other changes:").  I
>> >>> modified comments for pgfdw_get_result/pgfdw_exec_query also, mainly 
>> >>> because
>> >>> words like "non-blocking mode" there seems confusing (note that we have
>> >>> PQsetnonbloking).
>> >>
>> >> OK, so that is what I sent except that the comments mentioning PG_TRY
>> >> are moved to their correct places. That's fine for me. Thanks for
>> >> gathering everything in a single patch and correcting it.
>> >
>> > I have committed this patch.  Thanks for working on this.  Sorry for the 
>> > delay.
>>
>> This 9.6-era patch, as it turns out, has a problem, which is that we
>> now respond to an interrupt by sending a cancel request and a
>> NON-interruptible ABORT TRANSACTION command to the remote side.  If
>> the reason that the user is trying to interrupt is that the network
>> connection has been cut, they interrupt the original query only to get
>> stuck in a non-interruptible wait for ABORT TRANSACTION.  That is
>> non-optimal.
>
> Agreed.
>
>> It is not exactly clear to me how to fix this.  Could we get by with
>> just slamming the remote connection shut, instead of sending an
>> explicit ABORT TRANSACTION?  The remote side ought to treat a
>> disconnect as equivalent to an ABORT anyway, I think, but maybe our
>> local state would get confused.  (I have not checked.)
>>
>> Thoughts?
>
> Perhaps we will get stuck at query cancellation before ABORT
> TRANSACTION in the case. A connection will be shut down when
> anything wrong (PQstatus(conn) != CONNECTION_OK and so) on
> aborting local transactoin . So I don't think fdw gets confused
> or sholdn't be confused by shutting down there.
>
> The most significant issue I can see is that the same thing
> happens in the case of graceful ABORT TRANSACTION. It could be a
> performance issue.
>
> We could set timeout here but maybe we can just slamming the
> connection down instead of sending a query cancellation. It is
> caused only by timeout or interrupts so I suppose it is not a
> problem *with a few connections*.
>
>
> Things are a bit diffent with hundreds of connections. The
> penalty of reconnection would be very high in the case.
>
> If we are not willing to pay such high penalty, maybe we are to
> manage busy-idle time of each connection and trying graceful
> abort if it is short enough, maybe having a shoft timeout.
>
> Furthermore, if most or all of the hundreds of connections get
> stuck, such timeout will accumulate up like a mountain...

Even when the transaction is aborted because a user cancels a query,
we do want to preserve the connection, if possible, to avoid
reconnection. If the request to cancel the query itself fails, we
should certainly drop the connection. Here's the patch to do that.

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


0001-Drop-connection-when-query-can-not-be-cancelled.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] some review comments on logical rep code

On Mon, Apr 17, 2017 at 7:39 PM, Kyotaro HORIGUCHI
 wrote:
> At Mon, 17 Apr 2017 18:02:57 +0900, Masahiko Sawada  
> wrote in 
>> On Fri, Apr 14, 2017 at 4:47 AM, Fujii Masao  wrote:
>> > Hi,
>> >
>> > Though I've read only a part of the logical rep code yet, I'd like to
>> > share some (relatively minor) review comments that I got so far.
>>
>> It seems nobody is working on dealing with these review comments, so
>> I've attached patches. Since there are lots of review comment I
>> numbered each review comment. The prefix of patches I attached is
>> corresponding to review comment number.
>>

Thank you for reviewing.

>> 1.
>> >
>> > In ApplyWorkerMain(), DatumGetInt32() should be used to get integer
>> > value from the argument, instead of DatumGetObjectId().
>>
>> Attached 001 patch fixes it.
>
> Hmm. I looked at the launcher side and found that it assigns bare
> integer to bgw_main_arg. It also should use Int32GetDatum.

Yeah, I agreed. Will fix it.

>
>> logicalrep_worker_launch(Oid dbid, Oid subid, const char *subname, Oid 
>> userid,
>>  Oid relid)
>> {
>> int  slot;
> ...
>> for (slot = 0; slot < max_logical_replication_workers; slot++)
> ...
>> bgw.bgw_main_arg = slot;
>
>
>
>> 2.
>> >
>> > No one resets on_commit_launcher_wakeup flag to false.
>>
>> Attached 002 patch makes on_commit_launcher_wakeup turn off after woke
>> up the launcher process.
>
> It is omitting the abort case. Maybe it should be
> AtEOXact_ApplyLauncher(bool commit)?

Should we wake up the launcher process even when abort?

>
>> 3.
>> >
>> > ApplyLauncherWakeup() should be static function.
>>
>> Attached 003 patch fixes it (and also fixes #5 review comment).
>>
>> 4.
>> >
>> > Seems not only CREATE SUBSCRIPTION but also ALTER SUBSCRIPTION's
>> > subcommands (e.g., ENABLED one) should wake the launcher up on commit.
>>
>> This is also reported by Horiguchi-san on another thread[1], and patch 
>> exists.
>>
>> 5.
>> >
>> > Why is IsBackendPid() check necessary in ApplyLauncherWakeup()?
>>
>> I also guess it's necessary. This change is included in #3 patch.
>
> IsBackendPid() is not free, I suppose that just ignoring failure
> with ESRCH is enough.

After thought, since LogicalRepCtx->launcher_pid could be 0 should, we
check if launcher_pid != 0?

>
>> 6.
>> >
>> > Normal statements that the walsender for logical rep runs are logged
>> > by log_replication_commands. So if log_statement is also set,
>> > those commands are logged twice.
>>
>> Attached 006 patch fixes it by adding  "-c log_statement = none" to
>> connection parameter of libpqrcv if logical = true.
>
> The control by log_replication_commands is performed on
> walsender, so this also shuld be done on the same place. Anyway
> log_statement is irrelevant to walsender.

Patch 006 emits all logs executed by the apply worker as a log of
log_replication_command but perhaps you're right. It would be better
to leave the log of log_statement if the command executed by the apply
worker is a SQL command. Will fix.

>
>> 7.
>> >
>> > LogicalRepCtx->launcher_pid isn't reset to 0 when the launcher emits
>> > an error. The callback function to reset it needs to be registered
>> > via on_shmem_exit().
>>
>> Attached 007 patch adds callback function to reset 
>> LogicalRepCtx->launcher_pid.
>>
>> 8.
>> >
>> > Typo: "an subscriber" should be "a subscriber" in some places.
>>
>> It seems that there is no longer these typo.
>>
>> 9.
>> > /* Get conninfo */
>> > datum = SysCacheGetAttr(SUBSCRIPTIONOID,
>> > tup,
>> > Anum_pg_subscription_subconninfo,
>> > &isnull);
>> > Assert(!isnull);
>> >
>> > This assertion makes me think that pg_subscription.subconnifo should
>> > have NOT NULL constraint. Also subslotname and subpublications
>> > should have NOT NULL constraint because of the same reason.
>>
>> Agreed. Attached 009 patch add NOT NULL constraint to all other
>> columns of pg_subscription. pg_subscription.subsynccommit should have
>> it as well.
>
> I'm not sure the policy here, but I suppose that the assertions
> there are still required irrelevantly from the nun-nullness of
> the attribute.

You're right. Will fix it.

>> 10.
>> >
>> > SpinLockAcquire(&MyLogicalRepWorker->relmutex);
>> > MyLogicalRepWorker->relstate =
>> >   GetSubscriptionRelState(MyLogicalRepWorker->subid,
>> > MyLogicalRepWorker->relid,
>> > &MyLogicalRepWorker->relstate_lsn,
>> > false);
>> > SpinLockRelease(&MyLogicalRepWorker->relmutex);
>> >
>> > Non-"short-term" function like GetSubscriptionRelState() should not
>> > be called while spinlock is being held.
>> >
>>
>> One option is adding new LWLock for the relation state change but this
>> lock is used at many locations where the operation actually doesn't
>> take time. I think that the discussion would be needed to get
>> consensus, so patch for it is not attached.
>
> From the point of view of time, I agree that it doesn't seem to
> harm

Re: [HACKERS] pgbench - allow to store select results into variables



Hello,


I think you'd better to change the following comments because there's
no psqlscan.l or psqlscanslash.l in pgbench source tree.

+ * underscore.  Keep this in sync with the definition of variable_char in
+ * psqlscan.l and psqlscanslash.l.


Here is a v3 with a more precise comment.

--
Fabien.diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml
index 1eee8dc..502d31b 100644
--- a/doc/src/sgml/ref/pgbench.sgml
+++ b/doc/src/sgml/ref/pgbench.sgml
@@ -825,6 +825,8 @@ pgbench  options  dbname
  
   Sets variable varname to a value calculated
   from expression.
+  The variable name must consist of letters (including non-Latin letters),
+  digits, and underscores.
   The expression may contain integer constants such as 5432,
   double constants such as 3.14159,
   references to variables :variablename,
diff --git a/src/bin/pgbench/exprscan.l b/src/bin/pgbench/exprscan.l
index dc1367b..1862fe4 100644
--- a/src/bin/pgbench/exprscan.l
+++ b/src/bin/pgbench/exprscan.l
@@ -58,9 +58,9 @@ extern void expr_yyset_column(int column_no, yyscan_t yyscanner);
 %option prefix="expr_yy"
 
 /* Character classes */
-alpha			[a-zA-Z_]
+alpha			[a-zA-Z\200-\377_]
 digit			[0-9]
-alnum			[a-zA-Z0-9_]
+alnum			[A-Za-z\200-\377_0-9]
 /* {space} + {nonspace} + {newline} should cover all characters */
 space			[ \t\r\f\v]
 nonspace		[^ \t\r\f\v\n]
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index ae36247..44ae29e 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -1020,19 +1020,38 @@ makeVariableNumeric(Variable *var)
 	return true;
 }
 
-/* check whether the name consists of alphabets, numerals and underscores. */
+/*
+ * Check whether a variable's name is allowed.
+ *
+ * We allow any non-ASCII character, as well as ASCII letters, digits, and
+ * underscore.
+ *
+ * Keep this in sync with the definitions of variable name characters in
+ * "src/fe_utils/psqlscan.l", "src/bin/psql/psqlscanslash.l" and
+ * "src/bin/pgbench/exprscan.l".
+ *
+ * Note: this static function is copied from "src/bin/psql/variables.c"
+ */
 static bool
-isLegalVariableName(const char *name)
+valid_variable_name(const char *name)
 {
-	int			i;
+	const unsigned char *ptr = (const unsigned char *) name;
 
-	for (i = 0; name[i] != '\0'; i++)
+	/* Mustn't be zero-length */
+	if (*ptr == '\0')
+		return false;
+
+	while (*ptr)
 	{
-		if (!isalnum((unsigned char) name[i]) && name[i] != '_')
+		if (IS_HIGHBIT_SET(*ptr) ||
+			strchr("ABCDEFGHIJKLMNOPQRSTUVWXYZ" "abcdefghijklmnopqrstuvwxyz"
+   "_0123456789", *ptr) != NULL)
+			ptr++;
+		else
 			return false;
 	}
 
-	return (i > 0);/* must be non-empty */
+	return true;
 }
 
 /*
@@ -1054,7 +1073,7 @@ lookupCreateVariable(CState *st, const char *context, char *name)
 		 * Check for the name only when declaring a new variable to avoid
 		 * overhead.
 		 */
-		if (!isLegalVariableName(name))
+		if (!valid_variable_name(name))
 		{
 			fprintf(stderr, "%s: invalid variable name: \"%s\"\n",
 	context, name);
@@ -1148,7 +1167,7 @@ parseVariable(const char *sql, int *eaten)
 	do
 	{
 		i++;
-	} while (isalnum((unsigned char) sql[i]) || sql[i] == '_');
+	} while (IS_HIGHBIT_SET(sql[i]) || isalnum((unsigned char) sql[i]) || sql[i] == '_');
 	if (i == 1)
 		return NULL;
 

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

Re: [HACKERS] some review comments on logical rep code

At Mon, 17 Apr 2017 18:02:57 +0900, Masahiko Sawada  
wrote in 
> On Fri, Apr 14, 2017 at 4:47 AM, Fujii Masao  wrote:
> > Hi,
> >
> > Though I've read only a part of the logical rep code yet, I'd like to
> > share some (relatively minor) review comments that I got so far.
> 
> It seems nobody is working on dealing with these review comments, so
> I've attached patches. Since there are lots of review comment I
> numbered each review comment. The prefix of patches I attached is
> corresponding to review comment number.
> 
> 1.
> >
> > In ApplyWorkerMain(), DatumGetInt32() should be used to get integer
> > value from the argument, instead of DatumGetObjectId().
> 
> Attached 001 patch fixes it.

Hmm. I looked at the launcher side and found that it assigns bare
integer to bgw_main_arg. It also should use Int32GetDatum.

> logicalrep_worker_launch(Oid dbid, Oid subid, const char *subname, Oid userid,
>  Oid relid)
> {
> int  slot;
...
> for (slot = 0; slot < max_logical_replication_workers; slot++)
...
> bgw.bgw_main_arg = slot;



> 2.
> >
> > No one resets on_commit_launcher_wakeup flag to false.
> 
> Attached 002 patch makes on_commit_launcher_wakeup turn off after woke
> up the launcher process.

It is omitting the abort case. Maybe it should be
AtEOXact_ApplyLauncher(bool commit)?

> 3.
> >
> > ApplyLauncherWakeup() should be static function.
> 
> Attached 003 patch fixes it (and also fixes #5 review comment).
> 
> 4.
> >
> > Seems not only CREATE SUBSCRIPTION but also ALTER SUBSCRIPTION's
> > subcommands (e.g., ENABLED one) should wake the launcher up on commit.
> 
> This is also reported by Horiguchi-san on another thread[1], and patch exists.
> 
> 5.
> >
> > Why is IsBackendPid() check necessary in ApplyLauncherWakeup()?
> 
> I also guess it's necessary. This change is included in #3 patch.

IsBackendPid() is not free, I suppose that just ignoring failure
with ESRCH is enough.

> 6.
> >
> > Normal statements that the walsender for logical rep runs are logged
> > by log_replication_commands. So if log_statement is also set,
> > those commands are logged twice.
> 
> Attached 006 patch fixes it by adding  "-c log_statement = none" to
> connection parameter of libpqrcv if logical = true.

The control by log_replication_commands is performed on
walsender, so this also shuld be done on the same place. Anyway
log_statement is irrelevant to walsender.

> 7.
> >
> > LogicalRepCtx->launcher_pid isn't reset to 0 when the launcher emits
> > an error. The callback function to reset it needs to be registered
> > via on_shmem_exit().
> 
> Attached 007 patch adds callback function to reset 
> LogicalRepCtx->launcher_pid.
> 
> 8.
> >
> > Typo: "an subscriber" should be "a subscriber" in some places.
> 
> It seems that there is no longer these typo.
> 
> 9.
> > /* Get conninfo */
> > datum = SysCacheGetAttr(SUBSCRIPTIONOID,
> > tup,
> > Anum_pg_subscription_subconninfo,
> > &isnull);
> > Assert(!isnull);
> >
> > This assertion makes me think that pg_subscription.subconnifo should
> > have NOT NULL constraint. Also subslotname and subpublications
> > should have NOT NULL constraint because of the same reason.
> 
> Agreed. Attached 009 patch add NOT NULL constraint to all other
> columns of pg_subscription. pg_subscription.subsynccommit should have
> it as well.

I'm not sure the policy here, but I suppose that the assertions
there are still required irrelevantly from the nun-nullness of
the attribute.

> 10.
> >
> > SpinLockAcquire(&MyLogicalRepWorker->relmutex);
> > MyLogicalRepWorker->relstate =
> >   GetSubscriptionRelState(MyLogicalRepWorker->subid,
> > MyLogicalRepWorker->relid,
> > &MyLogicalRepWorker->relstate_lsn,
> > false);
> > SpinLockRelease(&MyLogicalRepWorker->relmutex);
> >
> > Non-"short-term" function like GetSubscriptionRelState() should not
> > be called while spinlock is being held.
> >
> 
> One option is adding new LWLock for the relation state change but this
> lock is used at many locations where the operation actually doesn't
> take time. I think that the discussion would be needed to get
> consensus, so patch for it is not attached.

>From the point of view of time, I agree that it doesn't seem to
harm. Bt I thing it as more significant problem that
potentially-throwable function is called within the mutex
region. It potentially causes a kind of dead lock.  (That said,
theoretically dosn't occur and I'm not sure what happens by the
dead lock..)


> [1] 
> https://www.postgresql.org/message-id/20170406.212441.177025736.horiguchi.kyot...@lab.ntt.co.jp

-- 
Kyotaro Horiguchi
NTT Open Source Software Center



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

Re: [HACKERS] Failed recovery with new faster 2PC code

2017-04-17 Thread Nikhil Sontakke

> >> commit 728bd991c3c4389fb39c45dcb0fe57e4a1dccd71
> >> Author: Simon Riggs 
> >> Date:   Tue Apr 4 15:56:56 2017 -0400
> >>
> >>Speedup 2PC recovery by skipping two phase state files in normal path
> >
> > Thanks Jeff for your tests.
> >
> > So that's now two crash bugs in as many days and lack of clarity about
> > how to fix it.
> >
>

I am trying to reproduce and debug it from my end as well.

Regards,
Nikhils

Re: [HACKERS] Failed recovery with new faster 2PC code

2017-04-17 Thread Stas Kelvich


> On 17 Apr 2017, at 12:14, Simon Riggs  wrote:
> 
> On 15 April 2017 at 23:37, Jeff Janes  wrote:
>> After this commit, I get crash recovery failures when using prepared
>> transactions.
>> 
>> commit 728bd991c3c4389fb39c45dcb0fe57e4a1dccd71
>> Author: Simon Riggs 
>> Date:   Tue Apr 4 15:56:56 2017 -0400
>> 
>>Speedup 2PC recovery by skipping two phase state files in normal path
> 
> Thanks Jeff for your tests.
> 
> So that's now two crash bugs in as many days and lack of clarity about
> how to fix it.
> 
> Stas, I thought this patch was very important to you, yet two releases
> in a row we are too-late-and-buggy.

I’m looking at pgstat issue in nearby thread right now and will switch to this
shortly.

If that’s possible, I’m asking to delay revert for several days.

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] Variable substitution in psql backtick expansion

2017-04-17 Thread Pavel Stehule

2017-04-17 10:58 GMT+02:00 Fabien COELHO :

>
> I don't think so :?xxx is good idea, because for me :xxx is related to
>> content, not to metadata
>>
>
> Hmmm. Indeed it is not. I do not see it as an issue, but it is a good
> point.
>
> Consider perl "defined $x" or "exists $f{k}". $x/$f{k} should be contents,
> but it is not, the dereferencing is suspended by "defined/exists" Yuk, but
> simple and effective.
>
> Also with CPP: "#define x 1, #ifdef x", somehow "x" should be the value,
> not the name, but yet again it is not dereferenced.
>
> Now consider python: "if 'varname' in locals():" at least it is
> consistent, but I cannot say it looks better in the end:-)
>
> So playing around with a value vs metadata is a frequent trick to keep the
> syntax simple, even if the logic is not all there as you point out.
>
> and Robert's tip of using bash syntax is more logical for me (to have
>> syntax for default and custom message).
>>
>
> There is no way to simply test for definition in bash, which is exactly
> what is needed...
>
> A second issue with sh-like proposal is that it needs a boundary thing,
> i.e. bash uses braces ${namevalue}. If it was the beginning of
> psql I would suggest to consider ${name} stuff, but now I'm not sure that
> such a thing can be introduced like ":{xxx}" ? Maybe that can be done.
>
> However it does not change the issue that sh does not allow to test
> whether a variable is defined, which is the thought for feature. Providing
> a default value or erroring out is not the same thing.
>
> Another question to address: how do you handle ' and " escaping? Pg
> :'name' and :"name" solutions are somewhat horrible, but they are there
> which show that it was needed. I'm not sure how to translate that with
> braces in pg. Maybe :{'name'} and :{"name"}? Hmmm...
> Or ":{name}", but then what happens if I write ':{n} x :{m}', should the
> lexer interpolate and escape them inside the strings? That is the sh
> solution, but I'm not sure it should be done now in psql.


I have same thinks. We can disallow nesting - it can be acceptable limit.
The :{xxx:operator} can be used for more things - default, check, user
input, ...

necessary escaping can be done in next line



>
>
> I understand well so it is subjective - and more, don't think so this
>> point is significant.
>>
>
> Well, depending on the syntax things can be done or not, eg test the
> variable definition server-side, not only client side. Hence the
> discussion:-)


It depends if variables are declared or defined by value. In psql there are
defined by value. So some tests if var is defined or not is necessary.


>
>
> We should to have this functionality.
>>
>
> Yes.
>
> --
> Fabien.
>

Re: [HACKERS] Failed recovery with new faster 2PC code

2017-04-17 Thread Simon Riggs

On 15 April 2017 at 23:37, Jeff Janes  wrote:
> After this commit, I get crash recovery failures when using prepared
> transactions.
>
> commit 728bd991c3c4389fb39c45dcb0fe57e4a1dccd71
> Author: Simon Riggs 
> Date:   Tue Apr 4 15:56:56 2017 -0400
>
> Speedup 2PC recovery by skipping two phase state files in normal path

Thanks Jeff for your tests.

So that's now two crash bugs in as many days and lack of clarity about
how to fix it.

Stas, I thought this patch was very important to you, yet two releases
in a row we are too-late-and-buggy.

If anybody has a reason why I shouldn't revert this, please say so now
fairly soon.

 Any further attempts to fix must run Jeff's tests.

-- 
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] pgbench - allow to store select results into variables

2017-04-17 Thread Tatsuo Ishii

Fabien,

> Hello Tatsuo-san,
> 
>> Thank you for the patch. I tested a little bit and found that it does
>> not allow value replacement against non ascii variables in given SQL
>> statements . Is it intentional?
> 
> No, this is a bug.
> 
>> If not, I think you need to fix parseVariable() as well.
> 
> Indeed. Here is v2.

I think you'd better to change the following comments because there's
no psqlscan.l or psqlscanslash.l in pgbench source tree.

+ * underscore.  Keep this in sync with the definition of variable_char in
+ * psqlscan.l and psqlscanslash.l.

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp


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

[HACKERS] CREATE TRIGGER document typo

2017-04-17 Thread Yugo Nagata

Hi,

Attached is a patch fixing simple typos in the CREATE TRIGGER document.

-- 
Yugo Nagata 
diff --git a/doc/src/sgml/ref/create_trigger.sgml b/doc/src/sgml/ref/create_trigger.sgml
index 24195b3..c5f7c75 100644
--- a/doc/src/sgml/ref/create_trigger.sgml
+++ b/doc/src/sgml/ref/create_trigger.sgml
@@ -531,8 +531,8 @@ CREATE TRIGGER view_insert
 
 CREATE TRIGGER transfer_insert
 AFTER INSERT ON transfer
-FOR EACH STATEMENT
 REFERENCING NEW TABLE AS inserted
+FOR EACH STATEMENT
 EXECUTE PROCEDURE check_transfer_balances_to_zero();
 
 
@@ -543,8 +543,8 @@ CREATE TRIGGER transfer_insert
 
 CREATE TRIGGER paired_items_update
 AFTER UPDATE ON paired_items
-FOR EACH ROW
 REFERENCING NEW TABLE AS newtab OLD TABLE AS oldtab
+FOR EACH ROW
 EXECUTE PROCEDURE check_matching_pairs();
 
   

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

Re: [HACKERS] some review comments on logical rep code

On Fri, Apr 14, 2017 at 4:47 AM, Fujii Masao  wrote:
> Hi,
>
> Though I've read only a part of the logical rep code yet, I'd like to
> share some (relatively minor) review comments that I got so far.

It seems nobody is working on dealing with these review comments, so
I've attached patches. Since there are lots of review comment I
numbered each review comment. The prefix of patches I attached is
corresponding to review comment number.

1.
>
> In ApplyWorkerMain(), DatumGetInt32() should be used to get integer
> value from the argument, instead of DatumGetObjectId().

Attached 001 patch fixes it.

2.
>
> No one resets on_commit_launcher_wakeup flag to false.

Attached 002 patch makes on_commit_launcher_wakeup turn off after woke
up the launcher process.

3.
>
> ApplyLauncherWakeup() should be static function.

Attached 003 patch fixes it (and also fixes #5 review comment).

4.
>
> Seems not only CREATE SUBSCRIPTION but also ALTER SUBSCRIPTION's
> subcommands (e.g., ENABLED one) should wake the launcher up on commit.

This is also reported by Horiguchi-san on another thread[1], and patch exists.

5.
>
> Why is IsBackendPid() check necessary in ApplyLauncherWakeup()?

I also guess it's necessary. This change is included in #3 patch.

6.
>
> Normal statements that the walsender for logical rep runs are logged
> by log_replication_commands. So if log_statement is also set,
> those commands are logged twice.

Attached 006 patch fixes it by adding  "-c log_statement = none" to
connection parameter of libpqrcv if logical = true.

7.
>
> LogicalRepCtx->launcher_pid isn't reset to 0 when the launcher emits
> an error. The callback function to reset it needs to be registered
> via on_shmem_exit().

Attached 007 patch adds callback function to reset LogicalRepCtx->launcher_pid.

8.
>
> Typo: "an subscriber" should be "a subscriber" in some places.

It seems that there is no longer these typo.

9.
> /* Get conninfo */
> datum = SysCacheGetAttr(SUBSCRIPTIONOID,
> tup,
> Anum_pg_subscription_subconninfo,
> &isnull);
> Assert(!isnull);
>
> This assertion makes me think that pg_subscription.subconnifo should
> have NOT NULL constraint. Also subslotname and subpublications
> should have NOT NULL constraint because of the same reason.

Agreed. Attached 009 patch add NOT NULL constraint to all other
columns of pg_subscription. pg_subscription.subsynccommit should have
it as well.

10.
>
> SpinLockAcquire(&MyLogicalRepWorker->relmutex);
> MyLogicalRepWorker->relstate =
>   GetSubscriptionRelState(MyLogicalRepWorker->subid,
> MyLogicalRepWorker->relid,
> &MyLogicalRepWorker->relstate_lsn,
> false);
> SpinLockRelease(&MyLogicalRepWorker->relmutex);
>
> Non-"short-term" function like GetSubscriptionRelState() should not
> be called while spinlock is being held.
>

One option is adding new LWLock for the relation state change but this
lock is used at many locations where the operation actually doesn't
take time. I think that the discussion would be needed to get
consensus, so patch for it is not attached.

[1] 
https://www.postgresql.org/message-id/20170406.212441.177025736.horiguchi.kyot...@lab.ntt.co.jp

Regards,

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

001_change_DatumGetObjectId_to_DatumGetInt32.patch
Description: Binary data

002_Reset_on_commit_launcher_wakeup.patch
Description: Binary data

003_Fix_ApplyLancherWakeUp_function.patch
Description: Binary data

006_Prevent_to_emit_statement_log_double.patch
Description: Binary data

007_Regiter_on_shmem_exit_for_launcher.patch
Description: Binary data

009_Add_not_null_constraint.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] Variable substitution in psql backtick expansion



I don't think so :?xxx is good idea, because for me :xxx is related to 
content, not to metadata


Hmmm. Indeed it is not. I do not see it as an issue, but it is a good 
point.


Consider perl "defined $x" or "exists $f{k}". $x/$f{k} should be contents, 
but it is not, the dereferencing is suspended by "defined/exists" Yuk, 
but simple and effective.


Also with CPP: "#define x 1, #ifdef x", somehow "x" should be the value, 
not the name, but yet again it is not dereferenced.


Now consider python: "if 'varname' in locals():" at least it is 
consistent, but I cannot say it looks better in the end:-)


So playing around with a value vs metadata is a frequent trick to keep the 
syntax simple, even if the logic is not all there as you point out.


and Robert's tip of using bash syntax is more logical for me (to have 
syntax for default and custom message).


There is no way to simply test for definition in bash, which is exactly 
what is needed...


A second issue with sh-like proposal is that it needs a boundary thing, 
i.e. bash uses braces ${namevalue}. If it was the beginning of 
psql I would suggest to consider ${name} stuff, but now I'm not sure that 
such a thing can be introduced like ":{xxx}" ? Maybe that can be done.


However it does not change the issue that sh does not allow to test 
whether a variable is defined, which is the thought for feature. Providing 
a default value or erroring out is not the same thing.


Another question to address: how do you handle ' and " escaping? Pg 
:'name' and :"name" solutions are somewhat horrible, but they are there 
which show that it was needed. I'm not sure how to translate that with 
braces in pg. Maybe :{'name'} and :{"name"}? Hmmm...
Or ":{name}", but then what happens if I write ':{n} x :{m}', should the 
lexer interpolate and escape them inside the strings? That is the sh 
solution, but I'm not sure it should be done now in psql.


I understand well so it is subjective - and more, don't think so this 
point is significant.


Well, depending on the syntax things can be done or not, eg test the 
variable definition server-side, not only client side. Hence the 
discussion:-)



We should to have this functionality.


Yes.

--
Fabien.


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

2017-04-17 Thread Yugo Nagata

On Fri, 14 Apr 2017 09:05:14 -0400
Robert Haas  wrote:

> On Fri, Apr 14, 2017 at 4:23 AM, Yugo Nagata  wrote:
> > On Thu, 13 Apr 2017 16:40:29 -0400
> > Robert Haas  wrote:
> >> On Fri, Mar 17, 2017 at 7:57 AM, Yugo Nagata  wrote:
> >> > I also understanded that my design has a problem during pg_dump and
> >> > pg_upgrade, and that some information to identify the partition
> >> > is required not depending the command order. However, I feel that
> >> > Amul's design is a bit complicated with the rule to specify modulus.
> >> >
> >> > I think we can use simpler syntax, for example, as below.
> >> >
> >> >  CREATE TABLE h1 PARTITION OF h FOR (0);
> >> >  CREATE TABLE h2 PARTITION OF h FOR (1);
> >> >  CREATE TABLE h3 PARTITION OF h FOR (2);
> >>
> >> I don't see how that can possibly work.  Until you see all the table
> >> partitions, you don't know what the partitioning constraint for any
> >> given partition should be, which seems to me to be a fatal problem.
> >
> > If a partition has an id, the partitioning constraint can be written as
> >
> >  hash_func(hash_key) % N = id
> >
> > wehre N is the number of paritions. Doesn't it work?
> 
> Only if you know the number of partitions.  But with your syntax,
> after seeing only the first of the CREATE TABLE .. PARTITION OF
> commands, what should the partition constraint be?  It depends on how
> many more such commands appear later in the dump file, which you do
> not know at that point.

I thought that the partition constraint could be decided every
time a new partition is created or attached, and that it woule be
needed to relocate records automatically when the partition configuration
changes. However, I have come to think that the automatic relocation
might not be needed at this point.

> 
> >> I agree that Amul's syntax - really, I proposed it to him - is not the
> >> simplest, but I think all the details needed to reconstruct the
> >> partitioning constraint need to be explicit.  Otherwise, I'm pretty
> >> sure things we're going to have lots of problems that we can't really
> >> solve cleanly.  We can later invent convenience syntax that makes
> >> common configurations easier to set up, but we should invent the
> >> syntax that spells out all the details first.
> >
> > I have a question about Amul's syntax. After we create partitions
> > as followings,
> >
> >  create table foo (a integer, b text) partition by hash (a);
> >  create table foo1 partition of foo with (modulus 2, remainder 0);
> >  create table foo2 partition of foo with (modulus 2, remainder 1);
> >
> > we cannot create any additional partitions for the partition.
> >
> > Then, after inserting records into foo1 and foo2, how we can
> > increase the number of partitions?
> 
> You can detach foo1, create two new partitions with modulus 4 and
> remainders 0 and 2, and move the data over from the old partition.
> 
> I realize that's not as automated as you might like, but it's no worse
> than what is currently required for list and range partitioning when
> you split a partition.  Someday we might build in tools to do that
> kind of data migration automatically, but right now we have none.

Thanks. I understood it. The automatic data migration feature 
would be better to be implemented separately.

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


-- 
Yugo Nagata 


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

Re: [HACKERS] Extracting GiST index structure stats?

2017-04-17 Thread Arthur Zakirov


Hello,

On 15.04.2017 20:38, Thomas Mercieca wrote:
Hi all, I am interested in getting simple statistics of a GiST index 
structure. For example, heightof tree.



It seems that the other indexes have a metapage for this. I am still 
unsure but it looks to me like the GiST access method internal does not 
work exactly in this way so it is a bit more complicated to extract this 
data. Is this correct? If I wanted to extract this kind of stat (back to 
the height of tree example) what would be the right approach? Thanks a lot.





Have you tried the 'gevel' module? You can find it here:
http://www.sai.msu.su/~megera/wiki/Gevel

--
Arthur Zakirov
Postgres Professional: http://www.postgrespro.com
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] Optimization for updating foreign tables in Postgres FDW