Re: [HACKERS] [PATCH] A hook for session start

[Michael Paquier]

On Fri, Nov 3, 2017 at 1:55 PM, Fabrízio de Royes Mello
 wrote:
>> Passing the database name and user name does not look much useful to
>> me. You can have access to this data already with CurrentUserId and
>> MyDatabaseId.
>
> This way we don't need to convert oid to names inside hook code.

Well, arguments of hooks are useful if they are used. Now if I look at
the two examples mainly proposed in this thread, be it in your set of
patches or the possibility to do some SQL transaction, I see nothing
using them. So I'd vote for keeping an interface minimal.
-- 
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] Exclude pg_internal.init from base backup

[Michael Paquier]

On Fri, Nov 3, 2017 at 4:04 PM, Petr Jelinek
 wrote:
> Not specific problem to this patch, but I wonder if it should be made
> more clear that those files (there are couple above of what you added)
> are skipped no matter which directory they reside in.

Agreed, it is a good idea to tell in the docs how this behaves. We
could always change things so as the comparison is based on the full
path like what is done for pg_control, but that does not seem worth
complicating the code.
-- 
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] Small improvement to compactify_tuples

[Claudio Freire]

On Fri, Nov 3, 2017 at 4:30 PM, Tom Lane  wrote:
> Claudio Freire  writes:
>> On Thu, Nov 2, 2017 at 11:46 PM, Tom Lane  wrote:
>>> BTW, the originally given test case shows no measurable improvement
>>> on my box.
>
>> I did manage to reproduce the original test and got a consistent improvement.
>
> This gave me the idea to memcpy the page into some workspace and then use
> memcpy, not memmove, to put the tuples back into the caller's copy of the
> page.  That gave me about a 50% improvement in observed TPS, and a perf
> profile like this:
>
> +   38.50%38.40%299520  postmaster   postgres 
>   [.] compactify_tuples
> +   31.11%31.02%241975  postmaster   libc-2.12.so 
>   [.] memcpy
> +8.74% 8.72% 68022  postmaster   postgres 
>   [.] itemoffcompare
> +6.51% 6.49% 50625  postmaster   postgres 
>   [.] compactify_tuples_loop
> +4.21% 4.19% 32719  postmaster   postgres 
>   [.] pg_qsort
> +1.70% 1.69% 13213  postmaster   postgres 
>   [.] memcpy@plt
>
> There still doesn't seem to be any point in replacing the qsort,
> but it does seem like something like the second attached patch
> might be worth doing.
>
> So I'm now wondering why my results seem to be so much different
> from those of other people who have tried this, both as to whether
> compactify_tuples is worth working on at all and as to what needs
> to be done to it if so.  Thoughts?
>
> regards, tom lane
>

I'm going to venture a guess that the version of gcc and libc, and
build options used both in the libc (ie: the distro) and postgres may
play a part here.

I'm running with glibc 2.22, for instance, and building with gcc 4.8.5.

I will try and benchmark memcpy vs memmove and see what the
performance difference is there with my versions, too. This may
heavily depend on compiler optimizations that may vary between
versions, since memcpy/memmove tend to be inlined a lot.


-- 
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 Plans and Cost of non-filter functions

[Tom Lane]

Paul Ramsey  writes:
>> Whether I get a parallel aggregate seems entirely determined by the number
>> of rows, not the cost of preparing those rows.

> This is true, as far as I can tell and unfortunate. Feeding tables with
> 100ks of rows, I get parallel plans, feeding 10ks of rows, never do, no
> matter how costly the work going on within. That's true of changing costs
> on the subquery select list, and on the aggregate transfn.

This sounds like it might be the same issue being discussed in

https://www.postgresql.org/message-id/flat/CAMkU=1ycXNipvhWuweUVpKuyu6SpNjF=yhwu4c4us5jgvgx...@mail.gmail.com

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] Parallel Plans and Cost of non-filter functions

[Paul Ramsey]

Just clarifying myself a little, since I made a dumb error partway through.

On Thu, Nov 2, 2017 at 10:09 AM, Paul Ramsey 
wrote:

> I'm working on a custom aggregate, that generates a serialized data
> format. The preparation of the geometry before being formatted is pretty
> intense, so it is probably a good thing for that work to be done in
> parallel, in partial aggregates. Here's an example SQL call:
>
> EXPLAIN analyze
> SELECT length(ST_AsMVT(a)) FROM (
> SELECT ST_AsMVTGeom(p.geom, ::geometry_literal, 4096, 0, true), gid,
> fed_num
> FROM pts_10 p
> WHERE p.geom && ::geometry_literal
> AND p.geom IS NOT NULL
> ) a;
>
> The ST_AsMVTGeom() function can be comically expensive, it's really good
> when it's in partial aggregates. But the cost of the function seems to be
> ignored.
>
> (First note that, in order to consistently get parallel plans I have to
> brutally suppress parallel_tuple_cost, as described here http://blog.
> cleverelephant.ca/2017/10/parallel-postgis-2.html)
>
> Whether I get a parallel aggregate seems entirely determined by the number
> of rows, not the cost of preparing those rows.
>

This is true, as far as I can tell and unfortunate. Feeding tables with
100ks of rows, I get parallel plans, feeding 10ks of rows, never do, no
matter how costly the work going on within. That's true of changing costs
on the subquery select list, and on the aggregate transfn.


> When changing the number of rows in the subquery, with a LIMIT, I can
> change from a seq scan to a paralllel seq scan and finally to a parallel
> aggregate, as the number of rows goes up.
>

I see now that as soon as I brought the LIMIT in, the plans had to go
sequential, just due to the nature of a LIMIT in a subquery. Ignore the
below, sorry.
Thanks!
P



>
> An odd effect: when I have enough rows to get a paralllel seq scan, I get
> flip it back to a seq scan, by *increasing* the cost of ST_AsMVTGeom. That
> seems odd and backwards.
>
> Is there anywhere a guide or rough description to how costs are used in
> determining parallel plans? The empirical approach starts to wear one down
> after a while :)
>
> P.
>
>

Re: [HACKERS] How to implement a SP-GiST index as a extension module?

[Alexander Korotkov]

On Fri, Nov 3, 2017 at 12:37 PM, Connor Wolf <
conn...@imaginaryindustries.com> wrote:

> EDIT: That's actually exactly how the example I'm working off of works.
> DERP. The SQL is
>
> CREATE TYPE vptree_area AS
> (
> center _int4,
> distance float8
> );
>
> CREATE OR REPLACE FUNCTION vptree_area_match(_int4, vptree_area) RETURNS
> boolean AS
> 'MODULE_PATHNAME','vptree_area_match'
> LANGUAGE C IMMUTABLE STRICT;
>
> CREATE OPERATOR <@ (
> LEFTARG = _int4,
> RIGHTARG = vptree_area,
> PROCEDURE = vptree_area_match,
> RESTRICT = contsel,
> JOIN = contjoinsel);
>
> so I just need to understand how to parse out the custom type in my index
> operator.
>

You can see the implementation of vptree_area_match function located in
vptree.c.  It just calls GetAttributeByNum() function.

There is also alternative approach for that implemented in pg_trgm contrib
module.  It has "text % text" operator which checks if two strings are
similar enough.  The similarity threshold is defined by
pg_trgm.similarity_threshold GUC.  Thus, you can also define GUC with
threshold distance value.  However, it would place some limitations.  For
instance, you wouldn't be able to use different distance threshold in the
same query.

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

Re: [HACKERS] WIP: long transactions on hot standby feedback replica / proof of concept

[Alexander Korotkov]

On Wed, Nov 1, 2017 at 5:55 AM, Masahiko Sawada 
wrote:

> On Tue, Oct 31, 2017 at 6:17 PM, Alexander Korotkov
>  wrote:
> > On Tue, Oct 31, 2017 at 5:16 AM, Masahiko Sawada 
> > wrote:
> >>
> >> On Mon, Oct 30, 2017 at 10:16 PM, Robert Haas 
> >> wrote:
> >> > On Tue, Oct 24, 2017 at 1:26 PM, Ivan Kartyshov
> >> >  wrote:
> >> >> Hello. I made some bugfixes and rewrite the patch.
> >> >
> >> > I don't think it's a good idea to deliberately leave the state of the
> >> > standby different from the state of the  master on the theory that it
> >> > won't matter.  I feel like that's something that's likely to come back
> >> > to bite us.
> >>
> >> I agree with Robert. What happen if we intentionally don't apply the
> >> truncation WAL and switched over? If we insert a tuple on the new
> >> master server to a block that has been truncated on the old master,
> >> the WAL apply on the new standby will fail? I guess there are such
> >> corner cases causing failures of WAL replay after switch-over.
> >
> >
> > Yes, that looks dangerous.  One approach to cope that could be teaching
> heap
> > redo function to handle such these situations.  But I expect this
> approach
> > to be criticized for bad design.  And I understand fairness of this
> > criticism.
> >
> > However, from user prospective of view, current behavior of
> > hot_standby_feedback is just broken, because it both increases bloat and
> > doesn't guarantee that read-only query on standby wouldn't be cancelled
> > because of vacuum.  Therefore, we should be looking for solution: if one
> > approach isn't good enough, then we should look for another approach.
> >
> > I can propose following alternative approach: teach read-only queries on
> hot
> > standby to tolerate concurrent relation truncation.  Therefore, when
> > non-existent heap page is accessed on hot standby, we can know that it
> was
> > deleted by concurrent truncation and should be assumed to be empty.  Any
> > thoughts?
> >
>
> You also meant that the applying WAL for AccessExclusiveLock is always
> skipped on standby servers to allow scans to access the relation?


Definitely not every AccessExclusiveLock WAL records should be skipped, but
only whose were emitted during heap truncation.  There are other cases when
AccessExclusiveLock WAL records are emitted, for instance, during DDL
operations.  But, I'd like to focus on AccessExclusiveLock WAL records
caused by VACUUM for now.  It's kind of understandable for users that DDL
might cancel read-only query on standby.  So, if you're running long report
query then you should wait with your DDL.  But VACUUM is a different
story.  It runs automatically when you do normal DML queries.

AccessExclusiveLock WAL records by VACUUM could be either not emitted, or
somehow distinguished and skipped on standby.  I haven't thought out that
level of detail for now.

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

Re: [HACKERS] WIP: long transactions on hot standby feedback replica / proof of concept

[Alexander Korotkov]

On Thu, Nov 2, 2017 at 5:56 AM, Robert Haas  wrote:

> On Tue, Oct 31, 2017 at 2:47 PM, Alexander Korotkov
>  wrote:
> > However, from user prospective of view, current behavior of
> > hot_standby_feedback is just broken, because it both increases bloat and
> > doesn't guarantee that read-only query on standby wouldn't be cancelled
> > because of vacuum.  Therefore, we should be looking for solution: if one
> > approach isn't good enough, then we should look for another approach.
> >
> > I can propose following alternative approach: teach read-only queries on
> hot
> > standby to tolerate concurrent relation truncation.  Therefore, when
> > non-existent heap page is accessed on hot standby, we can know that it
> was
> > deleted by concurrent truncation and should be assumed to be empty.  Any
> > thoughts?
>
> Sounds like it might break MVCC?


I don't know why it might be broken.  VACUUM truncates heap only when tail
to be truncated is already empty.  When applying truncate WAL record,
previous WAL records deleting all those tuples in the tail are already
applied.  Thus, if even MVCC is broken and we will miss some tuples after
heap truncation, they were anyway were gone before heap truncation.

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

Re: [HACKERS] Re: PANIC: invalid index offnum: 186 when processing BRIN indexes in VACUUM

[Jeff Janes]

On Fri, Nov 3, 2017 at 1:34 PM, Tom Lane  wrote:

> Jeff Janes  writes:
> > With this v3 patch (assuming this is the one you just committed
> > as ec42a1dcb30de235b252f6d4) am now getting make check failures.
>
> There's a followup commit already :-(
>
> regards, tom lane
>

Yeah, that fixed it.  Thanks.

Jeff

Re: [HACKERS] Small improvement to compactify_tuples

[Tom Lane]

I wrote:
> Have not looked at the 0002 patch yet.

I looked at that one, and it seems to be a potential win with no
downside, so pushed.  (I tweaked it slightly to avoid an unnecessary
conflict with the test patch I posted earlier.)

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] Re: PANIC: invalid index offnum: 186 when processing BRIN indexes in VACUUM

[Tom Lane]

Jeff Janes  writes:
> With this v3 patch (assuming this is the one you just committed
> as ec42a1dcb30de235b252f6d4) am now getting make check failures.

There's a followup commit already :-(

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] [COMMITTERS] pgsql: Fix freezing of a dead HOT-updated tuple

[Andres Freund]

On November 4, 2017 1:22:04 AM GMT+05:30, Alvaro Herrera 
 wrote:
>Peter Geoghegan wrote:
>> Andres Freund  wrote:
>
>> > Staring at the vacuumlazy hunk I think I might have found a related
>bug:
>> > heap_update_tuple() just copies the old xmax to the new tuple's
>xmax if
>> > a multixact and still running.  It does so without verifying
>liveliness
>> > of members.  Isn't that buggy? Consider what happens if we have
>three
>> > blocks: 1 has free space, two is being vacuumed and is locked,
>three is
>> > full and has a tuple that's key share locked by a live tuple and is
>> > updated by a dead xmax from before the xmin horizon. In that case
>afaict
>> > the multi will be copied from the third page to the first one. 
>Which is
>> > quite bad, because vacuum already processed it, and we'll set
>> > relfrozenxid accordingly.  I hope I'm missing something here?
>> 
>> Can you be more specific about what you mean here? I think that I
>> understand where you're going with this, but I'm not sure.
>
>He means that the tuple that heap_update moves to page 1 (which will no
>longer be processed by vacuum) will contain a multixact that's older
>than relminmxid -- because it is copied unchanged by heap_update
>instead
>of properly checking against age limit.

Right. That, or a member xid below relminxid. I think both scenarios are 
possible.

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


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

Re: [HACKERS] Setting pd_lower in GIN metapage

[Tom Lane]

Michael Paquier  writes:
> On Fri, Nov 3, 2017 at 1:10 AM, Amit Kapila  wrote:
>> On Fri, Nov 3, 2017 at 2:54 AM, Tom Lane  wrote:
>>> I've marked the CF entry closed.  However, I'm not sure if we're quite
>>> done with this thread.  Weren't we going to adjust nbtree and hash to
>>> be more aggressive about labeling their metapages as REGBUF_STANDARD?

>> I have already posted the patches [1] for the same in this thread and
>> those are reviewed [2][3] as well. I have adjusted the comments as per
>> latest commit.   Please find updated patches attached.

> Confirmed. Setting those makes sense even if REGBUF_WILL_INIT is set,
> at least for page masking.

Thanks, I'd forgotten those patches were already posted.  Looks good,
so pushed.

Looking around, I noted that contrib/bloom also had the disease of
not telling log_newpage it was writing a standard-format metapage,
so I fixed that too.

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] [COMMITTERS] pgsql: Fix freezing of a dead HOT-updated tuple

[Peter Geoghegan]

Alvaro Herrera  wrote:

He means that the tuple that heap_update moves to page 1 (which will no
longer be processed by vacuum) will contain a multixact that's older
than relminmxid -- because it is copied unchanged by heap_update instead
of properly checking against age limit.


I see. The problem is more or less with this heap_update() code:

   /*
* And also prepare an Xmax value for the new copy of the tuple.  If there
* was no xmax previously, or there was one but all lockers are now gone,
* then use InvalidXid; otherwise, get the xmax from the old tuple.  (In
* rare cases that might also be InvalidXid and yet not have the
* HEAP_XMAX_INVALID bit set; that's fine.)
*/
   if ((oldtup.t_data->t_infomask & HEAP_XMAX_INVALID) ||
   HEAP_LOCKED_UPGRADED(oldtup.t_data->t_infomask) ||
   (checked_lockers && !locker_remains))
   xmax_new_tuple = InvalidTransactionId;
   else
   xmax_new_tuple = HeapTupleHeaderGetRawXmax(oldtup.t_data);

My naive guess is that we have to create a new MultiXactId here in at
least some cases, just like FreezeMultiXactId() sometimes does.

--
Peter Geoghegan


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

Re: [HACKERS] Re: PANIC: invalid index offnum: 186 when processing BRIN indexes in VACUUM

[Jeff Janes]

Hi Alvaro,

With this v3 patch (assuming this is the one you just committed
as ec42a1dcb30de235b252f6d4) am now getting make check failures.

brin_summarize_range is returning unexpected values.

CentOS6,
PostgreSQL 11devel on x86_64-pc-linux-gnu, compiled by gcc (GCC) 4.4.7
20120313 (Red Hat 4.4.7-18), 64-bit

Cheers,

Jeff


regression.diffs
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] LDAPS

[Thomas Munro]

On Sat, Nov 4, 2017 at 2:05 AM, Thomas Munro
 wrote:
> That
> said, I've only tested the attached lightly on FreeBSD + OpenLDAP and
> don't know if it'll work elsewhere.

Oops, that version's TAP test was a little too dependent on my
system's ldap.conf file.  Here's a version that sets the LDAPCONF env
var to fix that.

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


ldaps-v2.patch
Description: Binary data

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

Re: [HACKERS] Re: PANIC: invalid index offnum: 186 when processing BRIN indexes in VACUUM

[Alvaro Herrera]

Alvaro Herrera wrote:

> I think your argument is sensible for some uses (where people run manual
> VACUUM after loading data) but not others (where people just use manual
> VACUUM in place of autovacuuming -- because they don't trust autovac, or
> the schedule isn't convenient, or whatever other reason).  I've seen
> both things being done in production.

BTW I also noticed that creating an index does summarize the first range
(rather than leaving it empty, as this rationale would suggest).  I
think we changed this at the last minute before commit in 9.5 and never
revisited; I'm inclined to change it for pg11 (but not now, of course).

-- 
Á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] [COMMITTERS] pgsql: Fix freezing of a dead HOT-updated tuple

[Alvaro Herrera]

Peter Geoghegan wrote:
> Andres Freund  wrote:

> > Staring at the vacuumlazy hunk I think I might have found a related bug:
> > heap_update_tuple() just copies the old xmax to the new tuple's xmax if
> > a multixact and still running.  It does so without verifying liveliness
> > of members.  Isn't that buggy? Consider what happens if we have three
> > blocks: 1 has free space, two is being vacuumed and is locked, three is
> > full and has a tuple that's key share locked by a live tuple and is
> > updated by a dead xmax from before the xmin horizon. In that case afaict
> > the multi will be copied from the third page to the first one.  Which is
> > quite bad, because vacuum already processed it, and we'll set
> > relfrozenxid accordingly.  I hope I'm missing something here?
> 
> Can you be more specific about what you mean here? I think that I
> understand where you're going with this, but I'm not sure.

He means that the tuple that heap_update moves to page 1 (which will no
longer be processed by vacuum) will contain a multixact that's older
than relminmxid -- because it is copied unchanged by heap_update instead
of properly checking against age limit.

-- 
Á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] ucs_wcwidth vintage

[Tom Lane]

Alvaro Herrera  writes:
> Thomas Munro wrote:
>> src/backend/utils/mb/wchar.c contains a ~16 year old wcwidth
>> implementation that originally arrived in commit df4cba68, but the
>> upstream code[1] apparently continued evolving and there have been
>> more Unicode revisions since.

> I think we should update it to current upstream source, then, just like
> we (are supposed to) do for any other piece of code we adopt.

+1 ... also, is that upstream still the best reference?

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] [COMMITTERS] pgsql: Fix freezing of a dead HOT-updated tuple

[Peter Geoghegan]

Andres Freund  wrote:

Here's that patch.  I've stared at this some, and Robert did too. Robert
mentioned that the commit message might need some polish and I'm not
100% sure about the error message texts yet.


The commit message should probably say that the bug involves the
resurrection of previously dead tuples, which is different to there
being duplicates because a constraint is not enforced because HOT chains
are broken (that's a separate, arguably less serious problem).


Staring at the vacuumlazy hunk I think I might have found a related bug:
heap_update_tuple() just copies the old xmax to the new tuple's xmax if
a multixact and still running.  It does so without verifying liveliness
of members.  Isn't that buggy? Consider what happens if we have three
blocks: 1 has free space, two is being vacuumed and is locked, three is
full and has a tuple that's key share locked by a live tuple and is
updated by a dead xmax from before the xmin horizon. In that case afaict
the multi will be copied from the third page to the first one.  Which is
quite bad, because vacuum already processed it, and we'll set
relfrozenxid accordingly.  I hope I'm missing something here?


Can you be more specific about what you mean here? I think that I
understand where you're going with this, but I'm not sure.

--
Peter Geoghegan


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

Re: [HACKERS] Small improvement to compactify_tuples

[Tom Lane]

Claudio Freire  writes:
> On Thu, Nov 2, 2017 at 11:46 PM, Tom Lane  wrote:
>> BTW, the originally given test case shows no measurable improvement
>> on my box.

> I did manage to reproduce the original test and got a consistent improvement.

It occurred to me that I could force the issue by hacking bufpage.c to
execute compactify_tuples multiple times each time it was called, as in
the first patch attached below.  This has nothing directly to do with real
performance of course, but it's making use of the PG system to provide
realistic test data for microbenchmarking compactify_tuples.  I was a bit
surprised to find that I had to set the repeat count to 1000 to make
compactify_tuples really dominate the runtime (while using the originally
posted test case ... maybe there's a better one?).  But once I did get it
to dominate the runtime, perf gave me this for the CPU hotspots:

+   27.97%27.88%229040  postmaster   libc-2.12.so   
  [.] memmove
+   14.61%14.57%119704  postmaster   postgres   
  [.] compactify_tuples
+   12.40%12.37%101566  postmaster   libc-2.12.so   
  [.] _wordcopy_bwd_aligned
+   11.68%11.65% 95685  postmaster   libc-2.12.so   
  [.] _wordcopy_fwd_aligned
+7.67% 7.64% 62801  postmaster   postgres   
  [.] itemoffcompare
+7.00% 6.98% 57303  postmaster   postgres   
  [.] compactify_tuples_loop
+4.53% 4.52% 37111  postmaster   postgres   
  [.] pg_qsort
+1.71% 1.70% 13992  postmaster   libc-2.12.so   
  [.] memcpy

which says that micro-optimizing the sort step is a complete, utter waste
of time, and what we need to be worried about is the data copying part.

The memcpy part of the above is presumably from the scaffolding memcpy's
in compactify_tuples_loop, which is interesting because that's moving as
much data as the memmove's are.  So at least with RHEL6's version of
glibc, memmove is apparently a lot slower than memcpy.

This gave me the idea to memcpy the page into some workspace and then use
memcpy, not memmove, to put the tuples back into the caller's copy of the
page.  That gave me about a 50% improvement in observed TPS, and a perf
profile like this:

+   38.50%38.40%299520  postmaster   postgres   
[.] compactify_tuples
+   31.11%31.02%241975  postmaster   libc-2.12.so   
[.] memcpy
+8.74% 8.72% 68022  postmaster   postgres   
[.] itemoffcompare
+6.51% 6.49% 50625  postmaster   postgres   
[.] compactify_tuples_loop
+4.21% 4.19% 32719  postmaster   postgres   
[.] pg_qsort
+1.70% 1.69% 13213  postmaster   postgres   
[.] memcpy@plt

There still doesn't seem to be any point in replacing the qsort,
but it does seem like something like the second attached patch
might be worth doing.

So I'm now wondering why my results seem to be so much different
from those of other people who have tried this, both as to whether
compactify_tuples is worth working on at all and as to what needs
to be done to it if so.  Thoughts?

regards, tom lane

diff --git a/src/backend/storage/page/bufpage.c b/src/backend/storage/page/bufpage.c
index 41642eb..bf6d308 100644
*** a/src/backend/storage/page/bufpage.c
--- b/src/backend/storage/page/bufpage.c
*** compactify_tuples(itemIdSort itemidbase,
*** 465,470 
--- 465,489 
  	phdr->pd_upper = upper;
  }
  
+ static void
+ compactify_tuples_loop(itemIdSort itemidbase, int nitems, Page page)
+ {
+ 	itemIdSortData copy[Max(MaxIndexTuplesPerPage, MaxHeapTuplesPerPage)];
+ 	union {
+ 		char page[BLCKSZ];
+ 		double align;
+ 	} pagecopy;
+ 	int i;
+ 
+ 	for (i = 1; i < 1000; i++)
+ 	{
+ 		memcpy(copy, itemidbase, sizeof(itemIdSortData) * nitems);
+ 		memcpy(pagecopy.page, page, BLCKSZ);
+ 		compactify_tuples(copy, nitems, pagecopy.page);
+ 	}
+ 	compactify_tuples(itemidbase, nitems, page);
+ }
+ 
  /*
   * PageRepairFragmentation
   *
*** PageRepairFragmentation(Page page)
*** 560,566 
  	 errmsg("corrupted item lengths: total %u, available space %u",
  			(unsigned int) totallen, pd_special - pd_lower)));
  
! 		compactify_tuples(itemidbase, nstorage, page);
  	}
  
  	/* Set hint bit for PageAddItem */
--- 579,585 
  	 errmsg("corrupted item lengths: total %u, available space %u",
  			(unsigned int) totallen, pd_special - pd_lower)));
  
! 		compactify_tuples_loop(itemidbase, nstorage, page);
  	}
  
  	/* Set hint bit for PageAddItem */
*** PageIndexMultiDelete(Page page, OffsetNu
*** 940,946 
  	phdr->pd_lower = SizeOfPageHeaderData + nused * 

Re: [HACKERS] Proposal: Local indexes for partitioned table

[Tom Lane]

Alvaro Herrera  writes:
> Tom Lane wrote:
>> It might work to build the new key in a context that's initially a
>> child of CurrentMemoryContext, then reparent it to be a child of
>> CacheMemoryContext when done. 

> That's another way (than the PG_TRY block), but I think it's more
> complicated with no gain.

I disagree: PG_TRY blocks are pretty expensive.

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] ucs_wcwidth vintage

[Alvaro Herrera]

Thomas Munro wrote:
> Hi hackers,
> 
> src/backend/utils/mb/wchar.c contains a ~16 year old wcwidth
> implementation that originally arrived in commit df4cba68, but the
> upstream code[1] apparently continued evolving and there have been
> more Unicode revisions since.  It probably doesn't matter much: the
> observation made by Zr40 in the #postgresql IRC channel that lead me
> to guess that this code might be responsible is that emojis screw up
> psql's formatting, since current terminal emulators recognise them as
> double-width but PostgreSQL doesn't.  Still, it's interesting that we
> have artefacts deriving from various different frozen versions of the
> Unicode standard in the source tree, and that might affect some proper
> languages.
> 
> 樂

Ah, thanks for the test case:

alvherre=# select '樂', 'hello';
 ?column? │ ?column? 
──┼──
 樂│ hello
(1 fila)



-- 
Á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] ucs_wcwidth vintage

[Alvaro Herrera]

Thomas Munro wrote:
> Hi hackers,
> 
> src/backend/utils/mb/wchar.c contains a ~16 year old wcwidth
> implementation that originally arrived in commit df4cba68, but the
> upstream code[1] apparently continued evolving and there have been
> more Unicode revisions since.

I think we should update it to current upstream source, then, just like
we (are supposed to) do for any other piece of code we adopt.


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

[Alvaro Herrera]

> Robert Haas  writes:

> > We could do that, but the motivation for the current system was to
> > avoid leaking memory in a long-lived context.

Yeah, my approach here is to use a CATCH block that deletes the memory
context just created, thus avoiding a long-lived leak.

Tom Lane wrote:

> Another key point is to avoid leaving a corrupted relcache entry behind
> if you fail partway through.

Sure ... in the code as I have it we only assign the local variable to
the relcache entry if everything is succesful.  So no relcache
corruption should result.

> It might work to build the new key in a context that's initially a
> child of CurrentMemoryContext, then reparent it to be a child of
> CacheMemoryContext when done. 

That's another way (than the PG_TRY block), but I think it's more
complicated with no gain.

-- 
Á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] Where is it documented what role executes constraint triggers?

[Tom Lane]

Chapman Flack  writes:
> From a little experimenting in 9.5, it seems that a referential
> integrity trigger is executed with the identity of the referencED
> table's owner, but I have not been able to find this covered in
> the docs. Is this a documentation oversight, or is it explained
> somewhere I didn't look (or may have skimmed right over it)?

Don't know if it's documented anywhere user-facing, but a look into
the code in ri_triggers.c says we run RI queries as the owner of
whichever table the query touches (the referenced table for verification
of FK inserts, the referencing table when cascading a PK change).

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] Where is it documented what role executes constraint triggers?

[Nico Williams]

On Fri, Nov 03, 2017 at 02:09:00PM -0400, Chapman Flack wrote:
> From a little experimenting in 9.5, it seems that a referential
> integrity trigger is executed with the identity of the referencED
> table's owner, but I have not been able to find this covered in
> the docs. Is this a documentation oversight, or is it explained
> somewhere I didn't look (or may have skimmed right over it)?
> 
> The question came up at $work after the departure of $colleague,
> who had created some tables as himself and not changed their
> ownership. His role had the superuser bit at the time, so
> RI checks involving those tables never incurred 'permission denied'
> errors until he left. Then, his role was not dropped, only disabled
> for login and made no longer superuser, and that's when RI checks
> started incurring 'permission denied'.

Are the trigger functions SECURITY DEFINER?


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

[HACKERS] Where is it documented what role executes constraint triggers?

[Chapman Flack]

>From a little experimenting in 9.5, it seems that a referential
integrity trigger is executed with the identity of the referencED
table's owner, but I have not been able to find this covered in
the docs. Is this a documentation oversight, or is it explained
somewhere I didn't look (or may have skimmed right over it)?

The question came up at $work after the departure of $colleague,
who had created some tables as himself and not changed their
ownership. His role had the superuser bit at the time, so
RI checks involving those tables never incurred 'permission denied'
errors until he left. Then, his role was not dropped, only disabled
for login and made no longer superuser, and that's when RI checks
started incurring 'permission denied'.

-Chap


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

Re: [HACKERS] [PATCH] Add ALWAYS DEFERRED option for constraints

[Nico Williams]

On Fri, Nov 03, 2017 at 01:41:45PM -0400, Peter Eisentraut wrote:
> On 11/2/17 16:54, Nico Williams wrote:
> > Replacing condeferred and condeferrable with a char columns also
> > occurred to me, and though I assume backwards-incompatible changes to
> > pg_catalog tables are fair game, I assumed everyone would prefer
> > avoiding such changes where possible.
> 
> I don't think there is an overriding mandate to avoid such catalog
> changes.  Consider old clients that don't know about your new column.
> They might look at the catalog entries and derive information about a
> constraint, not being aware that there is additional information in
> another column that overrides that.  So in such cases it's arguably
> better to make a break.

Makes sense.

> (In any case, it might be worth waiting for a review of the rest of the
> patch before taking on a significant rewrite of the catalog structures.)

I'll wait then :)

When you're done with that I'll make this change (replacing those three
bool columns with a single char column).

> > Hmmm, must I do anything special about _downgrades_?  Does PostgreSQL
> > support downgrades?
> 
> no

Oh good.  Thanks for clarifying that.

Nico
-- 


-- 
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] Dynamic result sets from procedures

[Peter Eisentraut]

On 11/2/17 16:40, Daniel Verite wrote:
> But instead of having procedures not return anything,
> couldn't they return whatever resultset(s) they want to
> ("no resultset" being just a particular case of "anything"),
> so that we could leave out cursors and simply write:

We could in general design this any way we want.  I'm just going by
what's in the SQL standard and in existing implementations.

> CREATE PROCEDURE test()
> LANGUAGE plpgsql
> AS $$
>   RETURN QUERYEXECUTE 'SELECT 1 AS col1, 2 AS col2';
> END;
> $$;
> 
> Or is that not possible or not desirable?

RETURN means the execution ends there, so how would you return multiple
result sets?

> Similarly, for the SQL language, I wonder if the above example
> could be simplified to:
> 
> CREATE PROCEDURE pdrstest1()
> LANGUAGE SQL
> AS $$
>  SELECT * FROM cp_test2;
>  SELECT * FROM cp_test3;
> $$;
> by which the two result sets would go back to the client again
> without declaring explicit cursors.
> Currently, it does not error out and no result set is sent.

But maybe you don't want to return all those results, so you'd need a
way to designate which ones, e.g.,

AS $$
SELECT set_config('something', 'value');
SELECT * FROM interesting_table;  -- return only this one
SELECT set_config('something', 'oldvalue');
$$;

-- 
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] Add some const decorations to prototypes

[Tom Lane]

Peter Eisentraut  writes:
> On 10/31/17 10:56, Tom Lane wrote:
>>> Some functions have a strtol()-like behavior
>>> where they take in a const char * and return a pointer into that as
>>> another argument.  In those cases, I added a cast or two.

>> ... but I'm not sure that it's an improvement in cases where you have to
>> cast away the const somewhere else.  I realize that strtol has an ancient
>> pedigree, but I do not think it's very good design.

> Would you prefer leaving the input argument as char *, or change the
> endptr argument to const as well?

Just leave it as char*.  If you change the endptr argument you're going to
force every call site to change their return variable, and some of them
would end up having to cast away the const on their end.

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] Dynamic result sets from procedures

[Peter Eisentraut]

On 11/1/17 22:40, Robert Haas wrote:
> That seems like it is at least arguably a wire protocol break.  Today,
> if you send a string containing only one command, you will only get
> one answer.

The wire protocol already supports this.  And the wire protocol doesn't
really know about statements, only about a command string that might
contain multiple commands.  So I don't think anything would break.

-- 
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] Dynamic result sets from procedures

[Peter Eisentraut]

On 11/1/17 06:23, Pavel Stehule wrote:
> We need to think about how the \timing option should work in such
> scenarios.  Right now it does

> Has the total time sense  in this case?
> 
> should not be total time related to any fetched result?

The \timing option in psql measures from the time you send off the
statement until you get all the results back.  I don't see a fundamental
reason to change that if a statement happens to produce multiple
results.  The results already come over the write and are processed by
libpq.  So the time is already measured.  It's just that psql doesn't
print the non-last result sets.

We don't have any way to measure from psql how long each individual
result set took to compose.  For that you will need deeper tools like
EXPLAIN ANALYZE and some way to process that data.  That is way beyond
what \timing currently does.

-- 
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] Linking libpq statically to libssl

[Peter Eisentraut]

On 11/2/17 19:52, Stephen Frost wrote:
> Uh, libpq doesn't actually have symbol versioning, at least not on the
> installed Ubuntu packages for PG10, as shown by objdump -T :

You're right, I was dreaming.  But in any case, he would need symbol
versioning of libssl.

-- 
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] [PATCH] Add ALWAYS DEFERRED option for constraints

[Peter Eisentraut]

On 11/2/17 16:54, Nico Williams wrote:
> Replacing condeferred and condeferrable with a char columns also
> occurred to me, and though I assume backwards-incompatible changes to
> pg_catalog tables are fair game, I assumed everyone would prefer
> avoiding such changes where possible.

I don't think there is an overriding mandate to avoid such catalog
changes.  Consider old clients that don't know about your new column.
They might look at the catalog entries and derive information about a
constraint, not being aware that there is additional information in
another column that overrides that.  So in such cases it's arguably
better to make a break.

(In any case, it might be worth waiting for a review of the rest of the
patch before taking on a significant rewrite of the catalog structures.)

> Hmmm, must I do anything special about _downgrades_?  Does PostgreSQL
> support downgrades?

no

-- 
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] Add some const decorations to prototypes

[Peter Eisentraut]

On 10/31/17 10:56, Tom Lane wrote:
>> Some functions have a strtol()-like behavior
>> where they take in a const char * and return a pointer into that as
>> another argument.  In those cases, I added a cast or two.
> ... but I'm not sure that it's an improvement in cases where you have to
> cast away the const somewhere else.  I realize that strtol has an ancient
> pedigree, but I do not think it's very good design.

Would you prefer leaving the input argument as char *, or change the
endptr argument to const as well?

-- 
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] MERGE SQL Statement for PG11

[Peter Geoghegan]

Simon Riggs  wrote:

The *only* behavioural difference I have proposed would be the *lack*
of an ERROR in (some) concurrent cases.


I think that's a big difference.  Error vs. non-error is a big deal by
itself;


Are you saying avoiding an ERROR is a bad or good thing?


Are you really asking Robert to repeat what has already been said about
a dozen different ways?

That's *not* the only difference. You need to see a couple of steps
ahead to see further differences, as the real dilemma comes when you
have to reconcile having provided the UPSERT-guarantees with cases that
that doesn't map on to (which can happen in a number of different ways).

I don't understand why you'll talk about just about anything but that.
This is a high-level concern about the overarching design. Do you really
not understand the concern at this point?


also, the non-error case involves departing from MVCC
semantics just as INSERT .. ON CONFLICT UPDATE does.


Meaning what exactly? What situation occurs that a user would be concerned with?

Please describe exactly what you mean so we get it clear.

The concurrent behaviour for MERGE is allowed to be
implementation-specific, so we can define it any way we want.


Agreed -- we can. It isn't controversial at all to say that the SQL
standard has nothing to say on this question. The problem is that the
semantics you argue for are ill-defined, and seem to create more
problems than they solve. Why keep bringing up the SQL standard?

--
Peter Geoghegan


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

Re: [HACKERS] Subscriber resets additional columns to NULL on UPDATE

[Peter Eisentraut]

On 10/26/17 05:20, Petr Jelinek wrote:
> I found bug in logical replication where extra (nullable) columns on
> subscriber will be reset to NULL value when update comes from provider.
> 
> The issue is apparently that we /points finger at himself/ forgot to
> check specifically for columns that are not part of attribute map in
> slot_modify_cstrings() so the extra columns will fall through to the
> else block which sets the value to NULL.
> 
> Attached patch fixes it and adds couple of tests for this scenario.
> 
> This is rather serious issue so it would be good if we could get it
> fixed in 10.1.

done

-- 
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] Re: PANIC: invalid index offnum: 186 when processing BRIN indexes in VACUUM

[Alvaro Herrera]

Tom Lane wrote:
> Alvaro Herrera  writes:
> > Tom Lane wrote:
> >> Do we still need the complication in brinsummarize to discriminate
> >> against the last partial range?  Now that the lock consideration
> >> is gone, I think that might be a wart.
> 
> > In the case of VACUUM, it's not desirable to create a summarization for
> > the last partial range, because if the table is still being filled, that
> > would slow down the insertion process.
> 
> Hm.  Okay, but you should change the comment then, because "we do not want
> to spend one RelationGetNumberOfBlocks call" is a pretty weak reason.

Changed.

> Also, I think I would accept that argument for autovacuum, but maybe
> not so much for a manual vacuum.  Maybe you should drive it off
> IsAutovacuumWorker rather than which operation is being done.

I think your argument is sensible for some uses (where people run manual
VACUUM after loading data) but not others (where people just use manual
VACUUM in place of autovacuuming -- because they don't trust autovac, or
the schedule isn't convenient, or whatever other reason).  I've seen
both things being done in production.  If we do as you suggest, there is
no way to do the manual vacuum without summarizing the partial also; the
approach of doing the partial only in brin_summarize_new_values lets the
user choose what to do.

Once upon a time I thought about adding a reloption to let the user
choose what to do, but I never got around to writing a patch.

-- 
Á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] Exclude pg_internal.init from base backup

[Petr Jelinek]

Hi,

On 02/09/17 21:08, David Steele wrote:
> Hackers,
> 
> The cache in pg_internal.init was reused in days of yore but has been
> rebuilt on postmaster startup since v8.1.  It appears there is no reason
> for this file to be backed up.
> 

Makes sense.

> I also moved the RELCACHE_INIT_FILENAME constant to relcache.h to avoid
> duplicating the string.

+1

> +++ b/doc/src/sgml/protocol.sgml
> @@ -2384,6 +2384,11 @@ The commands accepted in walsender mode are:
> 
> 
>  
> + pg_internal.init
> +
> +   
> +   
> +

Not specific problem to this patch, but I wonder if it should be made
more clear that those files (there are couple above of what you added)
are skipped no matter which directory they reside in.

-- 
  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] Skip unneeded temp file in 'make html'

[David Fetter]

On Fri, Nov 03, 2017 at 11:34:18AM -0400, Peter Eisentraut wrote:
> On 11/2/17 22:07, David Fetter wrote:
> >  postgres.xml: $(srcdir)/postgres.sgml $(ALLSGML)
> > -   $(OSX) $(SPFLAGS) $(SGMLINCLUDE) -x lower $< >$@.tmp
> > -   $(call mangle-xml,book)
> > +   $(OSX) $(SPFLAGS) $(SGMLINCLUDE) -x lower $< | $(call mangle-xml,book)
> 
> The reason why it's not done that way is that this would not catch
> errors of the command before the pipe.

Thanks for clarifying.

Best,
David.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778

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


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

Re: [HACKERS] Skip unneeded temp file in 'make html'

[Peter Eisentraut]

On 11/2/17 22:07, David Fetter wrote:
>  postgres.xml: $(srcdir)/postgres.sgml $(ALLSGML)
> - $(OSX) $(SPFLAGS) $(SGMLINCLUDE) -x lower $< >$@.tmp
> - $(call mangle-xml,book)
> + $(OSX) $(SPFLAGS) $(SGMLINCLUDE) -x lower $< | $(call mangle-xml,book)

The reason why it's not done that way is that this would not catch
errors of the command before the pipe.

-- 
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] WIP: Aggregation push-down

[Antonin Houska]

Antonin Houska  wrote:

> This is another version of the patch.

> shard.tgz demonstrates the typical postgres_fdw use case. One query shows base
> scans of base relation's partitions being pushed to shard nodes, the other
> pushes down a join and performs aggregation of the join result on the remote
> node. Of course, the query can only references one particular partition, until
> the "partition-wise join" [1] patch gets committed and merged with this my
> patch.

Since [1] is already there, the new version of shard.tgz shows what I consider
the typical use case. (I'm aware of the postgres_fdw regression test failures,
I'll try to fix them all in the next version.)

Besides that:

* A concept of "path unique keys" has been introduced. It helps to find out if
  the final relation appears to generate a distinct set of grouping keys. If
  that happens, the final aggregation is replaced by mere call of aggfinalfn()
  function on each transient state value.

* FDW can sort rows by aggregate.

* enable_agg_pushdown GUC was added. The default value is false.

* I fixed errors reported during the previous CF.

* Added a few more regression tests.


I'm not about to add any other features now. Implementation of the missing
parts (see the TODO comments in the code) is the next step. But what I'd
appreciate most is a feedback on the design. Thanks.

> [1] https://commitfest.postgresql.org/14/994/

-- 
Antonin Houska
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt
Web: http://www.postgresql-support.de, http://www.cybertec.at



agg_pushdown_v4.tgz
Description: GNU Zip compressed data


shard.tgz
Description: GNU Zip compressed 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] Walsender timeouts and large transactions

[Petr Jelinek]

Hi,

thanks for checking.

On 02/11/17 10:00, Robert Haas wrote:
> On Wed, Nov 1, 2017 at 8:19 PM, Petr Jelinek
>  wrote:
>> sorry for the delay but I didn't have much time in past weeks to follow
>> this thread.
> 
> +TimestampTz now = GetCurrentTimestamp();
> +
>  /* output previously gathered data in a CopyData packet */
>  pq_putmessage_noblock('d', ctx->out->data, ctx->out->len);
> 
>  /*
>   * Fill the send timestamp last, so that it is taken as late as possible.
>   * This is somewhat ugly, but the protocol is set as it's already used 
> for
>   * several releases by streaming physical replication.
>   */
>  resetStringInfo();
> -pq_sendint64(, GetCurrentTimestamp());
> +pq_sendint64(, now);
>  memcpy(>out->data[1 + sizeof(int64) + sizeof(int64)],
> tmpbuf.data, sizeof(int64));
> 
> This change falsifies the comments.  Maybe initialize now just after
> resetSetringInfo() is done.

Eh, right, I can do that.

> 
> -/* fast path */
> -/* Try to flush pending output to the client */
> -if (pq_flush_if_writable() != 0)
> -WalSndShutdown();
> +/* Try taking fast path unless we get too close to walsender timeout. */
> +if (now < TimestampTzPlusMilliseconds(last_reply_timestamp,
> +  wal_sender_timeout / 2))
> +{
> +CHECK_FOR_INTERRUPTS();
> 
> -if (!pq_is_send_pending())
> -return;
> +/* Try to flush pending output to the client */
> +if (pq_flush_if_writable() != 0)
> +WalSndShutdown();
> +
> +if (!pq_is_send_pending())
> +return;
> +}
> 
> I think it's only the if (!pq_is_send_pending()) return; that needs to
> be conditional here, isn't it?  The pq_flush_if_writable() can be done
> unconditionally.
> 

Well, even the CHECK_FOR_INTERRUPTS() can be called unconditionally yes.
It just seems like it's needless call as we'll call both in for loop
anyway if we take the "slow" path. I admit it's not exactly big win
though. If you think it would improve readability I can move it.

-- 
  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] [COMMITTERS] pgsql: Fix freezing of a dead HOT-updated tuple

[Andres Freund]

On 2017-11-02 06:05:51 -0700, Andres Freund wrote:
> Hi,
> 
> On 2017-11-02 13:49:47 +0100, Alvaro Herrera wrote:
> > Andres Freund wrote:
> > > I think the problem is on the pruning, rather than the freezing side. We
> > > can't freeze a tuple if it has an alive predecessor - rather than
> > > weakining this, we should be fixing the pruning to not have the alive
> > > predecessor.
> > 
> > I gave a look at HTSV back then, but I didn't find what the right tweak
> > was, but then I only tried changing the return value to DEAD and
> > DELETE_IN_PROGRESS; the thought of selecting DEAD or RECENTLY_DEAD based
> > on OldestXmin didn't occur to me ... I was thinking that the fact that
> > there were live lockers meant that the tuple could not be removed,
> > obviously failing to notice that the subsequent versions of the tuple
> > would be good enough.
> 
> I'll try to write up a commit based on that idea. I think there's some
> comment work needed too, Robert and I were both confused by a few
> things.
> I'm unfortunately travelling atm - it's evening here, and I'll flying
> back to the US all Saturday. I'm fairly sure I'll be able to come up
> with a decent patch tomorrow, but I'll need review and testing by
> others.

Here's that patch.  I've stared at this some, and Robert did too. Robert
mentioned that the commit message might need some polish and I'm not
100% sure about the error message texts yet.

I'm not yet convinced that the new elog in vacuumlazy can never trigger
- but I also don't think we want to actually freeze the tuple in that
case.

Staring at the vacuumlazy hunk I think I might have found a related bug:
heap_update_tuple() just copies the old xmax to the new tuple's xmax if
a multixact and still running.  It does so without verifying liveliness
of members.  Isn't that buggy? Consider what happens if we have three
blocks: 1 has free space, two is being vacuumed and is locked, three is
full and has a tuple that's key share locked by a live tuple and is
updated by a dead xmax from before the xmin horizon. In that case afaict
the multi will be copied from the third page to the first one.  Which is
quite bad, because vacuum already processed it, and we'll set
relfrozenxid accordingly.  I hope I'm missing something here?

Greetings,

Andres Freund
>From 774376dc8f7ed76657660e6beb0f5191d1bdaae4 Mon Sep 17 00:00:00 2001
From: Andres Freund 
Date: Fri, 3 Nov 2017 07:52:29 -0700
Subject: [PATCH] Fix pruning of locked and updated tuples.

Previously it was possible that a tuple was not pruned during vacuum,
even though it's update xmax (i.e. the updating xid in a multixact
with both key share lockers and an updater) was below the cutoff
horizon.

As the freezing code assumed, rightly so, that that's not supposed to
happen, xmax would be preserved (as a member of a new multixact or
xmax directly). That causes two problems: For one the tuple is below
the xmin horizon, which can cause problems if the clog is truncated or
once there's an xid wraparound. The bigger problem is that that will
break HOT chains, leading to index lookups failing, which in turn can
lead to constraints being violated.

Fix the problem by recognizing that tuples can be DEAD instead of
RECENTLY_DEAD, even if the multixactid has alive members, if the
update_xid is below the xmin horizon. That's safe because newer
versions of the tuple will contain the locking xids.

As the wrong freezing of xmax can cause data corruption, extend sanity
checks and promote them to elogs rather than assertions.

Per-Discussion: Robert Haas and Freund
Reported-By: Daniel Wood
Discussion:
https://postgr.es/m/e5711e62-8fdf-4dca-a888-c200bf6b5...@amazon.com
https://postgr.es/m/20171102112019.33wb7g5wp4zpj...@alap3.anarazel.de
---
 src/backend/access/heap/heapam.c  | 34 --
 src/backend/commands/vacuumlazy.c | 13 +
 src/backend/utils/time/tqual.c| 53 +++
 src/test/isolation/isolation_schedule |  1 +
 4 files changed, 62 insertions(+), 39 deletions(-)

diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 765750b8743..9b963e0cd0d 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -6412,7 +6412,8 @@ FreezeMultiXactId(MultiXactId multi, uint16 t_infomask,
 			 */
 			if (TransactionIdPrecedes(xid, cutoff_xid))
 			{
-Assert(!TransactionIdDidCommit(xid));
+if (TransactionIdDidCommit(xid))
+	elog(ERROR, "can't freeze committed xmax");
 *flags |= FRM_INVALIDATE_XMAX;
 xid = InvalidTransactionId; /* not strictly necessary */
 			}
@@ -6484,6 +6485,8 @@ FreezeMultiXactId(MultiXactId multi, uint16 t_infomask,
 		{
 			TransactionId xid = members[i].xid;
 
+			Assert(TransactionIdIsValid(xid));
+
 			/*
 			 * It's an update; should we keep it?  If the transaction is known
 			 * aborted or crashed then it's okay to ignore it, otherwise not.
@@ -6512,18 +6515,24 @@ 

Re: [HACKERS] Small improvement to compactify_tuples

[Claudio Freire]

On Thu, Nov 2, 2017 at 11:46 PM, Tom Lane  wrote:
> Sokolov Yura  writes:
>> [ 0001-Improve-compactify_tuples.patch, v5 or thereabouts ]
>
> I started to review this patch.  I spent a fair amount of time on
> beautifying the code, because I found it rather ugly and drastically
> undercommented.  Once I had it to the point where it seemed readable,
> I went to check the shellsort algorithm against Wikipedia's entry,
> and found that this appears to be an incorrect implementation of
> shellsort: where pg_shell_sort_pass has
>
> for (_i = off; _i < _n; _i += off) \
>
> it seems to me that we need to have
>
> for (_i = off; _i < _n; _i += 1) \
>
> or maybe just _i++.  As-is, this isn't h-sorting the whole file,
> but just the subset of entries that have multiple-of-h indexes
> (ie, the first of the h distinct subfiles that should get sorted).
> The bug is masked by the final pass of plain insertion sort, but
> we are not getting the benefit we should get from the earlier passes.
>
> However, I'm a bit dubious that it's worth fixing that; instead
> my inclination would be to rip out the shellsort implementation
> entirely.  The code is only using it for the nitems <= 48 case
> (which makes the first three offset steps certainly no-ops) and
> I am really unconvinced that it's worth expending the code space
> for a shellsort rather than plain insertion sort in that case,
> especially when we have good reason to think that the input data
> is nearly sorted.

I actually noticed that and benchmarked some variants. Neither
made any noticeable difference in performance, so I decided not
to complain about them.

I guess the same case can be made for removing the shell sort.
So I'm inclined to agree.

> BTW, the originally given test case shows no measurable improvement
> on my box.

I did manage to reproduce the original test and got a consistent improvement.

> I was eventually able to convince myself by profiling
> that the patch makes us spend less time in compactify_tuples, but
> this test case isn't a very convincing one.
>
> So, quite aside from the bug, I'm not excited about committing the
> attached as-is.  I think we should remove pg_shell_sort and just
> use pg_insertion_sort.  If somebody can show a test case that
> provides a measurable speed improvement from the extra code,
> I could be persuaded to reconsider.

My tests modifying the shell sort didn't produce any measurable
difference, but I didn't test removing it altogether.


-- 
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: PANIC: invalid index offnum: 186 when processing BRIN indexes in VACUUM

[Tom Lane]

Alvaro Herrera  writes:
> Tom Lane wrote:
>> Do we still need the complication in brinsummarize to discriminate
>> against the last partial range?  Now that the lock consideration
>> is gone, I think that might be a wart.

> In the case of VACUUM, it's not desirable to create a summarization for
> the last partial range, because if the table is still being filled, that
> would slow down the insertion process.

Hm.  Okay, but you should change the comment then, because "we do not want
to spend one RelationGetNumberOfBlocks call" is a pretty weak reason.

Also, I think I would accept that argument for autovacuum, but maybe
not so much for a manual vacuum.  Maybe you should drive it off
IsAutovacuumWorker rather than which operation is being done.

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] Re: PANIC: invalid index offnum: 186 when processing BRIN indexes in VACUUM

[Alvaro Herrera]

Tom Lane wrote:
> Alvaro Herrera  writes:
> 
> > Yeah, I think this approach results in better code.  The attached patch
> > implements that, and it passes the test for me (incl. calling
> > brin_summarize_new_values concurrently with vacuum, when running the
> > insert; the former does include the final page range whereas the latter
> > does not.)
> 
> Hm, so IIUC the point is that once the placeholder tuple is in, we can
> rely on concurrent inserters to update it for insertions into pages that
> are added after we determine our scan stop point.  But if the scan stop
> point is chosen before inserting the placeholder, then we have a race
> condition.

Exactly.  We don't need to scan those pages once the placeholder tuple
is in.

> The given code seems a brick or so short of a load, though: if the table
> has been extended sufficiently, it could compute scanNumBlks as larger
> than bs_pagesPerRange, no?  You need to clamp the computation result.

Oops, right.

> Also, shouldn't the passed-in heapBlk always be a multiple of
> pagesPerRange already?

Yeah, I guess I can turn that into an assert.

> Do we still need the complication in brinsummarize to discriminate
> against the last partial range?  Now that the lock consideration
> is gone, I think that might be a wart.

You mean this code?

/*
 * Unless requested to summarize even a partial range, go away 
now if
 * we think the next range is partial.
 *
 * Maybe the table already grew to cover this range completely, 
but we
 * don't want to spend a whole RelationGetNumberOfBlocks to 
find out,
 * so just leave it for next time.
 */
if (!include_partial &&
(startBlk + pagesPerRange > heapNumBlocks))
break;

In the case of VACUUM, it's not desirable to create a summarization for
the last partial range, because if the table is still being filled, that
would slow down the insertion process.  So we pass include_partial=false
there.  In brin_summarize_new_values, the theory is that the user called
that function because they're done loading (at least temporarily) so
it's better to process the partial range.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>From 6d31088c39d455637179853a3c72a7d9e20fe053 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera 
Date: Thu, 2 Nov 2017 18:35:10 +0100
Subject: [PATCH v3] Fix summarization concurrent with relation extension

---
 src/backend/access/brin/brin.c | 91 --
 1 file changed, 62 insertions(+), 29 deletions(-)

diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c
index e6909d7aea..72ac8929bd 100644
--- a/src/backend/access/brin/brin.c
+++ b/src/backend/access/brin/brin.c
@@ -67,7 +67,7 @@ static BrinBuildState *initialize_brin_buildstate(Relation 
idxRel,
   BrinRevmap *revmap, 
BlockNumber pagesPerRange);
 static void terminate_brin_buildstate(BrinBuildState *state);
 static void brinsummarize(Relation index, Relation heapRel, BlockNumber 
pageRange,
- double *numSummarized, double *numExisting);
+ bool include_partial, double *numSummarized, double 
*numExisting);
 static void form_and_insert_tuple(BrinBuildState *state);
 static void union_tuples(BrinDesc *bdesc, BrinMemTuple *a,
 BrinTuple *b);
@@ -791,7 +791,7 @@ brinvacuumcleanup(IndexVacuumInfo *info, 
IndexBulkDeleteResult *stats)
 
brin_vacuum_scan(info->index, info->strategy);
 
-   brinsummarize(info->index, heapRel, BRIN_ALL_BLOCKRANGES,
+   brinsummarize(info->index, heapRel, BRIN_ALL_BLOCKRANGES, false,
  >num_index_tuples, 
>num_index_tuples);
 
heap_close(heapRel, AccessShareLock);
@@ -909,7 +909,7 @@ brin_summarize_range(PG_FUNCTION_ARGS)

RelationGetRelationName(indexRel;
 
/* OK, do it */
-   brinsummarize(indexRel, heapRel, heapBlk, , NULL);
+   brinsummarize(indexRel, heapRel, heapBlk, true, , NULL);
 
relation_close(indexRel, ShareUpdateExclusiveLock);
relation_close(heapRel, ShareUpdateExclusiveLock);
@@ -1129,7 +1129,8 @@ terminate_brin_buildstate(BrinBuildState *state)
 }
 
 /*
- * Summarize the given page range of the given index.
+ * On the given BRIN index, summarize the heap page range that corresponds
+ * to the heap block number given.
  *
  * This routine can run in parallel with insertions into the heap.  To avoid
  * missing those values from the summary tuple, we first insert a placeholder
@@ -1139,6 +1140,12 @@ terminate_brin_buildstate(BrinBuildState *state)
  * update of the index value happens 

Re: [HACKERS] MERGE SQL Statement for PG11

[Simon Riggs]

On 3 November 2017 at 08:26, Robert Haas  wrote:
> On Fri, Nov 3, 2017 at 1:05 PM, Simon Riggs  wrote:
>>> Therefore, if MERGE eventually uses INSERT .. ON CONFLICT
>>> UPDATE when a relevant unique index exists and does something else,
>>> such as your proposal of taking a strong lock, or Peter's proposal of
>>> doing this in a concurrency-oblivious manner, in other cases, then
>>> those two cases will behave very differently.
>>
>> The *only* behavioural difference I have proposed would be the *lack*
>> of an ERROR in (some) concurrent cases.
>
> I think that's a big difference.  Error vs. non-error is a big deal by
> itself;

Are you saying avoiding an ERROR is a bad or good thing?

> also, the non-error case involves departing from MVCC
> semantics just as INSERT .. ON CONFLICT UPDATE does.

Meaning what exactly? What situation occurs that a user would be concerned with?

Please describe exactly what you mean so we get it clear.

The concurrent behaviour for MERGE is allowed to be
implementation-specific, so we can define it any way we want.

-- 
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] Re: PANIC: invalid index offnum: 186 when processing BRIN indexes in VACUUM

[Tom Lane]

Alvaro Herrera  writes:
> Alvaro Herrera wrote:
>> Maybe a solution is to call RelationGetNumberOfBlocks() after the
>> placeholder tuple has been inserted, for the case where we would be
>> scanning past end of relation; passing InvalidBlockNumber as stop point
>> would indicate to do things that way.  I'll try with that approach now.

> Yeah, I think this approach results in better code.  The attached patch
> implements that, and it passes the test for me (incl. calling
> brin_summarize_new_values concurrently with vacuum, when running the
> insert; the former does include the final page range whereas the latter
> does not.)

Hm, so IIUC the point is that once the placeholder tuple is in, we can
rely on concurrent inserters to update it for insertions into pages that
are added after we determine our scan stop point.  But if the scan stop
point is chosen before inserting the placeholder, then we have a race
condition.

The given code seems a brick or so short of a load, though: if the table
has been extended sufficiently, it could compute scanNumBlks as larger
than bs_pagesPerRange, no?  You need to clamp the computation result.
Also, shouldn't the passed-in heapBlk always be a multiple of
pagesPerRange already?

Do we still need the complication in brinsummarize to discriminate
against the last partial range?  Now that the lock consideration
is gone, I think that might be a wart.

regards, tom lane


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

Re: [HACKERS] [PATCH] A hook for session start

[Fabrízio de Royes Mello]

On Fri, Nov 3, 2017 at 11:43 AM, Aleksandr Parfenov <
a.parfe...@postgrespro.ru> wrote:
>
> README file in patch 0003 is a copy of README from test_pg_dump module
> without any changes.
>

Thanks, I'll fix it.

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

Re: [HACKERS] [PATCH] A hook for session start

[Fabrízio de Royes Mello]

On Fri, Nov 3, 2017 at 11:19 AM, Michael Paquier 
wrote:
>
>  /*
> + * Setup handler to session end hook
> + */
> +if (IsUnderPostmaster)
> +on_proc_exit(do_session_end_hook, 0);
> I think that it would be better to place that in ShutdownPostgres.
> This way it is possible to take actions before any resource is shut
> down.
>

Hmmm... ok but I have some doubt... ShutdownPostgres make sure to abort any
active transaction (AbortOutOfAnyTransaction) and release all locks
(LockReleaseAll). If we hook session end at this point we'll do that after
this two steps and after that run again... something like:

...
/* Make sure we've killed any active transaction */
AbortOutOfAnyTransaction();

/*
 * User locks are not released by transaction end, so be sure to release
 * them explicitly.
 */
LockReleaseAll(USER_LOCKMETHOD, true);

if (session_end_hook) {
(*session_end_hook) (port->database_name, port->user_name);

/* Make sure session end hook doesn't leave trash behind */
AbortOutOfAnyTransaction();
LockReleaseAll(USER_LOCKMETHOD, true);
}
...


> Passing the database name and user name does not look much useful to
> me. You can have access to this data already with CurrentUserId and
> MyDatabaseId.
>

This way we don't need to convert oid to names inside hook code.

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

Re: [HACKERS] [PATCH] A hook for session start

[Aleksandr Parfenov]

README file in patch 0003 is a copy of README from test_pg_dump module
without any changes.

-- 
Aleksandr Parfenov
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] [PATCH] A hook for session start

[Michael Paquier]

On Thu, Nov 2, 2017 at 11:36 PM, Fabrízio de Royes Mello
 wrote:
> On Thu, Nov 2, 2017 at 5:42 AM, Aleksandr Parfenov
>  wrote:
>> Unfortunately, patches 0001 and 0002 don't apply to current master.
>>
>> The new status of this patch is: Waiting on Author
>
> Thanks for your review. Rebased versions attached.

Looking at this thread, there are clearly arguments in favor of having
a session hook after authentication. One use case mentioned by Robert
is inserting data into a table when a user logs in. I can imagine that
something like that could be applied to a session ending.

 /*
+ * Setup handler to session end hook
+ */
+if (IsUnderPostmaster)
+on_proc_exit(do_session_end_hook, 0);
I think that it would be better to place that in ShutdownPostgres.
This way it is possible to take actions before any resource is shut
down.

Passing the database name and user name does not look much useful to
me. You can have access to this data already with CurrentUserId and
MyDatabaseId.
-- 
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] Re: PANIC: invalid index offnum: 186 when processing BRIN indexes in VACUUM

[Alvaro Herrera]

Alvaro Herrera wrote:

> Maybe a solution is to call RelationGetNumberOfBlocks() after the
> placeholder tuple has been inserted, for the case where we would be
> scanning past end of relation; passing InvalidBlockNumber as stop point
> would indicate to do things that way.  I'll try with that approach now.

Yeah, I think this approach results in better code.  The attached patch
implements that, and it passes the test for me (incl. calling
brin_summarize_new_values concurrently with vacuum, when running the
insert; the former does include the final page range whereas the latter
does not.)

Tomas Vondra wrote:

> FWIW this patch fixes the issue for me - I can no longer reproduce the
> bitmapscan vs. seqscan result discrepancies (even with the extra UPDATE
> phase).

Thanks for testing!  This confirms that the issue was correctly
identified.  Would you try the current patch, which is better than the
other one?  It's significantly different that I think it invalidates
prior testing.

Thanks!

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>From b3568a5475526fbd7768bd4d74c6ad681ede920f Mon Sep 17 00:00:00 2001
From: Alvaro Herrera 
Date: Thu, 2 Nov 2017 18:35:10 +0100
Subject: [PATCH v2] Fix summarization concurrent with relation extension

---
 src/backend/access/brin/brin.c | 78 ++
 1 file changed, 49 insertions(+), 29 deletions(-)

diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c
index e6909d7aea..ad512c2c5f 100644
--- a/src/backend/access/brin/brin.c
+++ b/src/backend/access/brin/brin.c
@@ -67,7 +67,7 @@ static BrinBuildState *initialize_brin_buildstate(Relation 
idxRel,
   BrinRevmap *revmap, 
BlockNumber pagesPerRange);
 static void terminate_brin_buildstate(BrinBuildState *state);
 static void brinsummarize(Relation index, Relation heapRel, BlockNumber 
pageRange,
- double *numSummarized, double *numExisting);
+ bool include_partial, double *numSummarized, double 
*numExisting);
 static void form_and_insert_tuple(BrinBuildState *state);
 static void union_tuples(BrinDesc *bdesc, BrinMemTuple *a,
 BrinTuple *b);
@@ -791,7 +791,7 @@ brinvacuumcleanup(IndexVacuumInfo *info, 
IndexBulkDeleteResult *stats)
 
brin_vacuum_scan(info->index, info->strategy);
 
-   brinsummarize(info->index, heapRel, BRIN_ALL_BLOCKRANGES,
+   brinsummarize(info->index, heapRel, BRIN_ALL_BLOCKRANGES, false,
  >num_index_tuples, 
>num_index_tuples);
 
heap_close(heapRel, AccessShareLock);
@@ -909,7 +909,7 @@ brin_summarize_range(PG_FUNCTION_ARGS)

RelationGetRelationName(indexRel;
 
/* OK, do it */
-   brinsummarize(indexRel, heapRel, heapBlk, , NULL);
+   brinsummarize(indexRel, heapRel, heapBlk, true, , NULL);
 
relation_close(indexRel, ShareUpdateExclusiveLock);
relation_close(heapRel, ShareUpdateExclusiveLock);
@@ -1129,7 +1129,8 @@ terminate_brin_buildstate(BrinBuildState *state)
 }
 
 /*
- * Summarize the given page range of the given index.
+ * On the given BRIN index, summarize the heap page range that corresponds
+ * to the heap block number given.
  *
  * This routine can run in parallel with insertions into the heap.  To avoid
  * missing those values from the summary tuple, we first insert a placeholder
@@ -1139,6 +1140,12 @@ terminate_brin_buildstate(BrinBuildState *state)
  * update of the index value happens in a loop, so that if somebody updates
  * the placeholder tuple after we read it, we detect the case and try again.
  * This ensures that the concurrently inserted tuples are not lost.
+ *
+ * A further corner case is this routine being asked to summarize the partial
+ * range at the end of the table.  heapNumBlocks is the (possibly outdated)
+ * table size; if we notice that the requested range lies beyond that size,
+ * we re-compute the table size after inserting the placeholder tuple, to
+ * avoid missing pages that were appended recently.
  */
 static void
 summarize_range(IndexInfo *indexInfo, BrinBuildState *state, Relation heapRel,
@@ -1160,6 +1167,20 @@ summarize_range(IndexInfo *indexInfo, BrinBuildState 
*state, Relation heapRel,
   heapBlk, phtup, phsz);
 
/*
+* Whenever we're scanning what we believe to be the final range on the
+* table (i.e. one that might be partial) we need to recompute our idea 
of
+* what the latest page is after inserting the placeholder tuple.  This
+* ensures that we don't miss pages just because they were extended by
+* concurrent backends that didn't have the chance to see our 
placeholder
+* tuple.  Fortunately, this should occur 

[HACKERS] LDAPS

[Thomas Munro]

Hi hackers,

I've run into a few requests for $SUBJECT in the field.  I understand
that this is a bit controversial: LDAP + StartTLS (what we already
support) is better than LDAPS because it's a proper standard, and LDAP
auth in general is not as good as some other authentication methods
that we should be encouraging.  I don't actually have a strong view on
whether we should support it myself, but I took a swing at it to see
if there would be any technical obstacles.  I did not find any.  That
said, I've only tested the attached lightly on FreeBSD + OpenLDAP and
don't know if it'll work elsewhere.  Thoughts?

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


ldaps-v1.patch
Description: Binary data

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

Re: [HACKERS] parallelize queries containing initplans

[Amit Kapila]

On Mon, Oct 30, 2017 at 10:07 AM, Robert Haas  wrote:
> On Mon, Oct 30, 2017 at 9:00 AM, Amit Kapila  wrote:
>> Now that the PARAM_EXTERN issue is fixed, I have rebased this patch.
>> This patch had been switched to Ready For Committer in last CF, then
>> Robert had comments which I have addressed, so I think the status
>> should be switched back to Ready For committer.  Let me know if you
>> think it should be switched to some other status.
>
> The change to ExplainPrintPlan doesn't look good to me, because it
> actually moves the initPlan; I don't think it's good for EXPLAIN to
> mutate the plan state tree.  It should find a way to display the
> results *as if* the initPlans were attached to the subnode, but
> without actually moving them.
>


Actually, with the latest patch, we don't need these changes in
ExplainPrintPlan.   Earlier, we need these changes because the patch
had changed SS_charge_for_initplans to mark the path with initplan as
parallel safe.  However, after removing that change in the previous
version of patch [1], this is not required as now we won't add gather
on top plan node having initplan.


[1] - 
https://www.postgresql.org/message-id/CAA4eK1JD%3DpJYBn8rN5RimiEVtPJmVNmyq5p6VoZBnUw2xRYB7w%40mail.gmail.com
-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com


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

[HACKERS] LDAP URI decoding bugs

[Thomas Munro]

Hi hackers,

1.  If you set up a pg_hba.conf with a URL that lacks a base DN or
hostname, hba.c will segfault on startup when it tries to pstrdup a
null pointer.  Examples: ldapurl="ldap://localhost; and
ldapurl="ldap://;.

2.  If we fail to bind but have no binddn configured, we'll pass NULL
to ereport (snprint?) for %s, which segfaults on some libc
implementations.  That crash requires more effort to reproduce but you
can see pretty clearly a few lines above in auth.c that it can be
NULL.  (I'm surprised Coverity didn't complain about that.  Maybe it
can't see this code due to macros.)

Please see attached.

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


ldap-fixes.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] proposal: schema variables

[Chris Travers]

Some thoughts on this.

On Thu, Nov 2, 2017 at 4:48 PM, Tom Lane  wrote:

> Nico Williams  writes:
> > With access controls, GUCs could become schema variables, and settings
> > from postgresql.conf could move into the database itself (which I think
> > would be nice).
>
> People re-propose some variant of that every so often, but it never works,
> because it ignores the fact that some of the GUCs' values are needed
> before you can access system catalogs at all, or in places where relying
> on system catalog access would be a bad idea.
>

I think the basic point one should get here is that no matter the
unification, you still have some things in the db and some things out.

I would rather look at how the GUC could be improved on a functional/use
case level before we look at the question of a technical solution.

 One major use case today would be restricting how high various users can
set something like work_mem or the like.  As it stands, there isn't really
a way to control this with any granularity.  So some of the proposals
regarding granting access to a session variable would be very handy in
granting access to a GUC variable.

>
> Sure, we could have two completely different configuration mechanisms
> so that some of the variables could be "inside the database", but that
> doesn't seem like a net improvement to me.  The point of the Grand Unified
> Configuration mechanism was to be unified, after all.
>

+1

>
> I'm on board with having a totally different mechanism for session
> variables.  The fact that people have been abusing GUC to store
> user-defined variables doesn't make it a good way to do that.
>

What about having a more clunky syntax as:

SET VARIABLE foo='bar';

Perhaps one can have a short form of:

SET VAR foo = 'bar';

vs

SET foo = 'bar'; -- GUC



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



-- 
Best Regards,
Chris Travers
Database Administrator

Tel: +49 162 9037 210 | Skype: einhverfr | www.adjust.com
Saarbrücker Straße 37a, 10405 Berlin

Re: [HACKERS] dropping partitioned tables without CASCADE

[Alvaro Herrera]

Ashutosh Bapat wrote:
> On Fri, Nov 3, 2017 at 1:42 PM, Alvaro Herrera  
> wrote:

> > I think adding "is partitioned" at end of line isn't good; looks like a
> > phrase but isn't translatable.  Maybe add keyword PARTITIONED instead?
> 
> In that case may be we should separate bounds and "PARTITIONED" with a
> ",". "part_default DEFAULT, PARTITIONED" would read better than
> "part_default DEFAULT PARTITIONED"?

Hmm, I vote +0.5 for the comma.

> > Having the DEFAULT partition show up in the middle of the list is weird.
> 
> Agreed. But that's true even without this patch.

Yes.

> > Is it possible to put it at either start or end of the list?
> 
> Right now, we could do that if we order the list by bound expression;
> lexically DEFAULT would come before FOR VALUES ... . But that's not
> future-safe; we may have a bound expression starting with A, B or C.
> Beyond that it really gets tricky to order the partitions by bounds.

I was just thinking in changing the query to be "order by
is_the_default_partition, partition_name" instead of just "order by
partition_name".  Sorting by bounds rather than name (a feature whose
worth should definitely be discussed separately IMV) sounds a lot more
complicated.

-- 
Á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] SQL/JSON in PostgreSQL

[Michael Paquier]

On Fri, Nov 3, 2017 at 12:07 PM, Michael Paquier
 wrote:
> The patch sent previously does not directly apply on HEAD, and as far
> as I can see the last patch set published on
> https://www.postgresql.org/message-id/2361ae4a-66b1-c6c5-ea6a-84851a1c0...@postgrespro.ru
> has rotten. Could you send a new patch set?
>
> About the patch set, I had a look at the first patch which is not that
> heavy, however it provides zero documentation, close to zero comments,
> but adds more than 500 lines of code. I find that a bit hard to give
> an opinion on, having commit messages associated to each patch would
> be also nice. This way, reviewers can figure what's going out in this
> mess and provide feedback. Making things incremental is welcome as
> well, for example in the first patch I have a hard way finding out why
> timestamps are touched to begin with.

My mistake here, only the first patch adds 8,200 lines of code. This
makes the lack of comments and docs even worse.
-- 
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] Is it time to kill support for very old servers?

[Michael Paquier]

On Mon, Oct 16, 2017 at 10:08 PM, Andres Freund  wrote:
> On 2017-10-16 16:59:59 -0400, Peter Eisentraut wrote:
>> On 9/20/17 04:32, Andres Freund wrote:
>> > Here's what I roughly was thinking of.  I don't quite like the name, and
>> > the way the version is specified for libpq (basically just the "raw"
>> > integer).
>>
>> "forced_protocol_version" reads wrong to me.  I think
>> "force_protocol_version" might be better.  Other than that, no issues
>> with this concept.
>
> Yea, I agree. I've read through the patch since, and it struck me as
> odd. Not sure how I came up with it...

Andres, could you update the patch?
-- 
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] dropping partitioned tables without CASCADE

[Ashutosh Bapat]

On Fri, Nov 3, 2017 at 1:42 PM, Alvaro Herrera  wrote:
> Amit Langote wrote:
>> On 2017/09/06 19:14, Amit Langote wrote:
>> > On 2017/09/06 18:46, Rushabh Lathia wrote:
>> >> Okay, I have marked this as ready for committer.
>> >
>> > Thanks Ashutosh and Rushabh for rebasing and improving the patch.  Looks
>> > good to me too.
>>
>> Patch needed to be rebased after the default partitions patch went in, so
>> done.  Per build status on http://commitfest.cputube.org :)
>
> I think adding "is partitioned" at end of line isn't good; looks like a
> phrase but isn't translatable.  Maybe add keyword PARTITIONED instead?

In that case may be we should separate bounds and "PARTITIONED" with a
",". "part_default DEFAULT, PARTITIONED" would read better than
"part_default DEFAULT PARTITIONED"?

>
> Having the DEFAULT partition show up in the middle of the list is weird.

Agreed. But that's true even without this patch.

> Is it possible to put it at either start or end of the list?
>

Right now, we could do that if we order the list by bound expression;
lexically DEFAULT would come before FOR VALUES ... . But that's not
future-safe; we may have a bound expression starting with A, B or C.
Beyond that it really gets tricky to order the partitions by bounds.

The goal of this patch is to mark the partitioned partitions as such
and show the number of partitions. While your suggestion is a valid
request, it's kind of beyond the scope of this patch. Someone might
want to extend this request and say that partitions should be listed
in the order of their bounds (I do feel that we should do some effort
in that direction). But I am not sure whether it should be done in
this patch.

-- 
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] SQL/JSON in PostgreSQL

[Michael Paquier]

On Fri, Nov 3, 2017 at 11:29 AM, Nikita Glukhov  wrote:
> By standard only string literals can be used in JSON path specifications.
> But of course it is possible to allow to use variable jsonpath expressions
> in
> SQL/JSON functions.
>
> Attached patch implements this feature for JSON query functions, JSON_TABLE
> is
> not supported now because it needs some refactoring.
>
> I have pushed this commit to the separate branch because it is not finished
> yet:
> https://github.com/postgrespro/sqljson/tree/sqljson_variable_json_path

The patch sent previously does not directly apply on HEAD, and as far
as I can see the last patch set published on
https://www.postgresql.org/message-id/2361ae4a-66b1-c6c5-ea6a-84851a1c0...@postgrespro.ru
has rotten. Could you send a new patch set?

About the patch set, I had a look at the first patch which is not that
heavy, however it provides zero documentation, close to zero comments,
but adds more than 500 lines of code. I find that a bit hard to give
an opinion on, having commit messages associated to each patch would
be also nice. This way, reviewers can figure what's going out in this
mess and provide feedback. Making things incremental is welcome as
well, for example in the first patch I have a hard way finding out why
timestamps are touched to begin with.

The patch is already marked as "waiting on author" for more than one month.
-- 
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] postgres_fdw: Add support for INSERT OVERRIDING clause

[Michael Paquier]

On Tue, Oct 31, 2017 at 3:45 PM, Peter Eisentraut
 wrote:
> It has been pointed out to me that the command deparsing in postgres_fdw
> does not support the INSERT OVERRIDING clause that was added in PG10.
> Here is a patch that seems to fix that.  I don't know much about this,
> whether anything else needs to be added or whether there should be
> tests.  Perhaps someone more familiar with postgres_fdw details can
> check it.

Trying to insert some data using OVERRIDING SYSTEM VALUE on a foreign
table with a remote relation defined with GENERATED ALWAYS would just
fail:
=# insert into id_always_foreign OVERRIDING system VALUE values (8);
ERROR:  428C9: cannot insert into column "a"
DETAIL:  Column "a" is an identity column defined as GENERATED ALWAYS.
HINT:  Use OVERRIDING SYSTEM VALUE to override.

And that's confusing because there is no actual way to avoid this
error if postgres_fdw is unpatched.

I think that you should add some tests, and make sure that the
documentation of postgres-fdw.sgml mentions that those two clauses are
pushed down.
-- 
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] SQL/JSON in PostgreSQL

[Nikita Glukhov]

On 03.11.2017 00:32, Piotr Stefaniak wrote:


On 2017-02-28 20:08, Oleg Bartunov wrote:

The standard describes SQL/JSON path language, which used by SQL/JSON query
operators to query JSON. It defines path language as string literal. We
implemented the path language as  JSONPATH data type, since other
approaches are not friendly to planner and executor.

I was a bit sad to discover that I can't
PREPARE jsq AS SELECT JSON_QUERY('{}', $1);
I assume because of this part of the updated grammar:
json_path_specification:
 Sconst { $$ = $1; }
;

Would it make sense, fundamentally, to allow variables there? After
Andrew Gierth's analysis of this grammar problem, I understand that it's
not reasonable to expect JSON_TABLE() to support variable jsonpaths, but
maybe it would be feasible for everything else? From Andrew's changes to
the new grammar (see attached) it seems to me that at least that part is
possible. Or should I forget about trying to implement the other part?

By standard only string literals can be used in JSON path specifications.
But of course it is possible to allow to use variable jsonpath 
expressions in

SQL/JSON functions.

Attached patch implements this feature for JSON query functions, 
JSON_TABLE is

not supported now because it needs some refactoring.

I have pushed this commit to the separate branch because it is not 
finished yet:

https://github.com/postgrespro/sqljson/tree/sqljson_variable_json_path

--
Nikita Glukhov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
>From cba58ec1410eb99fc974d8a46013ce7331c93377 Mon Sep 17 00:00:00 2001
From: Nikita Glukhov 
Date: Fri, 3 Nov 2017 14:15:51 +0300
Subject: [PATCH] Allow variable jsonpath specifications

---
 src/backend/executor/execExpr.c   |  7 +++
 src/backend/executor/execExprInterp.c |  5 ++---
 src/backend/nodes/copyfuncs.c |  2 +-
 src/backend/parser/gram.y | 11 ++-
 src/backend/parser/parse_clause.c | 35 ++-
 src/backend/parser/parse_expr.c   | 19 +--
 src/backend/utils/adt/ruleutils.c | 11 +--
 src/include/executor/execExpr.h   |  3 ++-
 src/include/nodes/parsenodes.h|  2 +-
 src/include/nodes/primnodes.h |  2 +-
 10 files changed, 72 insertions(+), 25 deletions(-)

diff --git a/src/backend/executor/execExpr.c b/src/backend/executor/execExpr.c
index 3cc8e12..86030b9 100644
--- a/src/backend/executor/execExpr.c
+++ b/src/backend/executor/execExpr.c
@@ -2042,6 +2042,13 @@ ExecInitExprRec(Expr *node, PlanState *parent, ExprState *state,
 _expr->value,
 _expr->isnull);
 
+scratch.d.jsonexpr.pathspec =
+	palloc(sizeof(*scratch.d.jsonexpr.pathspec));
+
+ExecInitExprRec((Expr *) jexpr->path_spec, parent, state,
+>value,
+>isnull);
+
 scratch.d.jsonexpr.formatted_expr =
 		ExecInitExpr((Expr *) jexpr->formatted_expr, parent);
 
diff --git a/src/backend/executor/execExprInterp.c b/src/backend/executor/execExprInterp.c
index b9d719d..36ef0fd 100644
--- a/src/backend/executor/execExprInterp.c
+++ b/src/backend/executor/execExprInterp.c
@@ -3897,7 +3897,7 @@ ExecEvalJson(ExprState *state, ExprEvalStep *op, ExprContext *econtext)
 	*op->resnull = true;		/* until we get a result */
 	*op->resvalue = (Datum) 0;
 
-	if (op->d.jsonexpr.raw_expr->isnull)
+	if (op->d.jsonexpr.raw_expr->isnull || op->d.jsonexpr.pathspec->isnull)
 	{
 		/* execute domain checks for NULLs */
 		(void) ExecEvalJsonExprCoercion(op, econtext, res, op->resnull, isjsonb);
@@ -3905,8 +3905,7 @@ ExecEvalJson(ExprState *state, ExprEvalStep *op, ExprContext *econtext)
 	}
 
 	item = op->d.jsonexpr.raw_expr->value;
-
-	path = DatumGetJsonPath(jexpr->path_spec->constvalue);
+	path = DatumGetJsonPath(op->d.jsonexpr.pathspec->value);
 
 	/* reset JSON path variable contexts */
 	foreach(lc, op->d.jsonexpr.args)
diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index 80c0e48..34fed1d 100644
--- a/src/backend/nodes/copyfuncs.c
+++ b/src/backend/nodes/copyfuncs.c
@@ -2332,7 +2332,7 @@ _copyJsonCommon(const JsonCommon *from)
 	JsonCommon	   *newnode = makeNode(JsonCommon);
 
 	COPY_NODE_FIELD(expr);
-	COPY_STRING_FIELD(pathspec);
+	COPY_NODE_FIELD(pathspec);
 	COPY_STRING_FIELD(pathname);
 	COPY_NODE_FIELD(passing);
 	COPY_LOCATION_FIELD(location);
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index adfe9b1..71a59d1 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -624,6 +624,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
 	json_table_plan_cross
 	json_table_plan_primary
 	json_table_default_plan
+	json_path_specification
 
 %type 		json_arguments
 	json_passing_clause_opt
@@ -635,8 +636,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
 
 %type 		json_returning_clause_opt
 

Re: [HACKERS] Re: PANIC: invalid index offnum: 186 when processing BRIN indexes in VACUUM

[Alvaro Herrera]

Tom Lane wrote:
> Alvaro Herrera  writes:

> > Rather than remove the capability, I'd be inclined to make
> > brin_summarize_new_values summarize the final partial range, and have
> > VACUUM not do it.  Would that be too inconsistent?
> 
> That doesn't really get you out of the problem that this is an abuse of
> the relation extension lock, and is likely to cause issues when people
> try to optimize that locking mechanism.

Right.

> Why is it that the regular technique doesn't work, ie create a placeholder
> tuple and let it get added to by any insertions that happen?

The problem is that we determine relation size (and scan stop point)
before inserting the placeholder tuple, so any relation extension that
occurs after we read the size is not covered by the scan.  The reason we
do this is to avoid calling RelationGetNumberOfBlocks once for each
range, since it's known to be very expensive.

Maybe a solution is to call RelationGetNumberOfBlocks() after the
placeholder tuple has been inserted, for the case where we would be
scanning past end of relation; passing InvalidBlockNumber as stop point
would indicate to do things that way.  I'll try with that approach 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] [bug fix] postgres.exe crashes with access violation on Windows while starting up

[Michael Paquier]

On Wed, Nov 1, 2017 at 12:07 AM, Tsunakawa, Takayuki
 wrote:
> From: Michael Paquier [mailto:michael.paqu...@gmail.com]
>> On Tue, Oct 31, 2017 at 6:59 AM, Tsunakawa, Takayuki
>>  wrote:
>> > When CurrentMemoryContext is NULL, the message  is logged with
>> ReportEventA().  This is similar to write_console().
>>
>> My point is that as Postgres is running as a service, isn't it wrong to
>> write a message to stderr as a fallback if the memory context is not set?
>> You would lose a message. It seems to me that for an operation that can
>> happen at a low-level like the postmaster startup, we should really use
>> a low-level operation as well.
>
> I'm sorry I may not have been clear.  With this patch, write_eventlog() 
> outputs the message to event log with ReportEventA() when 
> CurrentMemoryContext is NULL.  It doesn't write to stderr.  So the message 
> won't be lost.

Oh, yes. Sorry. I got confused a bit by write_eventlog(), which is
already doing what your patch is adding for write_eventlog(). I am
switching the patch as ready for committer, I definitely agree that
you are taking the write approach here.

I am also adding Noah to get some input on this issue, as he is the
author and committer of 5f538ad0 which has improved the handling of
non-ASCII characters in this code path, and more importantly has
tweaked 43adc7a7 to handle properly transaction contexts in
pgwin32_message_to_UTF16() which is where the palloc calls happen. I
would be the one in the pool of committers who would most likely
commit your patch.
-- 
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] Linking libpq statically to libssl

[Stephen Frost]

* Stephen Frost (sfr...@snowman.net) wrote:
> and we've certainly not spent effort that I've seen to try to actually
> make libpq work when multiple versions of libpq are linked into the same
> running backend.

... errr, same running application, that is, not backend.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] MERGE SQL Statement for PG11

[Stephen Frost]

* Robert Haas (robertmh...@gmail.com) wrote:
> On Fri, Nov 3, 2017 at 1:05 PM, Simon Riggs  wrote:
> > We seem to have a few options for PG11
> >
> > 1. Do nothing, we reject MERGE
> >
> > 2. Implement MERGE for unique index situations only, attempting to
> > avoid errors (Simon OP)
> >
> > 3. Implement MERGE, but without attempting to avoid concurrent ERRORs 
> > (Peter)
> >
> > 4. Implement MERGE, while attempting to avoid concurrent ERRORs in
> > cases where that is possible.
> >
> > Stephen, Robert, please say which option you now believe we should pick.
> 
> I think Peter has made a good case for #3, so I lean toward that
> option.  I think #4 is too much of a non-obvious behavior difference
> between the cases where we can avoid those errors and the cases where
> we can't, and I don't see where #2 can go in the future other than #4.

Agreed.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Client Connection redirection support for PostgreSQL

[Satyanarayana Narlapuram]

> pg_hba.conf is "host based access [control]" . I'm not sure it's really the 
> right place.
I am open to have another configuration file, say routing_list.conf to define 
the routing rules, but felt it is easy to extend the hba conf file.

> But we now have a session-intent stuff though. So we could possibly do it at 
> session level.
Session intent can be used as an obvious hint for the routing to kick in. This 
can be a rule in the routing list to route the read intent sessions round robin 
across multiple secondary replicas.

> Backends used just for a redirect would be pretty expensive though.
It is somewhat expensive as the new process fork has to happen for each new 
connection. The advantage is that it makes proxies optional (if the middle tier 
can do connection management), and all the routing configurations can be within 
the server.
This also benefits latency sensitive applications not going through proxy.


-- 
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] Client Connection redirection support for PostgreSQL

[Satyanarayana Narlapuram]

> What advantages do you see in doing this in the backend over the current 
> system where the concerns are separated, i.e. people use connection poolers 
> like pgbouncer to do the routing?
IMHO connection pooler is not great for latency sensitive applications. For 
small deployments, proxy is an overhead. For example, in the cloud environment, 
the proxy has to sit in one data center / region and has to server the client 
requests serving from other data centers.

> Would it make sense also to include an optional routing algorithm or pointer 
> to a routing function for each RoutingList, or do you see this as entirely 
> the client's responsibility?
This is a great point, I haven't put much though into this beyond round robin / 
random shuffling. Providing the priority list of endpoints to the client from 
the server will allow client connections balanced accordingly. However, it is 
up to the client implementation to honor the list.

> How does this work with SSL?
The protocol doesn't change much with SSL, and after the handshake, startup 
message is sent to the server from the client, and the new message flow kicks 
on the server based on the routing list.

>>   1.  Bumping the protocol version - old server instances may not understand 
>> the new client protocol
> This sounds more attractive, assuming that the feature is.
I agree, bumping the protocol version makes things simple.

> > 3.  The current proposal - to keep it in the hba.conf and let the
> > server admin deal with the configuration by taking conscious
> > choice on the configuration of routing list based on the clients
>>   connecting to the server instance.

>How would clients identify themselves as eligible without a protocol version 
>bump?
Either through optional parameter, or controlled configuration by the server 
administrator are the only choices.
Protocol bump seems to me is a better idea here.

> So to DoS the server, what's required is a flock of old clients?  I presume 
> there's a good reason to reroute rather than serve these requests.
Possible, but I would say the server admin understands where the requests are 
coming from (old / new client) and does the capacity planning accordingly.

> Comments and feedback have begun.
Thank you :)

Thanks,
Satya


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

Re: [HACKERS] Re: PANIC: invalid index offnum: 186 when processing BRIN indexes in VACUUM

[Tomas Vondra]

Hi,

On 11/02/2017 06:45 PM, Alvaro Herrera wrote:
> Tomas Vondra wrote:
> 
>> Unfortunately, I think we still have a problem ... I've been wondering
>> if we end up producing correct indexes, so I've done a simple test.
> 
> Here's a proposed patch that should fix this problem (and does, in my
> testing).  Would you please give it a try?
> 
> This patch changes two things:
> 
> 1. in VACUUM or brin_summarize_new_values, we only process fully loaded
> ranges, and ignore the partial range at end of table.
> 
> 2. when summarization is requested on the partial range at the end of a
> table, we acquire extension lock on the rel, then compute relation size
> and run summarization with the lock held.  This guarantees that we don't
> miss any pages.  This is bad for concurrency though, so it's only done
> in that specific scenario.
> 

FWIW this patch fixes the issue for me - I can no longer reproduce the
bitmapscan vs. seqscan result discrepancies (even with the extra UPDATE
phase).

regards

-- 
Tomas Vondra  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] Try to fix endless loop in ecpg with informix mode

[Michael Meskes]

> Although this error message is not wrong, I think it should be better
> to
> give error message as "invalid input syntax for type int" for "7.a".
> This could be done by delete "return false;" after "while(...)", let
> the following if to decide which to return.

Ah, now I understand. Sorry, I completely misunderstood your prior
email. Yes, you're right, I will change the code accordingly.

Thanks.

Michael
-- 
Michael Meskes
Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org)
Meskes at (Debian|Postgresql) dot Org
Jabber: michael at xmpp dot meskes dot org
VfL Borussia! Força Barça! SF 49ers! Use Debian GNU/Linux, PostgreSQL


-- 
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] How to implement a SP-GiST index as a extension module?

[Connor Wolf]

Yeah, unfortunately, the way these type of metric trees work, the entire
search procedure is a function of both the target value and the allowed
search distance. The only way I can think of to return ordered results
without just scanning the entire index would be to repeatedly search the
index while gradually incrementing the allowed search distance (which is
horrible).

>From looking at some of the contrib modules, the way other index libraries
that have similar needs manage it is by implementing custom types that
encapsulate the filter parameters. The sp-gist kd-tree and quadtree indexes
store point coordinates in n-dimensional space, but they (ab)use the BOX
type because it's a convenient way of passing multiple values into the
value parameter of the index query.

I'm thinking at this point, I'm mostly stuck having to define a custom type
to encapsulate the relevant parameters. Really, the filter condition is a
integer 2-tuple, so I wonder if I could do something horrible with the
array type. If the value parameter for the query could be a bigint
array[2], that would work, and I'd just have to remember the ordering.

Does that seem viable?


EDIT: That's actually exactly how the example I'm working off of works.
DERP. The SQL is

CREATE TYPE vptree_area AS
(
center _int4,
distance float8
);

CREATE OR REPLACE FUNCTION vptree_area_match(_int4, vptree_area) RETURNS
boolean AS
'MODULE_PATHNAME','vptree_area_match'
LANGUAGE C IMMUTABLE STRICT;

CREATE OPERATOR <@ (
LEFTARG = _int4,
RIGHTARG = vptree_area,
PROCEDURE = vptree_area_match,
RESTRICT = contsel,
JOIN = contjoinsel);

so I just need to understand how to parse out the custom type in my index
operator.
--

Sorry if I'm asking a lot of dumb questions. Postgresql is huge and I have
no idea what I'm really doing.

On Fri, Nov 3, 2017 at 12:20 AM, Robert Haas  wrote:

> On Thu, Nov 2, 2017 at 9:53 AM, Connor Wolf
>  wrote:
> > As such:
> > Will compound queries as I describe above basically require a custom
> type to
> > make it possible? My (admittedly naive) expectation
> > is that the eventual query for this index will look something like
> "SELECT *
> > FROM example_table WHERE indexed_column <=> target_value < 4;",
> > with "<=>" being the operator for the relevant distance calculation
> > (hamming, for the BK tree, numeric for the VP-tree).
> >
> > The existing VP-tree code appears to not support multiple operators
> > whatsoever, probably because it was very preliminary.
>
> I'm not an expert in this area in any way whatsoever; I don't know a
> VP-tree from a BK-tree from a maple tree.
>
> However, I can tell you that as a general rule, PostgreSQL index
> access methods can only apply index quals of the form "WHERE column op
> value" or ordering criteria of the form "ORDER BY column op value".
> So, in the above example, you might think about trying to set up the
> access method so that it can efficiently return values ordered by
> indexed_column <=> target_value and then wrapping the ORDER BY query
> in a subselect to cut off fetching values at the correct point.  But
> no operator class for any access method can directly handle that query
> efficiently as you've written it.
>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>

Re: [HACKERS] MERGE SQL Statement for PG11

[Simon Riggs]

On 3 November 2017 at 07:46, Thomas Kellerer  wrote:
> PMFJI
>
>> We seem to have a few options for PG11
>>
>> 1. Do nothing, we reject MERGE
>>
>> 2. Implement MERGE for unique index situations only, attempting to
>> avoid errors (Simon OP)
>>
>> 3. Implement MERGE, but without attempting to avoid concurrent ERRORs
>> (Peter)
>>
>> 4. Implement MERGE, while attempting to avoid concurrent ERRORs in
>> cases where that is possible.
>
> From an end-users point of view I would prefer 3 (or 4 if that won't prevent
> this from going into 11)

Sounds reasonable approach.

> INSERT ... ON CONFLICT is great, but there are situations where the
> restrictions can get in the way and it would be nice to have an alternative
> - albeit with some (documented) drawbacks. As far as I know Oracle also
> doesn't guarantee that MERGE is safe for concurrent use - you can still wind
> up with a unique key violation.

Yes, Oracle allows some unique key violations. It's clear that we
would need to allow some also. So we clearly can't infer whether they
avoid some errors just because they allow some.

My approach will be to reduce the errors in the best way, not to try
to copy errors Oracle makes, if any. But that error avoidance can
easily be a later add-on if we prefer it that way.

-- 
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] MERGE SQL Statement for PG11

[Robert Haas]

On Fri, Nov 3, 2017 at 1:05 PM, Simon Riggs  wrote:
>> Therefore, if MERGE eventually uses INSERT .. ON CONFLICT
>> UPDATE when a relevant unique index exists and does something else,
>> such as your proposal of taking a strong lock, or Peter's proposal of
>> doing this in a concurrency-oblivious manner, in other cases, then
>> those two cases will behave very differently.
>
> The *only* behavioural difference I have proposed would be the *lack*
> of an ERROR in (some) concurrent cases.

I think that's a big difference.  Error vs. non-error is a big deal by
itself; also, the non-error case involves departing from MVCC
semantics just as INSERT .. ON CONFLICT UPDATE does.

> All I have at the moment is that a few people disagree, but that
> doesn't help determine the next action.
>
> We seem to have a few options for PG11
>
> 1. Do nothing, we reject MERGE
>
> 2. Implement MERGE for unique index situations only, attempting to
> avoid errors (Simon OP)
>
> 3. Implement MERGE, but without attempting to avoid concurrent ERRORs (Peter)
>
> 4. Implement MERGE, while attempting to avoid concurrent ERRORs in
> cases where that is possible.
>
> Stephen, Robert, please say which option you now believe we should pick.

I think Peter has made a good case for #3, so I lean toward that
option.  I think #4 is too much of a non-obvious behavior difference
between the cases where we can avoid those errors and the cases where
we can't, and I don't see where #2 can go in the future other than #4.

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


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

Re: [HACKERS] dropping partitioned tables without CASCADE

[Alvaro Herrera]

Amit Langote wrote:
> On 2017/09/06 19:14, Amit Langote wrote:
> > On 2017/09/06 18:46, Rushabh Lathia wrote:
> >> Okay, I have marked this as ready for committer.
> > 
> > Thanks Ashutosh and Rushabh for rebasing and improving the patch.  Looks
> > good to me too.
> 
> Patch needed to be rebased after the default partitions patch went in, so
> done.  Per build status on http://commitfest.cputube.org :)

I think adding "is partitioned" at end of line isn't good; looks like a
phrase but isn't translatable.  Maybe add keyword PARTITIONED instead?

Having the DEFAULT partition show up in the middle of the list is weird.
Is it possible to put it at either start or end of the list?

-- 
Á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] MERGE SQL Statement for PG11

[Thomas Kellerer]

PMFJI

> We seem to have a few options for PG11
> 
> 1. Do nothing, we reject MERGE
> 
> 2. Implement MERGE for unique index situations only, attempting to
> avoid errors (Simon OP)
> 
> 3. Implement MERGE, but without attempting to avoid concurrent ERRORs
> (Peter)
> 
> 4. Implement MERGE, while attempting to avoid concurrent ERRORs in
> cases where that is possible.

>From an end-users point of view I would prefer 3 (or 4 if that won't prevent
this from going into 11) 

INSERT ... ON CONFLICT is great, but there are situations where the
restrictions can get in the way and it would be nice to have an alternative
- albeit with some (documented) drawbacks. As far as I know Oracle also
doesn't guarantee that MERGE is safe for concurrent use - you can still wind
up with a unique key violation. 





--
Sent from: http://www.postgresql-archive.org/PostgreSQL-hackers-f1928748.html


-- 
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] MERGE SQL Statement for PG11

[Simon Riggs]

On 2 November 2017 at 22:59, Nico Williams  wrote:
> On Thu, Nov 02, 2017 at 03:25:48PM -0700, Peter Geoghegan wrote:
>> Nico Williams  wrote:
>> >A MERGE mapped to a DML like this:
>>
>> This is a bad idea. An implementation like this is not at all
>> maintainable.
>
> Assuming the DELETE issue can be addressed, why would this not be
> maintainable?

It would only take one change to make this approach infeasible and
when that happened we would need to revert to the full-executor
version.

One difference that comes to mind is that MERGE doesn't behave the
same way as an UPDATE-join, according to SQL:2011 in that it must
throw an error if duplicate changes are requested. That would be hard
to emulate using a parser only version.

I would call it impressively clever but likely fragile, in this case,
though I encourage more ideas like that in the future.

-- 
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] MERGE SQL Statement for PG11

[Simon Riggs]

On 2 November 2017 at 17:06, Robert Haas  wrote:
> On Tue, Oct 31, 2017 at 5:14 PM, Simon Riggs  wrote:
>> I've proposed a SQL Standard compliant implementation that would do
>> much more than be new syntax over what we already have.
>>
>> So these two claims aren't accurate: "radical difference" and "syntax
>> sugar over a capability we have".
>
> I think those claims are pretty accurate.

No, because there is misunderstanding on some technical points.

They key point is at the end - what do we do next?

> The design of INSERT .. ON CONFLICT UPDATE and the supporting
> machinery only work if there is a unique index.  It provides radically
> different behavior than what you get with any other DML statement,
> with completely novel concurrency behavior, and there is no reasonable
> way to emulate that behavior in cases where no relevant unique index
> exists.

Agreed

> Therefore, if MERGE eventually uses INSERT .. ON CONFLICT
> UPDATE when a relevant unique index exists and does something else,
> such as your proposal of taking a strong lock, or Peter's proposal of
> doing this in a concurrency-oblivious manner, in other cases, then
> those two cases will behave very differently.

The *only* behavioural difference I have proposed would be the *lack*
of an ERROR in (some) concurrent cases.

We have a way to avoid some concurrency errors during MERGE, while
still maintaining exact SQL Standard behaviour. Peter has pointed out
that we could not catch errors in certain cases; that does nothing to
the cases where it will work well. It would be fallacious to imagine
it is an all or nothing situation.

> And if, in the meantime, MERGE can only handle the cases where there
> is a unique index,

I haven't proposed that MERGE would be forever limited to cases with a
unique index, only that MERGE-for-unique-indexes would be the version
in PG11, for good reason.

> then it can only handle the cases INSERT .. ON
> CONFLICT UPDATE can cover, which makes it, as far as I can see,
> syntactic sugar over what we already have.

SQL:2011 is greatly enhanced and this is no longer true; it was in
earlier versions but is not now.

MERGE allows you do DELETE, as well as conditional UPDATE and INSERT.
It is very clearly much more than UPSERT.

> Maybe it's not entirely -
> you might be planning to make some minor functional enhancements - but
> it's not clear what those are, and I feel like whatever it is could be
> done with less work and more elegance by just extending the INSERT ..
> ON CONFLICT UPDATE syntax.

The differences are clearly apparent in the Standard and in my submitted docs.

But I am interested in submitting a SQL Standard compliant feature.

> And it does seem to be your intention to only handle the cases which
> the INSERT .. ON CONFLICT UPDATE infrastructure can cover, because
> upthread you wrote this: "I didn't say it but my intention was to just
> throw an ERROR if no single unique index can be identified."  I don't
> think anybody's putting words into your mouth here.  We're just
> reading what you wrote.

Happy to have written it because that covers the common use case I was
hoping to deliver in PG11. Incremental development.

My proposal was to implement the common case, while avoiding
concurrency errors. Peter proposes that I cover both the common case
and more general cases, without avoiding concurrency errors.


All I have at the moment is that a few people disagree, but that
doesn't help determine the next action.

We seem to have a few options for PG11

1. Do nothing, we reject MERGE

2. Implement MERGE for unique index situations only, attempting to
avoid errors (Simon OP)

3. Implement MERGE, but without attempting to avoid concurrent ERRORs (Peter)

4. Implement MERGE, while attempting to avoid concurrent ERRORs in
cases where that is possible.

Stephen, Robert, please say which option you now believe we should pick.

-- 
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] How to implement a SP-GiST index as a extension module?

[Robert Haas]

On Thu, Nov 2, 2017 at 9:53 AM, Connor Wolf
 wrote:
> As such:
> Will compound queries as I describe above basically require a custom type to
> make it possible? My (admittedly naive) expectation
> is that the eventual query for this index will look something like "SELECT *
> FROM example_table WHERE indexed_column <=> target_value < 4;",
> with "<=>" being the operator for the relevant distance calculation
> (hamming, for the BK tree, numeric for the VP-tree).
>
> The existing VP-tree code appears to not support multiple operators
> whatsoever, probably because it was very preliminary.

I'm not an expert in this area in any way whatsoever; I don't know a
VP-tree from a BK-tree from a maple tree.

However, I can tell you that as a general rule, PostgreSQL index
access methods can only apply index quals of the form "WHERE column op
value" or ordering criteria of the form "ORDER BY column op value".
So, in the above example, you might think about trying to set up the
access method so that it can efficiently return values ordered by
indexed_column <=> target_value and then wrapping the ORDER BY query
in a subselect to cut off fetching values at the correct point.  But
no operator class for any access method can directly handle that query
efficiently as you've written it.

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


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

Re: [HACKERS] Try to fix endless loop in ecpg with informix mode

[高增琦]

Hi,

I found the last commit changed as:

```
/* skip invalid characters */
do {
(*scan_length)++;
-   } while (**scan_length != ' ' && **scan_length != '\0' &&
isdigit(**scan_length));
+   } while (isdigit(**scan_length));
return false;
}
```

It will still return false if we got non-digital characters after ".",
then it will error out "invalid input syntax for type int" for "a" . (if
input is "7.a")

Although this error message is not wrong, I think it should be better to
give error message as "invalid input syntax for type int" for "7.a".
This could be done by delete "return false;" after "while(...)", let
the following if to decide which to return.


2017-11-02 15:25 GMT+08:00 Michael Meskes :

> > I am afraid the changes may separate "7.a" to "7" and "a", then error
> > out
> > with "invalid input syntax for type int" for "a".
>
> Which is correct, is it not?
>
> > How about changes as below? (use following the if to decide true or
> > false)
> > ...
> >return false;
> > +} while (isdigit(**scan_length));
>
> Yes, this is certainly correct and better than what I committed. What
> was I thinking yesterday?
>
> I think the same function is used for identifying garbage in floats
> which might ask for different logic. Let me check.
>
> Michael
> --
> Michael Meskes
> Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org)
> Meskes at (Debian|Postgresql) dot Org
> Jabber: michael at xmpp dot meskes dot org
> VfL Borussia! Força Barça! SF 49ers! Use Debian GNU/Linux, PostgreSQL
>



-- 
GaoZengqi
pgf...@gmail.com
zengqi...@gmail.com

Re: [HACKERS] ArrayLists instead of List (for some things)

[Craig Ringer]

On 3 November 2017 at 12:41, David Rowley  wrote:
> On 3 November 2017 at 03:26, Craig Ringer  wrote:
>> On 2 November 2017 at 22:22, David Rowley  
>> wrote:
>>> Maybe, but the new implementation is not going to do well with places
>>> where we perform lcons(). Probably many of those places could be
>>> changed to lappend(), but I bet there's plenty that need prepend.
>>
>> Yeah, and it's IMO significant that pretty much every nontrivial
>> system with ADTs offers multiple implementations of list data
>> structures, often wrapped with a common API.
>>
>> Java's Collections, the STL, you name it.
>
> I've never really looked at much Java, but I've done quite a bit of
> dotnet stuff in my time and they have a common interface ICollection
> and various data structures that implement that interface. Which is
> implemented by a bunch of classes, something like
>
> ConcurrentDictionary (hash table)
> Dictionary (hash table)
> HashSet (hash table)
> LinkedList (some kinda linked list)
> List (Arrays, probably)
> SortedDictionary (bst, I think)
> SortedList (no idea, perhaps a btree)
> SortedSet
> Bag (no idea, but does not care about any order)
>
> Probably there are more, but the point is that they obviously don't
> believe there's a one-size-fits-all type. I don't think we should do
> that either. However, I've proved nothing on the performance front
> yet, so that should probably be my next step.

Right. If you've done C# you've done cleaned-up, modernized Java.

C# and Java both have an advantage we don't, namely JIT compilation,
so they can afford to do more with common interfaces then do runtime
specialization. As a pure C project we don't really have that option,
so it's unlikely to be efficient for us to have a fully common
interface and dynamic dispatch. Even C++ only gets away with its
common interfaces in the STL because it does compile-time
specialization via templates, non-virtual methods, etc. There's only
so much you can perpetrate with macros and rely on the compiler to
clean up before it just gets awful.

So I imagine if we do land up with a couple of collections we'd
probably have an AList, alcons, alappend, etc. Bit of a pain, but
hardly the end of the world.

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


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