Re: [HACKERS] [BUG] pg_basebackup from disconnected standby fails

[Kyotaro HORIGUCHI]

Sorry for the absense. Thank you for registering it.

regards.

At Fri, 24 Jun 2016 14:46:25 +0900, Michael Paquier  
wrote in 
> On Thu, Jun 23, 2016 at 3:51 PM, Michael Paquier
>  wrote:
> > By the way, do you mind if I add this patch to the next CF? Better not
> > to lose track of it...
> 
> Well, I have added an entry here at the end:
> https://commitfest.postgresql.org/10/654/
> Better doing it now before I forget about it...
> -- 
> Michael

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

[HACKERS] Re: GiST optimizing memmoves in gistplacetopage for fixed-size updates [PoC]

[Andrew Borodin]

Hi!
>I think you should implement PageReplaceItem() version and add it to the 
>commitfest.
Here is the patch.
I've called function PageIndexTupleOverwrite() because it's suitable
only for indices. It works on my tests and performance is the same as
in proof-of-concept (despite some sanity checks copied from
PageIndexTupleDelete).
Next weekend I'll do more testing, and then add it to commitfest.

Best regards, Andrey Borodin, Octonica & Ural Federal University.

2016-07-03 15:21 GMT+05:00 Alexander Korotkov :
> Hi!
>
> On Sun, Jul 3, 2016 at 12:24 PM, Andrew Borodin 
> wrote:
>>
>> I think there is some room for improving GiST inserts. Following is
>> the description of what I think is performance problem.
>>
>> Function gistplacetopage in file /src/backend/access/gist/gist.c is
>> responsible for updating or appending new tuple on GiST page.
>> Currently, after checking necessity of page split due to overflow, it
>> essentially executes following:
>>
>>if (OffsetNumberIsValid(oldoffnum))
>>PageIndexTupleDelete(page, oldoffnum);
>>gistfillbuffer(page, itup, ntup, InvalidOffsetNumber);
>>
>> That is: remove old tuple if it’s there, then place updated tuple at the
>> end.
>>
>> Half of the old data have to be shifted my memmove inside
>> PageIndexTupleDelete() call, half of the linp-s have to be corrected.
>>
>> If the updated tuple has same size as already residing on page we can
>> just overwrite it. Attached patch demonstrates that concept. Attached
>> test.sql inserts million rows into GiST index based on cube extension.
>> My machine is Hyper-V VM running Ubuntu on i5-2500 CPU with SSD
>> storage. Before patch, insert part of test is executed on average
>> within 159 second, after patch application: insert part is executed
>> within 77 seconds on average. That is almost twice faster (for
>> CPU\Mem-bounded inserts, disk-constrained test will show no
>> improvement). But it works only for fixed-size tuple inserts.
>
>
> Very promising results!
>
>> I know that code in patch is far from beautiful: it operates with
>> three different levels of abstraction within 5 lines of code. Those
>> are low level memmove(), system-wide PageAddItem() and GiST private
>> gistfillBuffer().
>>
>> By the way PageAddItem() have overwrite flag, but it only works with
>> unused ItemId’s. Marking old ItemId as unused before PageAddItem()
>> didn’t work for me. Unfortunately bufpage.c routines do not contain
>> one for updating(replacing with new) tuple on page. It is important
>> for me because I’m working on advanced GiST page layout (
>>
>> https://www.postgresql.org/message-id/CAJEAwVE0rrr%2BOBT-P0gDCtXbVDkBBG_WcXwCBK%3DGHo4fewu3Yg%40mail.gmail.com
>> ), current approach is to use skip-tuples which can be used to skip
>> many flowing tuples with one key check. Obviously, this design cares
>> about tuples order. And update in a fashion “place updated tuple at
>> the end” won’t work for me.
>>
>> So, I think it would be better to implement PageReplaceItem()
>> functionality to make code better, to make existing GiST inserts
>> faster and to enable new advanced page layouts in indices.
>
>
> +1 for PageReplaceItem()
> Even if item is not the same size, we can move the tail of page once instead
> of twice.
> I think you should implement PageReplaceItem() version and add it to the
> commitfest.
>
> --
> Alexander Korotkov
> Postgres Professional: http://www.postgrespro.com
> The Russian Postgres Company


PageIndexTupleOverwrite.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] IPv6 link-local addresses and init data type

[Haribabu Kommi]

On Wed, Jun 29, 2016 at 6:31 AM, Markus Wanner  wrote:
>> I added another character array of 256 member into inet_struct as a last 
>> member
>> to store the zone id.
>
> I haven't looked at the patch in detail, but zeroing or memcpy'ing those
> 256 bytes seems like overkill to me. I'd recommend to limit this to only
> allocate and move around as many bytes as needed for the scope id.

Currently a static structure members are passed to the inet_net_pton function
to fill the data, to make it simple i added a member to store the zoneid in the
structure.

>> Currently all the printable characters are treated as zone id's. I
>> will restrict this
>> to only alphabets and numbers.
>
> I fear alphanumeric only is too restrictive. RFC 4007 only specifies
> that the zone id "must not conflict with the delimiter character" and
> leaves everything beyond that to the implementation (which seems too
> loose, printable characters sounds about right to me...).

Ok. Currently the patch supports all printable characters, except '/' that is
reserved for specifying the bits.

>> How about the following case, Do we treat them as same or different?
>>
>> select 'fe80::%eth1'::inet = 'fe80::%ETH1'::inet;
>
> Let's be consistent in not interpreting the scope id in any way, meaning
> those would be different. (After all, interfaces names seem to be case
> sensitive - on my variant of Linux at the very least - i.e. ETH1 cannot
> be found, while eth1 can be.)

Added a case sensitive comparison for zoneid.

>> fe80::%2/64 is only treated as the valid address but not other way as
>> fe80::/64%2.
>> Do we need to throw an error in this case or just ignore.
>
> I didn't find any evidence for the second case being invalid; nor for it
> being valid.

Yes, that's correct. Second one invalid. Patch throws an error.


>I'm starting to question if it's really wise to add the scope id to the
>INET6 type...

I didn't find any major database that support inet datatype, which is good
for storing IP address. I feel it may be better to extend it to support new
IP enhancements.

Updated patch is attached. Docs and tests are pending.

Regards,
Hari Babu
Fujitsu Australia


ipv6_scopeid_1.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] Forthcoming SQL standards about JSON and Multi-Dimensional Arrays (FYI)

[Thomas Munro]

On Wed, Jun 29, 2016 at 11:51 AM, Stefan Keller  wrote:
> Hi,
>
> FYI: I'd just like to point you to following two forthcoming standard
> parts from "ISO/IEC JTS 1/SC 32" comittee: one on JSON, and one on
> "Multi-Dimensional Arrays" (SQL/MDA).
>
> They define there some things different as already in PG. See also
> Peter Baumann's slides [1] and e.g. [2]
>
> :Stefan
>
> [1] 
> https://www.unibw.de/inf4/professors/geoinformatics/agile-2016-workshop-gis-with-nosql
> [2] 
> http://jtc1sc32.org/doc/N2501-2550/32N2528-WG3-Tutorial-Opening-Plenary.pdf

Thanks for these pointers.  On the "standards under development"
page[1], I see that ISO/IEC PDTR 19075-6 (SQL/JSON) is at stage 30.60
"Close of voting/ comment period".  But ISO/IEC CD 9075-15
(Multi-Dimensional Arrays) is in stage 30.92 "CD referred back to
Working Group".  Is that how they say "returned with feedback"?
ISO/IEC PDTR 19075-5 (Row Pattern Recognition) has also reached stage
30.60.  Does anyone know what that one is about?  Maybe something like
MATCH_RECOGNIZE in Oracle?

[1] 
http://www.iso.org/iso/home/store/catalogue_tc/catalogue_tc_browse.htm?commid=45342=on

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


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

Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol

[Michael Paquier]

On Mon, Jul 4, 2016 at 6:34 AM, Peter Eisentraut
 wrote:
> On 7/2/16 3:54 PM, Heikki Linnakangas wrote:
>>
>> In related news, RFC 7677 that describes a new SCRAM-SHA-256
>> authentication mechanism, was published in November 2015. It's identical
>> to SCRAM-SHA-1, which is what this patch set implements, except that
>> SHA-1 has been replaced with SHA-256. Perhaps we should forget about
>> SCRAM-SHA-1 and jump straight to SCRAM-SHA-256.
>
> I think a global change from SHA-1 to SHA-256 is in the air already, so if
> we're going to release something brand new in 2017 or so, it should be
> SHA-256.
>
> I suspect this would be a relatively simple change, so I wouldn't mind
> seeing a SHA-1-based variant in CF1 to get things rolling.

I'd just move this thing to SHA256, we are likely going to use that at the end.

As I am coming back into that, I would as well suggest do the
following, that the current set of patches is clearly missing:
- Put the HMAC infrastructure stuff of pgcrypto into src/common/. It
is a bit a shame to not reuse what is currently available, then I
would suggest to reuse that with HMAC_SCRAM_SHAXXX as label.
- Move *all* the SHA-related things of pgcrypto to src/common,
including SHA1, SHA224 and SHA256. px_memset is a simple wrapper on
top of memset, we should clean up that first.
Any other things to consider that I am forgetting?
-- 
Michael


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

Re: [HACKERS] to_date_valid()

[Pavel Stehule]

2016-07-04 5:19 GMT+02:00 Pavel Stehule :

>
>
> 2016-07-04 4:25 GMT+02:00 Craig Ringer :
>
>> On 3 July 2016 at 09:32, Euler Taveira  wrote:
>>
>>> On 02-07-2016 22:04, Andreas 'ads' Scherbaum wrote:
>>> > The attached patch adds a new function "to_date_valid()" which will
>>> > validate the date and return an error if the input and output date do
>>> > not match. Tests included, documentation update as well.
>>> >
>>> Why don't you add a third parameter (say, validate = true | false)
>>> instead of creating another function? The new parameter could default to
>>> false to not break compatibility.
>>>
>>
>> because
>>
>>
>>SELECT to_date('blah', 'pattern', true)
>>
>> is less clear to read than
>>
>>SELECT to_date_valid('blah', 'pattern')
>>
>> and offers no advantage. It's likely faster to use a separate function
>> too.
>>
>
> personally I prefer first variant - this is same function with stronger
> check.
>

Currently probably we have not two similar function - one  fault tolerant
and second stricter. There is only one example of similar behave -
parse_ident with "strict" option.

The three parameters are ok still - so I don't see a reason why we have to
implement new function. If you need to emphasize the fact so behave should
be strict, you can use named parameters

select to_date('blah', 'patter', strict => true)

Regards

Pavel



>
> The name to_date_valid sounds little bit strange - maybe to_date_strict
> should be better.
>
> Regards
>
> Pavel
>
>
>>
>> --
>>  Craig Ringer   http://www.2ndQuadrant.com/
>>  PostgreSQL Development, 24x7 Support, Training & Services
>>
>
>

Re: [HACKERS] primary_conninfo missing from pg_stat_wal_receiver

[Michael Paquier]

On Sat, Jul 2, 2016 at 2:56 AM, Alvaro Herrera  wrote:
> Michael Paquier wrote:
>> On Fri, Jul 1, 2016 at 8:50 AM, Michael Paquier
>>  wrote:
>
>> >> Okay, that argument I buy.
>> >>
>> >> I suppose this function/view should report no row at all if there is no
>> >> wal receiver connected, rather than a view with nulls.
>> >
>> > The function returns PG_RETURN_NULL() so as we don't have to use a
>> > SRF, and the view checks for IS NOT NULL, so there would be no rows
>> > popping up.
>>
>> In short, I would just go with the attached and call it a day.
>
> Done, thanks.

Thanks. I have noticed that the item was still in CLOSE_WAIT, so I
have moved it to the section of resolved items.
-- 
Michael


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

Re: [HACKERS] to_date_valid()

[Gavin Flower]

On 04/07/16 15:19, Pavel Stehule wrote:



2016-07-04 4:25 GMT+02:00 Craig Ringer >:


On 3 July 2016 at 09:32, Euler Taveira > wrote:

On 02-07-2016 22 :04, Andreas 'ads'
Scherbaum wrote:
> The attached patch adds a new function "to_date_valid()"
which will
> validate the date and return an error if the input and
output date do
> not match. Tests included, documentation update as well.
>
Why don't you add a third parameter (say, validate = true | false)
instead of creating another function? The new parameter could
default to
false to not break compatibility.


because


   SELECT to_date('blah', 'pattern', true)

is less clear to read than

   SELECT to_date_valid('blah', 'pattern')

and offers no advantage. It's likely faster to use a separate
function too.


personally I prefer first variant - this is same function with 
stronger check.


The name to_date_valid sounds little bit strange - maybe 
to_date_strict should be better.


Regards

Pavel

-- 
 Craig Ringer http://www.2ndQuadrant.com/

 PostgreSQL Development, 24x7 Support, Training & Services



Yeah, my feeling too, is that 'to_date_strict' would be better!


Cheers,
Gavin



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

Re: [HACKERS] to_date_valid()

[Pavel Stehule]

2016-07-04 4:25 GMT+02:00 Craig Ringer :

> On 3 July 2016 at 09:32, Euler Taveira  wrote:
>
>> On 02-07-2016 22:04, Andreas 'ads' Scherbaum wrote:
>> > The attached patch adds a new function "to_date_valid()" which will
>> > validate the date and return an error if the input and output date do
>> > not match. Tests included, documentation update as well.
>> >
>> Why don't you add a third parameter (say, validate = true | false)
>> instead of creating another function? The new parameter could default to
>> false to not break compatibility.
>>
>
> because
>
>
>SELECT to_date('blah', 'pattern', true)
>
> is less clear to read than
>
>SELECT to_date_valid('blah', 'pattern')
>
> and offers no advantage. It's likely faster to use a separate function too.
>

personally I prefer first variant - this is same function with stronger
check.

The name to_date_valid sounds little bit strange - maybe to_date_strict
should be better.

Regards

Pavel


>
> --
>  Craig Ringer   http://www.2ndQuadrant.com/
>  PostgreSQL Development, 24x7 Support, Training & Services
>

Re: [HACKERS] Parallel appendrel scans?

[Robert Haas]

On Jul 3, 2016, at 6:58 PM, Tom Lane  wrote:
> It looks to me like set_append_rel_pathlist builds a partial path for
> an appendrel only if there exist partial paths for all members of the
> appendrel.  I don't understand this restriction.  Why could you not
> build it from a regular path for each appendrel child?  You'd have
> to farm out entire childrels to workers, which might be larger chunks
> of work than one could wish, but it seems better than failing to
> parallelize at all.

Absolutely.  The problem is merely that we need some executor support that has 
not been written yet. See discussions of Parallel Append in the archives.

...Robert

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

Re: [HACKERS] to_date_valid()

[Craig Ringer]

On 3 July 2016 at 09:32, Euler Taveira  wrote:

> On 02-07-2016 22:04, Andreas 'ads' Scherbaum wrote:
> > The attached patch adds a new function "to_date_valid()" which will
> > validate the date and return an error if the input and output date do
> > not match. Tests included, documentation update as well.
> >
> Why don't you add a third parameter (say, validate = true | false)
> instead of creating another function? The new parameter could default to
> false to not break compatibility.
>

because


   SELECT to_date('blah', 'pattern', true)

is less clear to read than

   SELECT to_date_valid('blah', 'pattern')

and offers no advantage. It's likely faster to use a separate function too.

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

Re: [HACKERS] Docs, backups, and MS VSS

[Craig Ringer]

On 2 July 2016 at 22:31, Craig Ringer  wrote:

>
> - Microsoft VSS is NOT safe, as it fails point 2. It is atomic only on a
> per-file level. You MUST use pg_start_backup() and pg_stop_backup() with
> WAL archiving or automated copy of the extra WAL if you use MS VSS. Most
> Windows backup products use MS VSS internally. You must ensure they have
> dedicated PostgreSQL backup support, using pg_basebackup,
> pg_dump/pg_restore, or pg_start_backup()/pg_stop_backup().
>

So, I read the manual [1] to confirm this, and I was wrong. VSS should be
fine if used correctly by the backup application, with a single snapshot
used for the entire PostgreSQL data directory. It is in fact block-level
CoW like LVM, or at least the default provider is.

It turns out that on Windows we could actually support safe snapshot based
backups across multiple volumes, too, by shipping a VSS Writer for
PostgreSQL that notifies PostgreSQL to perform a checkpoint and block
future XLogInsert()s until released, so we don't need a fully atomic
snapshot anymore. However, this would only work if the backup application
that the main datadir, xlog and any tablespace volumes be included in a
single VSS snapshot.

In the absence of a VSS Writer for PostgreSQL(provided by the backup app or
PostgreSQL install) and confirmation that the backup app snapshots all
volumes together, then like LVM it'll only be safe if the backup app does
pg_start_backup(), copies, pg_stop_backup() and copies the extra WAL. We
should probably provide a tool to make this easier by listing all the WAL
file names for a range of LSNs.

Anyway - still worth documenting safety requirements, but contrary to what
I remembered VSS looks to be fine.

It looks like a PostgreSQL VSS Writer could make the process a lot
smoother, though, by enumerating all volumes PostgreSQL uses to make sure
they get snapshotted, trying to flush in-progress xacts and pause new ones,
etc.


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

[HACKERS] Parallel appendrel scans?

[Tom Lane]

It looks to me like set_append_rel_pathlist builds a partial path for
an appendrel only if there exist partial paths for all members of the
appendrel.  I don't understand this restriction.  Why could you not
build it from a regular path for each appendrel child?  You'd have
to farm out entire childrels to workers, which might be larger chunks
of work than one could wish, but it seems better than failing to
parallelize at all.

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] fixing subplan/subquery confusion

[Tom Lane]

I wrote:
> Robert Haas  writes:
>> On Sun, Jul 3, 2016 at 1:14 PM, Tom Lane  wrote:
>>> You mentioned that you'll be on vacation for much of July.  If you like,
>>> I will take this open item off your hands, since I'll be around and can
>>> deal with any bugs that pop up in it.

>> That would be much appreciated.

> OK, will do.

I've reviewed, adjusted, and pushed this patch, and accordingly am
marking the open item closed.  (Any remaining bugs should become
new open items.)

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] Password identifiers, protocol aging and SCRAM protocol

[Peter Eisentraut]

On 7/2/16 3:54 PM, Heikki Linnakangas wrote:

In related news, RFC 7677 that describes a new SCRAM-SHA-256
authentication mechanism, was published in November 2015. It's identical
to SCRAM-SHA-1, which is what this patch set implements, except that
SHA-1 has been replaced with SHA-256. Perhaps we should forget about
SCRAM-SHA-1 and jump straight to SCRAM-SHA-256.


I think a global change from SHA-1 to SHA-256 is in the air already, so 
if we're going to release something brand new in 2017 or so, it should 
be SHA-256.


I suspect this would be a relatively simple change, so I wouldn't mind 
seeing a SHA-1-based variant in CF1 to get things rolling.


--
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] fixing subplan/subquery confusion

[Tom Lane]

Robert Haas  writes:
> On Sun, Jul 3, 2016 at 1:14 PM, Tom Lane  wrote:
>> I think a cleaner way is to have set_append_rel_size() invoke
>> set_rel_consider_parallel() on the child rels and then propagate their
>> parallel-unsafety up to the parent.  That seems fairly analogous to
>> the way we already deal with their sizes: in particular, set_rel_size
>> is invoked on an appendrel child from there, not from an extra pass in
>> set_base_rel_sizes.  Then set_append_rel_pathlist can propagate unsafety
>> down again, as you've done here.

> I was reluctant to do it that way because of the relatively stupid way
> that we have to go about finding the inheritance children to visit,
> but maybe I shouldn't let that dominate my thinking.  We could always
> replace that with some more efficient data structure at some point
> down the road.

That might be a reasonable objection if we needed to add a third such
loop, but I'm talking about putting the set_rel_consider_parallel call in
the loop that already exists.  So there shouldn't be any added overhead.

(It's definitely true that we could improve on the append_rel_list-based
search mechanism; but so far I have never seen any indication that that
was a bottleneck, so it's pretty far down my to-do list.)

>> You mentioned that you'll be on vacation for much of July.  If you like,
>> I will take this open item off your hands, since I'll be around and can
>> deal with any bugs that pop up in it.

> That would be much appreciated.

OK, will do.

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] fixing subplan/subquery confusion

[Robert Haas]

On Sun, Jul 3, 2016 at 1:14 PM, Tom Lane  wrote:
> BTW, looking elsewhere in set_rel_consider_parallel, isn't there an extra
> "return;" in the tablesample stanza, allpaths.c:541 as of HEAD?  Looks to
> me like we're failing to ever treat tablesampling as parallel-safe.
> I'm rather dubious about whether any of our tablesample methods actually
> are parallel-safe, but that should be down to the method to say.  If this
> code's been like this all along, we've never tested them.

You are correct.

>> I think it's hard to avoid the conclusion that
>> set_rel_consider_parallel() needs to run on other member rels as well
>> as baserels.  We might be able to do that only for cases where the
>> parent is a subquery RTE, since if the parent is a baserel then I
>> think we must have just a standard inheritance hierarchy and things
>> might be OK, but even then, I fear there might be problems with RLS.
>> Anyway, the attached patch solves the problem in a fairly "brute
>> force" fashion.
>
> I agree, and I also agree that this way is pretty brute-force.
> I think a cleaner way is to have set_append_rel_size() invoke
> set_rel_consider_parallel() on the child rels and then propagate their
> parallel-unsafety up to the parent.  That seems fairly analogous to
> the way we already deal with their sizes: in particular, set_rel_size
> is invoked on an appendrel child from there, not from an extra pass in
> set_base_rel_sizes.  Then set_append_rel_pathlist can propagate unsafety
> down again, as you've done here.

I was reluctant to do it that way because of the relatively stupid way
that we have to go about finding the inheritance children to visit,
but maybe I shouldn't let that dominate my thinking.  We could always
replace that with some more efficient data structure at some point
down the road.

>> (Official status update: I'm not prepared to commit this without some
>> review.  So I intend to wait for a while and see whether I get some
>> review.  I realize these status updates are supposed to contain a date
>> by which further action will be taken, but I don't know how to
>> meaningfully do that in this type of case.)
>
> You mentioned that you'll be on vacation for much of July.  If you like,
> I will take this open item off your hands, since I'll be around and can
> deal with any bugs that pop up in it.

That would be much appreciated.

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


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

[HACKERS] INSERT .. SET syntax

[Marko Tiikkaja]

Hi,

Here's a patch for $SUBJECT.  I'll probably work on the docs a bit more 
before the next CF, but I thought I'd post it anyway.



.m
diff --git a/doc/src/sgml/ref/insert.sgml b/doc/src/sgml/ref/insert.sgml
index e710cf4..33e577b 100644
--- a/doc/src/sgml/ref/insert.sgml
+++ b/doc/src/sgml/ref/insert.sgml
@@ -22,12 +22,21 @@ PostgreSQL documentation
  
 
 [ WITH [ RECURSIVE ] with_query 
[, ...] ]
-INSERT INTO table_name [ AS 
alias ] [ ( column_name [, ...] ) ]
-{ DEFAULT VALUES | VALUES ( { expression | DEFAULT } [, ...] ) [, ...] | 
query }
+INSERT INTO table_name [ AS 
alias ]
+{
+[ column_list ] VALUES ( { expression | DEFAULT } [, ...] ) [, ...] |
+[ column_list ] query |
+DEFAULT VALUES |
+SET column_name = { 
expression | DEFAULT } [, ...]
+}
 [ ON CONFLICT [ conflict_target ] conflict_action ]
 [ RETURNING * | output_expression [ [ AS ] output_name ] [, ...] ]
 
-where conflict_target can 
be one of:
+where column_list 
is:
+
+( column_name [, ...] )
+
+and conflict_target can 
be one of:
 
 ( { index_column_name | ( 
index_expression ) } [ COLLATE 
collation ] [ opclass ] [, ...] ) [ WHERE index_predicate ]
 ON CONSTRAINT constraint_name
@@ -53,13 +62,26 @@ INSERT INTO table_name [ AS 
 
   
-   The target column names can be listed in any order.  If no list of
-   column names is given at all, the default is all the columns of the
-   table in their declared order; or the first N column
-   names, if there are only N columns supplied by the
-   VALUES clause or query.  The values
-   supplied by the VALUES clause or query are
-   associated with the explicit or implicit column list left-to-right.
+   The target column names in a column_list can be
+   listed in any order.  If no column_list is given at
+   all (and the SET syntax is not used), the default is all
+   the columns of the table in their declared order; or the first
+   N column names, if there are only N
+   columns supplied by the VALUES clause or
+   query.  The values supplied by the VALUES
+   clause or query are associated with the explicit or
+   implicit column list left-to-right.
+  
+
+  
+Instead of a column_list and a VALUES
+clause, a SET clause similar to that of an
+UPDATE can be used instead.  The advantage of the
+SET clause is that instead of matching the elements in
+the two lists by ordinal position, the column name and the
+expression to assign to that column are visually next to each other.
+This can make long column assignment lists significantly more
+readable.
   
 
   
@@ -691,13 +713,13 @@ INSERT INTO distributors (did, dname) VALUES (10, 'Conrad 
International')
   
INSERT conforms to the SQL standard, except that
the RETURNING clause is a
-   PostgreSQL extension, as is the ability
-   to use WITH with INSERT, and the ability to
-   specify an alternative action with ON CONFLICT.
-   Also, the case in
-   which a column name list is omitted, but not all the columns are
-   filled from the VALUES clause or query,
-   is disallowed by the standard.
+   PostgreSQL extension, as is the
+   SET clause when used instead of a VALUES clause, the
+   ability to use WITH with INSERT, and the
+   ability to specify an alternative action with ON
+   CONFLICT.  Also, the case in which a column name list is omitted,
+   but not all the columns are filled from the VALUES clause
+   or query, is disallowed by the standard.
   
 
   
diff --git a/src/backend/parser/analyze.c b/src/backend/parser/analyze.c
index 29c8c4e..55c4cb3 100644
--- a/src/backend/parser/analyze.c
+++ b/src/backend/parser/analyze.c
@@ -466,8 +466,9 @@ transformInsertStmt(ParseState *pstate, InsertStmt *stmt)
stmt->onConflictClause->action 
== ONCONFLICT_UPDATE);
 
/*
-* We have three cases to deal with: DEFAULT VALUES (selectStmt == 
NULL),
-* VALUES list, or general SELECT input.  We special-case VALUES, both 
for
+* We have four cases to deal with: DEFAULT VALUES (selectStmt == NULL 
and
+* cols == NIL), SET syntax (selectStmt == NULL but cols != NIL), VALUES
+* list, or general SELECT input.  We special-case VALUES, both for
 * efficiency and so we can handle DEFAULT specifications.
 *
 * The grammar allows attaching ORDER BY, LIMIT, FOR UPDATE, or WITH to 
a
@@ -522,7 +523,7 @@ transformInsertStmt(ParseState *pstate, InsertStmt *stmt)
/*
 * Determine which variant of INSERT we have.
 */
-   if (selectStmt == NULL)
+   if (selectStmt == NULL && stmt->cols == NIL)
{
/*
 * We have INSERT ... DEFAULT VALUES.  We can handle this case 
by
@@ -531,6 +532,25 @@ transformInsertStmt(ParseState *pstate, InsertStmt *stmt)
 */
exprList = NIL;
}
+   else if (selectStmt == NULL)
+   {
+   /*
+  

Re: [HACKERS] fixing subplan/subquery confusion

[Tom Lane]

Robert Haas  writes:
> I dug into this a bit and found more problems.  I wondered why Tom's
> patch did this:

> !   if (has_parallel_hazard((Node *) rte->subquery, 
> false))
> !   return;
> !   break;

> Instead of just doing this:

> -return;
> +break;

> After all, the code that built subquery paths ought to be sufficient
> to find any parallel hazards during subquery planning; there seems to
> be no especially-good reason why we should walk the whole query tree
> searching for the again.

Yeah, I think you are right.  I modeled my patch on the nearby handling of
function RTEs, wherein function_rte_parallel_ok checks the RTE contents
for safety.  But we must do that because create_functionscan_path relies
entirely on the rel's consider_parallel flag.  In contrast,
create_subqueryscan_path looks at both rel->consider_parallel and
subpath->parallel_safe, so we can rely on the subpath(s)' markings as an
indication of the subquery's safety.  (rel->consider_parallel then tells
us just about the restriction and projection expressions on the subquery
rel itself, and we'll end up with the right choice if those are unsafe.)

BTW, looking elsewhere in set_rel_consider_parallel, isn't there an extra
"return;" in the tablesample stanza, allpaths.c:541 as of HEAD?  Looks to
me like we're failing to ever treat tablesampling as parallel-safe.
I'm rather dubious about whether any of our tablesample methods actually
are parallel-safe, but that should be down to the method to say.  If this
code's been like this all along, we've never tested them.

> I think it's hard to avoid the conclusion that
> set_rel_consider_parallel() needs to run on other member rels as well
> as baserels.  We might be able to do that only for cases where the
> parent is a subquery RTE, since if the parent is a baserel then I
> think we must have just a standard inheritance hierarchy and things
> might be OK, but even then, I fear there might be problems with RLS.
> Anyway, the attached patch solves the problem in a fairly "brute
> force" fashion.

I agree, and I also agree that this way is pretty brute-force.
I think a cleaner way is to have set_append_rel_size() invoke
set_rel_consider_parallel() on the child rels and then propagate their
parallel-unsafety up to the parent.  That seems fairly analogous to
the way we already deal with their sizes: in particular, set_rel_size
is invoked on an appendrel child from there, not from an extra pass in
set_base_rel_sizes.  Then set_append_rel_pathlist can propagate unsafety
down again, as you've done here.

> (Official status update: I'm not prepared to commit this without some
> review.  So I intend to wait for a while and see whether I get some
> review.  I realize these status updates are supposed to contain a date
> by which further action will be taken, but I don't know how to
> meaningfully do that in this type of case.)

You mentioned that you'll be on vacation for much of July.  If you like,
I will take this open item off your hands, since I'll be around and can
deal with any bugs that pop up in it.

regards, tom lane


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

Re: [HACKERS] Column COMMENTs in CREATE TABLE?

[Tom Lane]

David Fetter  writes:
> On Sat, Jul 02, 2016 at 01:06:49PM -0400, David G. Johnston wrote:
>> ​+1 for the idea - though restricting it to columns would not be ideal.

> +1 for adding it to all the CREATEs whose objects support COMMENT.

TBH, I think this is a pretty bad idea.  I can see the reasoning for
allowing COMMENT in a table column definition, but the argument for
allowing it in simpler CREATEs seems tissue-thin:

CREATE FUNCTION foo(int) RETURNS ... ;
COMMENT ON FUNCTION foo(int) IS 'blah';

vs

CREATE FUNCTION foo(int) RETURNS ...
WITH (COMMENT 'blah');

Not much of a keystroke savings, nor is the comment noticeably
"closer" to its object than before.  Furthermore, the code footprint
of allowing that everywhere will be enormous.  And for statements that
already use WITH for something, I'm not sure you'll be able to
shoehorn this in without any grammatical trouble, either.  (It would
certainly be embarrassing if you did thirty-five flavors of CREATE
this way and then the syntax failed to work in the thirty-sixth.)

I think we should add something to ColumnDef and call it good.

regards, tom lane


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

[HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

[Kevin Grittner]

On Sat, Jul 2, 2016 at 5:10 PM, Robert Haas  wrote:
> On Sat, Jul 2, 2016 at 3:20 PM, Kevin Grittner  wrote:

>> Attached is a patch which fixes this issue, which I will push
>> Monday unless there are objections.
>
> Considering that (1) this was posted on a weekend and (2) that Monday
> is also a US holiday and (3) that we are not about to wrap a release,
> I think you should postpone the proposed commit date by a few days to
> allow time for review.

OK, will push Thursday, the 7th of July unless there are objections.

-- 
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] to_date_valid()

[Andreas 'ads' Scherbaum]

On 03.07.2016 07:05, Jaime Casanova wrote:

El 2/7/2016 20:33, "Euler Taveira" > escribió:
 >
 > On 02-07-2016 22:04, Andreas 'ads' Scherbaum wrote:
 > > The attached patch adds a new function "to_date_valid()" which will
 > > validate the date and return an error if the input and output date do
 > > not match. Tests included, documentation update as well.
 > >
 > Why don't you add a third parameter (say, validate = true | false)
 > instead of creating another function? The new parameter could default to
 > false to not break compatibility.
 >

Shouldn't we fix this instead? Sounds like a bug to me. We don't usually
want to be bug compatible so it doesn't matter if we break something.


There are previous discussions about such a change, and this was rejected:

https://www.postgresql.org/message-id/lbjf1v%24a2v%241%40ger.gmane.org
https://www.postgresql.org/message-id/A737B7A37273E048B164557ADEF4A58B17C9140E%40ntex2010i.host.magwien.gv.at

Hence the new function, which does not collide with the existing 
implementation.



Regards,

--
Andreas 'ads' Scherbaum
German PostgreSQL User Group
European PostgreSQL User Group - Board of Directors
Volunteer Regional Contact, Germany - PostgreSQL Project



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

[HACKERS] Re: GiST optimizing memmoves in gistplacetopage for fixed-size updates [PoC]

[Alexander Korotkov]

Hi!

On Sun, Jul 3, 2016 at 12:24 PM, Andrew Borodin 
wrote:

> I think there is some room for improving GiST inserts. Following is
> the description of what I think is performance problem.
>
> Function gistplacetopage in file /src/backend/access/gist/gist.c is
> responsible for updating or appending new tuple on GiST page.
> Currently, after checking necessity of page split due to overflow, it
> essentially executes following:
>
>if (OffsetNumberIsValid(oldoffnum))
>PageIndexTupleDelete(page, oldoffnum);
>gistfillbuffer(page, itup, ntup, InvalidOffsetNumber);
>
> That is: remove old tuple if it’s there, then place updated tuple at the
> end.
>
> Half of the old data have to be shifted my memmove inside
> PageIndexTupleDelete() call, half of the linp-s have to be corrected.
>
> If the updated tuple has same size as already residing on page we can
> just overwrite it. Attached patch demonstrates that concept. Attached
> test.sql inserts million rows into GiST index based on cube extension.
> My machine is Hyper-V VM running Ubuntu on i5-2500 CPU with SSD
> storage. Before patch, insert part of test is executed on average
> within 159 second, after patch application: insert part is executed
> within 77 seconds on average. That is almost twice faster (for
> CPU\Mem-bounded inserts, disk-constrained test will show no
> improvement). But it works only for fixed-size tuple inserts.
>

Very promising results!

I know that code in patch is far from beautiful: it operates with
> three different levels of abstraction within 5 lines of code. Those
> are low level memmove(), system-wide PageAddItem() and GiST private
> gistfillBuffer().
>
> By the way PageAddItem() have overwrite flag, but it only works with
> unused ItemId’s. Marking old ItemId as unused before PageAddItem()
> didn’t work for me. Unfortunately bufpage.c routines do not contain
> one for updating(replacing with new) tuple on page. It is important
> for me because I’m working on advanced GiST page layout (
>
> https://www.postgresql.org/message-id/CAJEAwVE0rrr%2BOBT-P0gDCtXbVDkBBG_WcXwCBK%3DGHo4fewu3Yg%40mail.gmail.com
> ), current approach is to use skip-tuples which can be used to skip
> many flowing tuples with one key check. Obviously, this design cares
> about tuples order. And update in a fashion “place updated tuple at
> the end” won’t work for me.
>
> So, I think it would be better to implement PageReplaceItem()
> functionality to make code better, to make existing GiST inserts
> faster and to enable new advanced page layouts in indices.
>

+1 for PageReplaceItem()
Even if item is not the same size, we can move the tail of page once
instead of twice.
I think you should implement PageReplaceItem() version and add it to the
commitfest.

--
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

[HACKERS] GiST optimizing memmoves in gistplacetopage for fixed-size updates [PoC]

[Andrew Borodin]

Hi, hackers!

I think there is some room for improving GiST inserts. Following is
the description of what I think is performance problem.

Function gistplacetopage in file /src/backend/access/gist/gist.c is
responsible for updating or appending new tuple on GiST page.
Currently, after checking necessity of page split due to overflow, it
essentially executes following:

   if (OffsetNumberIsValid(oldoffnum))
   PageIndexTupleDelete(page, oldoffnum);
   gistfillbuffer(page, itup, ntup, InvalidOffsetNumber);

That is: remove old tuple if it’s there, then place updated tuple at the end.

Half of the old data have to be shifted my memmove inside
PageIndexTupleDelete() call, half of the linp-s have to be corrected.

If the updated tuple has same size as already residing on page we can
just overwrite it. Attached patch demonstrates that concept. Attached
test.sql inserts million rows into GiST index based on cube extension.
My machine is Hyper-V VM running Ubuntu on i5-2500 CPU with SSD
storage. Before patch, insert part of test is executed on average
within 159 second, after patch application: insert part is executed
within 77 seconds on average. That is almost twice faster (for
CPU\Mem-bounded inserts, disk-constrained test will show no
improvement). But it works only for fixed-size tuple inserts.

I know that code in patch is far from beautiful: it operates with
three different levels of abstraction within 5 lines of code. Those
are low level memmove(), system-wide PageAddItem() and GiST private
gistfillBuffer().

By the way PageAddItem() have overwrite flag, but it only works with
unused ItemId’s. Marking old ItemId as unused before PageAddItem()
didn’t work for me. Unfortunately bufpage.c routines do not contain
one for updating(replacing with new) tuple on page. It is important
for me because I’m working on advanced GiST page layout (
https://www.postgresql.org/message-id/CAJEAwVE0rrr%2BOBT-P0gDCtXbVDkBBG_WcXwCBK%3DGHo4fewu3Yg%40mail.gmail.com
), current approach is to use skip-tuples which can be used to skip
many flowing tuples with one key check. Obviously, this design cares
about tuples order. And update in a fashion “place updated tuple at
the end” won’t work for me.

So, I think it would be better to implement PageReplaceItem()
functionality to make code better, to make existing GiST inserts
faster and to enable new advanced page layouts in indices.

Current work is here https://github.com/x4m/pggistopt/tree/simplefix

What do you think about found performance problem and about patch
attached? Am I missing something?



Best regards, Andrey Borodin, Octonica & Ural Federal University.


test.sql
Description: Binary data


GiST memmove.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