Re: FETCH FIRST clause PERCENT option

[Surafel Temesgen]

On Tue, Jan 1, 2019 at 10:08 PM Tomas Vondra 
wrote:

> The execution part of the patch seems to be working correctly, but I
> think there's an improvement - we don't need to execute the outer plan
> to completion before emitting the first row. For example, let's say the
> outer plan produces 1 rows in total and we're supposed to return the
> first 1% of those rows. We can emit the first row after fetching the
> first 100 rows, we don't have to wait for fetching all 10k rows.
>
>
my other concern is in this case we are going to determine limit and safe
row to return on the fly
which have additional computation and i fair that it will not perform
better on most of the case

regards
Surafel

Re: FETCH FIRST clause PERCENT option

[Surafel Temesgen]

On Thu, Jan 3, 2019 at 4:51 PM Tomas Vondra 
wrote:

>
> On 1/3/19 1:00 PM, Surafel Temesgen wrote:
> > Hi
> >
> > On Tue, Jan 1, 2019 at 10:08 PM Tomas Vondra
> > mailto:tomas.von...@2ndquadrant.com>>
> wrote:
>
> > The execution part of the patch seems to be working correctly, but I
> > think there's an improvement - we don't need to execute the outer
> plan
> > to completion before emitting the first row. For example, let's say
> the
> > outer plan produces 1 rows in total and we're supposed to return
> the
> > first 1% of those rows. We can emit the first row after fetching the
> > first 100 rows, we don't have to wait for fetching all 10k rows.
> >
> >
> > but total rows count is not given how can we determine safe to return row
> >
>
> But you know how many rows were fetched from the outer plan, and this
> number only grows grows. So the number of rows returned by FETCH FIRST
> ... PERCENT also only grows. For example with 10% of rows, you know that
> once you reach 100 rows you should emit ~10 rows, with 200 rows you know
> you should emit ~20 rows, etc. So you may track how many rows we're
> supposed to return / returned so far, and emit them early.
>
>
>

yes that is clear but i don't find it easy to put that in formula. may be
someone with good mathematics will help

regards
Surafel

Re: SQL/JSON: functions

[Andrew Alsup]

> Attached patches implementing all SQL/JSON functions excluding 
JSON_TABLE:

>
> JSON_OBJECT()
> JSON_OBJECTAGG()
> JSON_ARRAY()
> JSON_ARRAYAGG()
>
> JSON_EXISTS()
> JSON_VALUE()
> JSON_QUERY()

Sorry if this is a stupid question, but is this patch intended to 
implement any SQL/JSON functions? I'm basing this question on the 
original patch post (quoted above). The patch appears to be focused 
exclusively on "jsonpath expressions". I only ask because I think I 
misinterpreted and spent some time wondering if I had missed a patch file.


So far, the jsonpath docs are very readable; however, I think a few 
complete examples (full SELECT statements) of using jsonpath expressions 
would be helpful to someone new to this technology. Does postgresql 
provide a sample schema, similar to the Or*cle scott/tiger (emp/dept) 
schema, we could use for examples? Alternatively, we could reference 
something mentioned at 
https://wiki.postgresql.org/wiki/Sample_Databases. I think it would be 
nice if all the docs referenced the same schema, when possible.


For tests, would it be helpful to have some tests that 
demonstrate/assert equality between "jsonb operators" and "jsonpath 
expressions"? For example, using the existing regression test data the 
following should assert equality in operator vs. expression:


SELECT
  CASE WHEN jop_count = expr_count THEN 'pass' ELSE 'fail' END
FROM
  (
    -- jsonb operator
    SELECT count(*)
    FROM testjsonb
    WHERE j->>'abstract' LIKE 'A%'
  ) as jop_count,
  (
    -- jsonpath expression
    SELECT count(*)
    FROM testjsonb
    WHERE j @? '$.abstract ? (@ starts with "A")'
  ) as expr_count;

Re: add_partial_path() may remove dominated path but still in use

[Kohei KaiGai]

I tried to make a patch to have dual hooks at set_rel_pathlist(), and
adjusted PG-Strom for the new design. It stopped to create GatherPath
by itself, just added a partial path for the base relation.
It successfully made a plan using parallel custom-scan node, without
system crash.

As I mentioned above, it does not use the new "post_rel_pathlist_hook"
because we can add both of partial/regular path-node at the first hook
with no particular problems.

Thanks,

dbt3=# explain select
sum(l_extendedprice * l_discount) as revenue
from
lineitem
where
l_shipdate >= date '1994-01-01'
and l_shipdate < cast(date '1994-01-01' + interval '1 year' as date)
and l_discount between 0.08 - 0.01 and 0.08 + 0.01
and l_quantity < 24 limit 1;
  QUERY PLAN

 Limit  (cost=144332.62..144332.63 rows=1 width=4)
   ->  Aggregate  (cost=144332.62..144332.63 rows=1 width=4)
 ->  Gather  (cost=144285.70..144329.56 rows=408 width=4)
   Workers Planned: 2
   ->  Parallel Custom Scan (GpuPreAgg) on lineitem
(cost=143285.70..143288.76 rows=204 width=4)
 Reduction: NoGroup
 Outer Scan: lineitem  (cost=1666.67..143246.16
rows=63254 width=8)
 Outer Scan Filter: ((l_discount >= '0.07'::double
precision) AND
   (l_discount <=
'0.09'::double precision) AND
   (l_quantity <
'24'::double precision) AND
   (l_shipdate <
'1995-01-01'::date) AND
   (l_shipdate >=
'1994-01-01'::date))
(8 rows)

Thanks,

2019年1月2日(水) 22:34 Kohei KaiGai :
>
> 2018年12月31日(月) 22:25 Amit Kapila :
> >
> > On Mon, Dec 31, 2018 at 5:48 PM Kohei KaiGai  wrote:
> > >
> > > 2018年12月31日(月) 13:10 Amit Kapila :
> > > >
> > > > On Sun, Dec 30, 2018 at 9:01 AM Kohei KaiGai  
> > > > wrote:
> > > > > 2018年12月30日(日) 4:12 Tom Lane :
> > > > > >
> > > > > > Kohei KaiGai  writes:
> > > > > > > 2018年12月29日(土) 1:44 Tom Lane :
> > > > > > >> However, first I'd like to know why this situation is arising in 
> > > > > > >> the first
> > > > > > >> place.  To have the situation you're describing, we'd have to 
> > > > > > >> have
> > > > > > >> attempted to make some Gather paths before we have all the 
> > > > > > >> partial paths
> > > > > > >> for the relation they're for.  Why is that a good thing to do?  
> > > > > > >> It seems
> > > > > > >> like such Gathers are necessarily being made with incomplete 
> > > > > > >> information,
> > > > > > >> and we'd be better off to fix things so that none are made till 
> > > > > > >> later.
> > > > > >
> > > > > > > Because of the hook location, Gather-node shall be constructed 
> > > > > > > with built-in
> > > > > > > and foreign partial scan node first, then extension gets a chance 
> > > > > > > to add its
> > > > > > > custom paths (partial and full).
> > > > > > > At the set_rel_pathlist(), set_rel_pathlist_hook() is invoked 
> > > > > > > next to the
> > > > > > > generate_gather_paths().
> > > > > >
> > > > > > Hmm.  I'm inclined to think that we should have a separate hook
> > > > > > in which extensions are allowed to add partial paths, and that
> > > > > > set_rel_pathlist_hook should only be allowed to add regular paths.
> > > >
> > > > +1.  This idea sounds sensible to me.
> > > >
> > > > > >
> > > > > I have almost same opinion, but the first hook does not need to be
> > > > > dedicated for partial paths. As like set_foreign_pathlist() doing, we 
> > > > > can
> > > > > add both of partial and regular paths here, then 
> > > > > generate_gather_paths()
> > > > > may generate a Gather-path on top of the best partial-path.
> > > > >
> > > >
> > > > Won't it be confusing for users if we allow both partial and full
> > > > paths in first hook and only full paths in the second hook?
> > > > Basically, in many cases, the second hook won't be of much use.  What
> > > > advantage you are seeing in allowing both partial and full paths in
> > > > the first hook?
> > > >
> > > Two advantages. The first one is, it follows same manner of
> > > set_foreign_pathlist()
> > > which allows to add both of full and partial path if FDW supports 
> > > parallel-scan.
> > > The second one is practical. During the path construction, extension 
> > > needs to
> > > check availability to run (e.g, whether operators in WHERE-clause is 
> > > supported
> > > on GPU device...), calculate its estimated cost and so on. Not a small
> > > portion of
> > > them are common for both of full and partial path. So, if the first
> > > hook accepts to
> > > add both kind of paths at once, extension can share the common properties.
> > >
> >
> > You have a point, though I am not sure how much difference it can
>

Re: WIP: Avoid creation of the free space map for small tables

[Amit Kapila]

On Fri, Jan 4, 2019 at 8:23 AM Mithun Cy  wrote:
> Thanks,
>
> On Sun, Dec 30, 2018 at 3:49 AM John Naylor  wrote:
> > On 12/29/18, Mithun Cy  wrote:
> > > Results are execution time(unit ms) taken by copy statement when number of
> > > records  equal to exact number which fit HEAP_FSM_CREATION_THRESHOLD = 4
> > > pages. For fill factor 20 it is till tid (3, 43) and for scale factor 70
> > > till tid (3, 157). Result is taken as a median of 10 runs.
> >
> > > So 2-3% consistent regression, And on every run I can see for patch v11
> > > execution time is slightly more than base.
> >
> > Thanks for testing!
> >
> > > I also tried to insert more
> > > records till 8 pages and same regression is observed! So I guess even
> > > HEAP_FSM_CREATION_THRESHOLD = 4 is not perfect!
> >
> > That's curious, because once the table exceeds the threshold, it would
> > be allowed to update the FSM, and in the process write 3 pages that it
> > didn't have to in the 4 page test. The master branch has the FSM
> > already, so I would expect the 8 page case to regress more.
>
> I tested with configuration HEAP_FSM_CREATION_THRESHOLD = 4 and just
> tried to insert till 8 blocks to see if regression is carried on with
> further inserts.
>
> > What I can do later is provide a supplementary patch to go on top of
> > mine that only checks the last block. If that improves performance,
> > I'll alter my patch to only check every other page.
>
> Running callgrind for same test shows below stats
> Before patch
> ==
> Number of callsfunction_name
> 2000 heap_multi_insert
> 2000 RelationGetBufferForTuple
> 3500 ReadBufferBI
>
> After Patch
> =
> Number of callsfunction_name
> 2000 heap_multi_insert
> 2000 RelationGetBufferForTuple
> 5000 ReadBufferBI
>
> I guess Increase in ReadBufferBI() calls might be the reason which is
> causing regression. Sorry I have not investigated it.
>

I think the reason is that we are checking each block when blocks are
less than HEAP_FSM_CREATION_THRESHOLD.  Even though all the blocks are
in memory, there is some cost to check them all.  OTOH, without the
patch, even if it accesses FSM, it won't have to make so many
in-memory reads for blocks.

BTW, have you check for scale_factor 80 or 100 as suggested last time?

> I will check
> same with your next patch!
>

Yeah, that makes sense, John, can you provide a patch on top of the
current patch where we check either the last block or every other
block.

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

Re: Logical decoding for operations on zheap tables

[Andres Freund]

Hi,

On 2019-01-04 08:54:34 +0530, Amit Kapila wrote:
> On Thu, Jan 3, 2019 at 11:30 PM Andres Freund  wrote:
> > On 2018-12-31 09:56:48 +0530, Amit Kapila wrote:
> > > To support logical decoding for zheap operations, we need a way to
> > > ensure zheap tuples can be registered as change streams.   One idea
> > > could be that we make ReorderBufferChange aware of another kind of
> > > tuples as well, something like this:
> > >
> > > @@ -100,6 +123,20 @@ typedef struct ReorderBufferChange
> > >   ReorderBufferTupleBuf *newtuple;
> > >   } tp;
> > > + struct
> > > + {
> > > + /* relation that has been changed */
> > > + RelFileNode relnode;
> > > +
> > > + /* no previously reassembled toast chunks are necessary anymore */
> > > + bool clear_toast_afterwards;
> > > +
> > > + /* valid for DELETE || UPDATE */
> > > + ReorderBufferZTupleBuf *oldtuple;
> > > + /* valid for INSERT || UPDATE */
> > > + ReorderBufferZTupleBuf *newtuple;
> > > + } ztp;
> > > +
> > >
> > >
> > > +/* an individual zheap tuple, stored in one chunk of memory */
> > > +typedef struct ReorderBufferZTupleBuf
> > > +{
> > > ..
> > > + /* tuple header, the interesting bit for users of logical decoding */
> > > + ZHeapTupleData tuple;
> > > ..
> > > +} ReorderBufferZTupleBuf;
> > >
> > > Apart from this, we need to define different decode functions for
> > > zheap operations as the WAL data is different for heap and zheap, so
> > > same functions can't be used to decode.
> >
> > I'm very strongly opposed to that. We shouldn't have expose every
> > possible storage method to output plugins, that'll make extensibility
> > a farce.  I think we'll either have to re-form a HeapTuple or decide
> > to bite the bullet and start exposing tuples via slots.
> >
> 
> To be clear, you are against exposing different format of tuples to
> plugins, not having different decoding routines for other storage
> engines, because later part is unavoidable due to WAL format.

Correct.


> Now,
> about tuple format, I guess it would be a lot better if we expose via
> slots, but won't that make existing plugins to change the way they
> decode the tuple, maybe that is okay?

I think one-off API changes are ok. What I'm strictly against is
primarily that output plugins will have to deal with more and more
different tuple formats.


> OTOH, re-forming the heap tuple
> has a cost which might be okay for the time being or first version,
> but eventually, we want to avoid that.

Right.


> The other reason why I refrained from tuple conversion was that I
> was not sure if we anywhere rely on the transaction information in
> the tuple during decode process, because that will be tricky to
> mimic, but I guess we don't check that.

Shouldn't be necessary - in fact, most of that information isn't in
the heap wal records in the first place.


> The only point for exposing a different tuple format via plugin was a
> performance which I think can be addressed if we expose via slots.  I
> don't want to take up exposing slots instead of tuples for plugins as
> part of this project and I think if we want to go with that, it is
> better done as part of pluggable API?

No, I don't think it makes sense to address this is as part of
pluggable storage. That patchset is already way too invasive and
large.

Greetings,

Andres Freund

Re: Logical decoding for operations on zheap tables

[Amit Kapila]

On Thu, Jan 3, 2019 at 11:30 PM Andres Freund  wrote:
>
> Hi,
>
> On 2018-12-31 09:56:48 +0530, Amit Kapila wrote:
> > To support logical decoding for zheap operations, we need a way to
> > ensure zheap tuples can be registered as change streams.   One idea
> > could be that we make ReorderBufferChange aware of another kind of
> > tuples as well, something like this:
> >
> > @@ -100,6 +123,20 @@ typedef struct ReorderBufferChange
> >   ReorderBufferTupleBuf *newtuple;
> >   } tp;
> > + struct
> > + {
> > + /* relation that has been changed */
> > + RelFileNode relnode;
> > +
> > + /* no previously reassembled toast chunks are necessary anymore */
> > + bool clear_toast_afterwards;
> > +
> > + /* valid for DELETE || UPDATE */
> > + ReorderBufferZTupleBuf *oldtuple;
> > + /* valid for INSERT || UPDATE */
> > + ReorderBufferZTupleBuf *newtuple;
> > + } ztp;
> > +
> >
> >
> > +/* an individual zheap tuple, stored in one chunk of memory */
> > +typedef struct ReorderBufferZTupleBuf
> > +{
> > ..
> > + /* tuple header, the interesting bit for users of logical decoding */
> > + ZHeapTupleData tuple;
> > ..
> > +} ReorderBufferZTupleBuf;
> >
> > Apart from this, we need to define different decode functions for
> > zheap operations as the WAL data is different for heap and zheap, so
> > same functions can't be used to decode.
>
> I'm very strongly opposed to that. We shouldn't have expose every
> possible storage method to output plugins, that'll make extensibility
> a farce.  I think we'll either have to re-form a HeapTuple or decide
> to bite the bullet and start exposing tuples via slots.
>

To be clear, you are against exposing different format of tuples to
plugins, not having different decoding routines for other storage
engines, because later part is unavoidable due to WAL format.   Now,
about tuple format, I guess it would be a lot better if we expose via
slots, but won't that make existing plugins to change the way they
decode the tuple, maybe that is okay?  OTOH, re-forming the heap tuple
has a cost which might be okay for the time being or first version,
but eventually, we want to avoid that.  The other reason why I
refrained from tuple conversion was that I was not sure if we anywhere
rely on the transaction information in the tuple during decode
process, because that will be tricky to mimic, but I guess we don't
check that.

The only point for exposing a different tuple format via plugin was a
performance which I think can be addressed if we expose via slots.  I
don't want to take up exposing slots instead of tuples for plugins as
part of this project and I think if we want to go with that, it is
better done as part of pluggable API?

>
> > This email is primarily to discuss about how the logical decoding for
> > basic DML operations (Insert/Update/Delete) will work in zheap.  We
> > might need some special mechanism to deal with sub-transactions as
> > zheap doesn't generate a transaction id for sub-transactions, but we
> > can discuss that separately.
>
> Subtransactions seems to be the hardest part besides the tuple format
> issue, so I think we should discuss that very soon.
>

Agreed, I am going to look at that part next.

>
> >  /*
> >   * Write relation description to the output stream.
> >   */
> > diff --git a/src/backend/replication/logical/reorderbuffer.c 
> > b/src/backend/replication/logical/reorderbuffer.c
> > index 23466bade2..70fb5e2934 100644
> > --- a/src/backend/replication/logical/reorderbuffer.c
> > +++ b/src/backend/replication/logical/reorderbuffer.c
> > @@ -393,6 +393,19 @@ ReorderBufferReturnChange(ReorderBuffer *rb, 
> > ReorderBufferChange *change)
> >   change->data.tp.oldtuple = NULL;
> >   }
> >   break;
> > + case REORDER_BUFFER_CHANGE_ZINSERT:
>
> This really needs to be undistinguishable from normal CHANGE_INSERT...
>

Sure, it will be if we decide to either re-form heap tuple or expose via slots.

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

Re: Delay locking partitions during query execution

[David Rowley]

On Fri, 4 Jan 2019 at 13:01, Tomas Vondra  wrote:
> On 1/3/19 11:57 PM, David Rowley wrote:
> > You'll know you're getting a generic plan when you see "Filter (a =
> > $1)" and see "Subplans Removed: " below the Append.
> >
>
> Indeed, with prepared statements I now see some improvements:
>
> partitions0  100 10001
> 
> master   19 1590 2090  128
> patched  18 1780 6820 1130
>
> So, that's nice. I wonder why the throughput drops so fast between 1k
> and 10k partitions, but I'll look into that later.

Those look strange. Why is it so slow with the non-partitioned case?
I'd have expected that to be the fastest result.

> Does this mean this optimization can only ever work with prepared
> statements, or can it be made to work with regular plans too?

That's a good question.  I confirm it's only any use of run-time
pruning occurs during init plan. For this patch to be any use, you'll
need to see a "Subplans Removed: " in the query's EXPLAIN ANALYZE
output.  If you don't see this then all Append/MergeAppend subplans
were initialised and the relation lock would have been obtained
regardless of if delaylock is set for the relation. The effect of the
patch here would just have been to obtain the lock during the first
call to ExecGetRangeTableRelation() for that relation instead of
during AcquireExecutorLocks().  There may actually be a tiny overhead
in this case since AcquireExecutorLocks() must skip the delaylock
relations, but they'll get locked later anyway. I doubt you could
measure that though.

When run-time pruning is able to prune partitions before execution
starts then the optimisation is useful since AcquireExecutorLocks()
won't obtain the lock and ExecGetRangeTableRelation() won't be called
for all pruned partition's rels as we don't bother to init the
Append/MergeAppend subplan for those.

I'm a little unsure if there are any cases where this type of run-time
pruning can occur when PREPAREd statements are not in use.  Initplan
parameters can't prune before executor run since we need to run the
executor to obtain the values of those. Likewise for evaluation of
volatile functions. So I think run-time pruning before initplan is
only ever going to happen for PARAM_EXTERN type parameters, i.e. with
PREPAREd statements (REF: analyze_partkey_exprs() partprune.c).
Without PREPAREd statements, if the planner itself was unable to prune
the partitions it would already have obtained the lock during
planning, so AcquireExecutorLocks(), in this case, would bump into the
local lock hash table entry and forego trying to obtain the lock
itself.  That's not free, but it's significantly faster than obtaining
a lock.

Or in short... it only good for prepared statements where the
statement's parameters allow for run-time pruning. However, that's a
pretty large case since the planner is still very slow at planning for
large numbers of partitions, meaning it's common (or at least it will
be) for people to use PREPAREd statement and plan_cache_mode =
force_generic_plan;

> >> Furthermore, I've repeatedly ran into this issue:
> >>
> >> test=# \d hashp
> >> ERROR:  unrecognized token: "false"
> >> LINE 2: ...catalog.array_to_string(array(select rolname from pg_catalog...
> >>  ^
> >> I have no idea why it breaks like this, and it's somewhat random (i.e.
> >> not readily reproducible). But I've only ever seen it with this patch
> >> applied.
> >
> > You'll probably need to initdb with the patch applied as there's a new
> > field in RangeTblEntry. If there's a serialised one of these stored in
> > the in the catalogue somewhere then the new read function will have
> > issues reading the old serialised format.
> >
>
> D'oh! That explains it, because switching from/to patched binaries might
> have easily been triggering the error. I've checked that there are no
> changes to catalogs, but it did not occur to me adding a new RTE field
> could have such consequences ...

schema-wise, no changes, but data-wise, there are changes.

$ pg_dump --schema=pg_catalog --data-only postgres | grep ":rellockmode" | wc -l
121

All of which are inside the pg_rewrite table:

$ pg_dump --schema=pg_catalog --data-only --table=pg_rewrite postgres
| grep ":rellockmode" | wc -l
121

I just used ":rellockmode" here as it's a field that exists in RangeTblEntry.

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

Re: inconsistency and inefficiency in setup_conversion()

[Tom Lane]

John Naylor  writes:
> Since it's been a few months since last discussion, I'd like to
> summarize the purpose of this patch and advocate for its inclusion in
> v12:

Pushed after some review and correction.  Notably, I didn't like
leaving the info about the encoding lookup out of bki.sgml, so
I changed that.

regards, tom lane

Re: Delay locking partitions during query execution

[Tomas Vondra]

On 1/3/19 11:57 PM, David Rowley wrote:
> On Fri, 4 Jan 2019 at 11:48, Tomas Vondra  
> wrote:
>> Nope, that doesn't seem to make any difference :-( In all cases the
>> resulting plan (with 10k partitions) looks like this:
>>
>> test=# explain analyze select * from hashp where a = 13442;
>>
>>   QUERY PLAN
>> ---
>>  Append  (cost=0.00..41.94 rows=13 width=4)
>>  (actual time=0.018..0.018 rows=0 loops=1)
>>->  Seq Scan on hashp6784 (cost=0.00..41.88 rows=13 width=4)
>>  (actual time=0.017..0.018 rows=0 loops=1)
>>  Filter: (a = 13442)
>>  Planning Time: 75.870 ms
>>  Execution Time: 0.471 ms
>> (5 rows)
>>
>> and it doesn't change (the timings on shape) no matter how I set any of
>> the GUCs.
> 
> For this to work, run-time pruning needs to take place, so it must be
> a PREPAREd statement.
> 
> With my test I used:
> 
> bench.sql:
> \set p_a 13315
> select * from hashp where a = :p_a;
> 
> $ pgbench -n -f bench.sql -M prepared -T 60 postgres
> 
> You'll know you're getting a generic plan when you see "Filter (a =
> $1)" and see "Subplans Removed: " below the Append.
> 

Indeed, with prepared statements I now see some improvements:

partitions0  100 10001

master   19 1590 2090  128
patched  18 1780 6820 1130

So, that's nice. I wonder why the throughput drops so fast between 1k
and 10k partitions, but I'll look into that later.

Does this mean this optimization can only ever work with prepared
statements, or can it be made to work with regular plans too?

>> Furthermore, I've repeatedly ran into this issue:
>>
>> test=# \d hashp
>> ERROR:  unrecognized token: "false"
>> LINE 2: ...catalog.array_to_string(array(select rolname from pg_catalog...
>>  ^
>> I have no idea why it breaks like this, and it's somewhat random (i.e.
>> not readily reproducible). But I've only ever seen it with this patch
>> applied.
> 
> You'll probably need to initdb with the patch applied as there's a new
> field in RangeTblEntry. If there's a serialised one of these stored in
> the in the catalogue somewhere then the new read function will have
> issues reading the old serialised format.
> 

D'oh! That explains it, because switching from/to patched binaries might
have easily been triggering the error. I've checked that there are no
changes to catalogs, but it did not occur to me adding a new RTE field
could have such consequences ...

regards

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

Re: Custom text type for title text

[Peter Eisentraut]

On 03/01/2019 23:22, Daniel Heath wrote:
> I propose a 'titletext' type, which has the following properties when
> compared for equality:
>  * Case insensitivity (like 'citext')
>  * Only considers characters in [:alnum:] (that is, ignores spaces,
> punctuation, etc)

My work on insensitive/non-deterministic collations[0] might cover this.

[0]:
https://www.postgresql.org/message-id/flat/1ccc668f-4cbc-0bef-af67-450b47cdfee7%402ndquadrant.com

For example:

CREATE COLLATION yournamehere (provider = icu,
  locale = 'und-u-ks-level2-ka-shifted', deterministic = false);

(Roughly, ks-level2 means ignore case, ka-shifted means ignore punctuation.)

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

Re: Custom text type for title text

[Fabrízio de Royes Mello]

Em qui, 3 de jan de 2019 às 20:53, Daniel Heath  escreveu:

> Would this also be appropriate for inclusion as contrib? I'm unfamiliar
> with the policy for what is / is not included there.
>
>
Please do not top post.

At first I recommend you implement it as an extension (using gitlab,
github, bitbucket or something else) and after you have a stable working
code maybe you should try to send it as a contrib module and then the
community will decide to accept it or not.

PostgreSQL is extensible enough to you provide this piece of work without
care with the community decisions. What I mean is you necessarily don’t
need to send it as a contrib module, just maintain it as a separate
extension project.

Regards,



-- 
   Fabrízio de Royes Mello Timbira - http://www.timbira.com.br/
   PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento

Re: pg_stat_ssl additions

[Peter Eisentraut]

Updated patches for some merge conflicts

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From 296d8d19cc49c2e7a6e8489a6d21cd6a182f2686 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Tue, 2 Oct 2018 12:17:22 +0200
Subject: [PATCH v4 1/4] doc: Add link from sslinfo to pg_stat_ssl

---
 doc/src/sgml/sslinfo.sgml | 5 +
 1 file changed, 5 insertions(+)

diff --git a/doc/src/sgml/sslinfo.sgml b/doc/src/sgml/sslinfo.sgml
index cda09aaafd..0fde0fc10e 100644
--- a/doc/src/sgml/sslinfo.sgml
+++ b/doc/src/sgml/sslinfo.sgml
@@ -14,6 +14,11 @@ sslinfo
   will return NULL) if the current connection does not use SSL.
  
 
+ 
+  Some of the information available through this module can also be obtained
+  using the built-in system view .
+ 
+
  
   This extension won't build at all unless the installation was
   configured with --with-openssl.

base-commit: 7170268efd511cb43bee49cd7963216a3f228648
-- 
2.20.1

From a4b7d9b25eab6d677e388860b4fee0f94c2fb93e Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Tue, 21 Aug 2018 21:11:36 +0200
Subject: [PATCH v4 2/4] Add tests for pg_stat_ssl system view

---
 src/test/ssl/t/001_ssltests.pl | 22 +-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/src/test/ssl/t/001_ssltests.pl b/src/test/ssl/t/001_ssltests.pl
index 2b875a3c95..2bcbb1b42a 100644
--- a/src/test/ssl/t/001_ssltests.pl
+++ b/src/test/ssl/t/001_ssltests.pl
@@ -8,7 +8,7 @@
 
 if ($ENV{with_openssl} eq 'yes')
 {
-   plan tests => 65;
+   plan tests => 71;
 }
 else
 {
@@ -309,6 +309,16 @@
qr/SSL error/,
"does not connect with client-side CRL");
 
+# pg_stat_ssl
+command_like([
+   'psql', '-X', '-A', '-F', ',', '-P', 'null=_null_',
+   '-d', $common_connstr,
+   '-c', "SELECT * FROM pg_stat_ssl WHERE pid = pg_backend_pid()"
+],
+qr{^pid,ssl,version,cipher,bits,compression,clientdn\n
+   ^\d+,t,TLSv[\d.]+,[\w-]+,\d+,f,$}mx,
+'pg_stat_ssl view without client certificate');
+
 ### Server-side tests.
 ###
 ### Test certificate authorization.
@@ -331,6 +341,16 @@
"user=ssltestuser sslcert=ssl/client.crt sslkey=ssl/client_tmp.key",
"certificate authorization succeeds with correct client cert");
 
+# pg_stat_ssl
+command_like([
+   'psql', '-X', '-A', '-F', ',', '-P', 'null=_null_',
+   '-d', "$common_connstr user=ssltestuser sslcert=ssl/client.crt 
sslkey=ssl/client_tmp.key",
+   '-c', "SELECT * FROM pg_stat_ssl WHERE pid = pg_backend_pid()"
+],
+qr{^pid,ssl,version,cipher,bits,compression,clientdn\n
+   
^\d+,t,TLSv[\d.]+,[\w-]+,\d+,f,/CN=ssltestuser$}mx,
+'pg_stat_ssl with client certificate');
+
 # client key with wrong permissions
 test_connect_fails(
$common_connstr,
-- 
2.20.1

From 12ac9f7e068ae6a762edff0e2c9706a6e8528986 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Mon, 15 Oct 2018 15:28:30 +0200
Subject: [PATCH v4 3/4] Fix pg_stat_ssl.clientdn

Return null if there is no client certificate.  This is how it has
always been documented, but in reality it returned an empty string.
---
 src/backend/utils/adt/pgstatfuncs.c | 5 -
 src/test/ssl/t/001_ssltests.pl  | 2 +-
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/src/backend/utils/adt/pgstatfuncs.c 
b/src/backend/utils/adt/pgstatfuncs.c
index 053bb73863..20ebcfbcdf 100644
--- a/src/backend/utils/adt/pgstatfuncs.c
+++ b/src/backend/utils/adt/pgstatfuncs.c
@@ -652,7 +652,10 @@ pg_stat_get_activity(PG_FUNCTION_ARGS)
values[20] = 
CStringGetTextDatum(beentry->st_sslstatus->ssl_cipher);
values[21] = 
Int32GetDatum(beentry->st_sslstatus->ssl_bits);
values[22] = 
BoolGetDatum(beentry->st_sslstatus->ssl_compression);
-   values[23] = 
CStringGetTextDatum(beentry->st_sslstatus->ssl_clientdn);
+   if (beentry->st_sslstatus->ssl_clientdn[0])
+   values[23] = 
CStringGetTextDatum(beentry->st_sslstatus->ssl_clientdn);
+   else
+   nulls[23] = true;
}
else
{
diff --git a/src/test/ssl/t/001_ssltests.pl b/src/test/ssl/t/001_ssltests.pl
index 2bcbb1b42a..ff7a20307f 100644
--- a/src/test/ssl/t/001_ssltests.pl
+++ b/src/test/ssl/t/001_ssltests.pl
@@ -316,7 +316,7 @@
'-c', "SELECT * FROM pg_stat_ssl WHERE pid = pg_backend_pid()"
 ],
 qr{^pid,ssl,version,cipher,bits,compression,clientdn\n
-   ^\d+,t,TLSv[\d.]+,[\w-]+,\d+,f,$}mx,
+   ^\d+,t,TLSv[\d.]+,[\w-]+,\d+,f,_null_$}mx,
 'pg_stat_ssl view wi

Re: [PATCH] Improvements to "Getting started" tutorial for Google Code-in task

[Alvaro Herrera]

On 2018-Nov-06, LAM JUN RONG wrote:

> Hi,
> 
> I must have forgotten to change the diff.

I noticed that you edited the diff by hand -- the line count in the last
hunk doesn't match.  This causes both "path" and "git apply" to reject
the patch saying it's corrupted.  I suggest not to do that.  I can get
it to apply by changing the "+239,14" in the previous-to-last hunk
header to "+239,13" IIRC.  (I think there's a utility to recompute line
counts in hunk headers, "rediff" I think, but better not to edit the
patch manually in the first place.)

Besides that, I have a hard time considering this patch committable.
There are some good additions, but they are mixed with some wording
changes that seem to be there just because the author doesn't like the
original, not because they're actual improvements.

One thing I definitely didn't like is that lines go well over 80 chars
per line.  We aren't terribly strict about that when there are long XML
tags, but this patch is over the top in that department.

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

Re: Delay locking partitions during query execution

[David Rowley]

On Fri, 4 Jan 2019 at 11:48, Tomas Vondra  wrote:
> Nope, that doesn't seem to make any difference :-( In all cases the
> resulting plan (with 10k partitions) looks like this:
>
> test=# explain analyze select * from hashp where a = 13442;
>
>   QUERY PLAN
> ---
>  Append  (cost=0.00..41.94 rows=13 width=4)
>  (actual time=0.018..0.018 rows=0 loops=1)
>->  Seq Scan on hashp6784 (cost=0.00..41.88 rows=13 width=4)
>  (actual time=0.017..0.018 rows=0 loops=1)
>  Filter: (a = 13442)
>  Planning Time: 75.870 ms
>  Execution Time: 0.471 ms
> (5 rows)
>
> and it doesn't change (the timings on shape) no matter how I set any of
> the GUCs.

For this to work, run-time pruning needs to take place, so it must be
a PREPAREd statement.

With my test I used:

bench.sql:
\set p_a 13315
select * from hashp where a = :p_a;

$ pgbench -n -f bench.sql -M prepared -T 60 postgres

You'll know you're getting a generic plan when you see "Filter (a =
$1)" and see "Subplans Removed: " below the Append.

> Furthermore, I've repeatedly ran into this issue:
>
> test=# \d hashp
> ERROR:  unrecognized token: "false"
> LINE 2: ...catalog.array_to_string(array(select rolname from pg_catalog...
>  ^
> I have no idea why it breaks like this, and it's somewhat random (i.e.
> not readily reproducible). But I've only ever seen it with this patch
> applied.

You'll probably need to initdb with the patch applied as there's a new
field in RangeTblEntry. If there's a serialised one of these stored in
the in the catalogue somewhere then the new read function will have
issues reading the old serialised format.

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

Re: Custom text type for title text

[Daniel Heath]

Would this also be appropriate for inclusion as contrib? I'm unfamiliar
with the policy for what is / is not included there.
Thanks,
Daniel Heath


On Fri, Jan 4, 2019, at 9:47 AM, Fabrízio de Royes Mello wrote:
> 
> 
> Em qui, 3 de jan de 2019 às 20:22, Daniel Heath 
> escreveu:>> Hi All,
>> 
>> I've frequently seen an issue in applications which store titles (eg
>> of books, events, user profiles) where duplicate values are not
>> properly vetted.>> 
>> The 'citext' type is helpful here, but I'd be keen to go further. 
>> 
>> I propose a 'titletext' type, which has the following properties when
>> compared for equality:>>  * Case insensitivity (like 'citext')
>>  * Only considers characters in [:alnum:] (that is, ignores spaces,
>>punctuation, etc)>> 
>> This would be useful for a range of situations where it's important
>> to avoid entering duplicate values.>> 
>> Given the discussion at
>> https://www.postgresql.org/message-id/CAKFQuwY9u14TqG8Yzj%3DfAB0tydvvtK7ibgFEx3tegbPWsGjJpg%40mail.gmail.com[1]
>> I'd lean towards making this type not automatically coerce to text
>> (to avoid surprising behaviour when comparing text to titletext).>> 
>> Is a suitable patch likely to be accepted?
>> 
> You don’t need touch the core to do that. Just implement it as an
> extension and share throught some channel like pgxn.org.> 
> Note that citext also is an extension and released as a contrib
> module.> 
> Regards,
> 
> -- 
>Fabrízio de Royes Mello Timbira -
>http://www.timbira.com.br/>PostgreSQL: Consultoria, Desenvolvimento, 
> Suporte 24x7 e
>Treinamento

Links:

  1. 
https://www.postgresql.org/message-id/CAKFQuwY9u14TqG8Yzj=fab0tydvvtk7ibgfex3tegbpwsgj...@mail.gmail.com

Re: Delay locking partitions during query execution

[Tomas Vondra]

On 1/3/19 10:50 PM, David Rowley wrote:
> On Fri, 4 Jan 2019 at 02:40, Tomas Vondra  
> wrote:
>> I'm a bit confused, because I can't reproduce any such speedup. I've
>> used the attached script that varies the number of partitions (which
>> worked quite nicely in the INSERT thread), but I'm getting results like
>> this:
>>
>> partitions  0   100 1000   1
>> 
>> master 49  1214  186  11
>> patched53  1225  187  11
>>
>> So I don't see any significant speedup, for some reason :-(
>>
>> Before I start digging into this, is there something that needs to be
>> done to enable it?
> 
> Thanks for looking at this.
> 
> One thing I seem to quite often forget to mention is that I was running with:
> 
> plan_cache_mode = force_generic_plan
> max_parallel_workers_per_gather = 0;
> 
> Without changing plan_cache_mode then the planner would likely never
> favour a generic plan since it will not appear to be very efficient
> due to the lack of consideration to the costing of run-time partition
> pruning.
> 
> Also, then with a generic plan, the planner will likely want to build
> a parallel plan since it sees up to 10k partitions that need to be
> scanned. max_parallel_workers_per_gather = 0 puts it right.
> 
> (Ideally, the planner would cost run-time pruning, but it's not quite
> so simple for RANGE partitions with non-equality operators. Likely
> we'll want to fix that one day, but that's not for here)
> 

Nope, that doesn't seem to make any difference :-( In all cases the
resulting plan (with 10k partitions) looks like this:

test=# explain analyze select * from hashp where a = 13442;

  QUERY PLAN
---
 Append  (cost=0.00..41.94 rows=13 width=4)
 (actual time=0.018..0.018 rows=0 loops=1)
   ->  Seq Scan on hashp6784 (cost=0.00..41.88 rows=13 width=4)
 (actual time=0.017..0.018 rows=0 loops=1)
 Filter: (a = 13442)
 Planning Time: 75.870 ms
 Execution Time: 0.471 ms
(5 rows)

and it doesn't change (the timings on shape) no matter how I set any of
the GUCs.

Furthermore, I've repeatedly ran into this issue:

test=# \d hashp
ERROR:  unrecognized token: "false"
LINE 2: ...catalog.array_to_string(array(select rolname from pg_catalog...
 ^
I have no idea why it breaks like this, and it's somewhat random (i.e.
not readily reproducible). But I've only ever seen it with this patch
applied.


regards

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

Re: Custom text type for title text

[Fabrízio de Royes Mello]

Em qui, 3 de jan de 2019 às 20:22, Daniel Heath  escreveu:

> Hi All,
>
> I've frequently seen an issue in applications which store titles (eg of
> books, events, user profiles) where duplicate values are not properly
> vetted.
>
> The 'citext' type is helpful here, but I'd be keen to go further.
>
> I propose a 'titletext' type, which has the following properties when
> compared for equality:
>  * Case insensitivity (like 'citext')
>  * Only considers characters in [:alnum:] (that is, ignores spaces,
> punctuation, etc)
>
> This would be useful for a range of situations where it's important to
> avoid entering duplicate values.
>
> Given the discussion at
> https://www.postgresql.org/message-id/CAKFQuwY9u14TqG8Yzj%3DfAB0tydvvtK7ibgFEx3tegbPWsGjJpg%40mail.gmail.com
> 
>  I'd
> lean towards making this type not automatically coerce to text (to avoid
> surprising behaviour when comparing text to titletext).
>
> Is a suitable patch likely to be accepted?
>

> You don’t need touch the core to do that. Just implement it as an
extension and share throught some channel like pgxn.org.

Note that citext also is an extension and released as a contrib module.

Regards,

-- 
   Fabrízio de Royes Mello Timbira - http://www.timbira.com.br/
   PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento

Custom text type for title text

[Daniel Heath]

Hi All,

I've frequently seen an issue in applications which store titles (eg of books, 
events, user profiles) where duplicate values are not properly vetted. 

The 'citext' type is helpful here, but I'd be keen to go further. 

I propose a 'titletext' type, which has the following properties when compared 
for equality:
 * Case insensitivity (like 'citext')
 * Only considers characters in [:alnum:] (that is, ignores spaces, 
punctuation, etc)

This would be useful for a range of situations where it's important to avoid 
entering duplicate values.

Given the discussion at 
https://www.postgresql.org/message-id/CAKFQuwY9u14TqG8Yzj%3DfAB0tydvvtK7ibgFEx3tegbPWsGjJpg%40mail.gmail.com
 

 I'd lean towards making this type not automatically coerce to text (to avoid 
surprising behaviour when comparing text to titletext).

Is a suitable patch likely to be accepted?

Thanks,

Daniel Heath

Re: Unified logging system for command-line programs

[Andres Freund]

Hi,

On 2019-01-03 17:03:43 -0500, Tom Lane wrote:
> Peter Eisentraut  writes:
> > On 03/01/2019 22:01, Tom Lane wrote:
> >> while ereport(ERROR) has the
> >> effect of writing a message and then calling exit(1).
> 
> > The problem is that in majority of cases the FRONTEND code, as it is
> > written today, doesn't want to exit() after an error.
> 
> Right, so for that you'd use ereport(WARNING) or LOG or whatever.

Or we could just add an ERROR variant that doesn't exit. Years back
I'd proposed that we make the log level a bitmask, but it could also
just be something like CALLSITE_ERROR or something roughly along those
lines.  There's a few cases in backend code where that'd be beneficial
too.

Greetings,

Andres Freund

Re: Unified logging system for command-line programs

[Tom Lane]

Peter Eisentraut  writes:
> On 03/01/2019 22:01, Tom Lane wrote:
>> while ereport(ERROR) has the
>> effect of writing a message and then calling exit(1).

> The problem is that in majority of cases the FRONTEND code, as it is
> written today, doesn't want to exit() after an error.

Right, so for that you'd use ereport(WARNING) or LOG or whatever.

We'd probably need a bit of care about which ereport levels produce
exactly what output, but I don't think that's insurmountable.  We
do not need all the backend-side message levels to exist for frontend.

regards, tom lane

Re: Delay locking partitions during query execution

[David Rowley]

On Fri, 4 Jan 2019 at 02:40, Tomas Vondra  wrote:
> I'm a bit confused, because I can't reproduce any such speedup. I've
> used the attached script that varies the number of partitions (which
> worked quite nicely in the INSERT thread), but I'm getting results like
> this:
>
> partitions  0   100 1000   1
> 
> master 49  1214  186  11
> patched53  1225  187  11
>
> So I don't see any significant speedup, for some reason :-(
>
> Before I start digging into this, is there something that needs to be
> done to enable it?

Thanks for looking at this.

One thing I seem to quite often forget to mention is that I was running with:

plan_cache_mode = force_generic_plan
max_parallel_workers_per_gather = 0;

Without changing plan_cache_mode then the planner would likely never
favour a generic plan since it will not appear to be very efficient
due to the lack of consideration to the costing of run-time partition
pruning.

Also, then with a generic plan, the planner will likely want to build
a parallel plan since it sees up to 10k partitions that need to be
scanned. max_parallel_workers_per_gather = 0 puts it right.

(Ideally, the planner would cost run-time pruning, but it's not quite
so simple for RANGE partitions with non-equality operators. Likely
we'll want to fix that one day, but that's not for here)

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

Re: [HACKERS] REINDEX CONCURRENTLY 2.0

[Peter Eisentraut]

Updated patch for some merge conflicts and addressing most of your
comments.  (I did not do anything about the syntax.)

On 09/12/2018 19:55, Sergei Kornilov wrote:
> I found one error in phase 4. Simple reproducer:
> 
>> create table test (i int);
>> create index this_is_very_large_exactly_maxnamelen_index_name_wink_wink_wink 
>> on test (i);
>> create index this_is_very_large_exactly_maxnamelen_index_name_wink_winkccold 
>> on test (i);
>> reindex table CONCURRENTLY test;
> 
> This fails with error
> 
>> ERROR:  duplicate key value violates unique constraint 
>> "pg_class_relname_nsp_index"
>> DETAIL:  Key (relname, 
>> relnamespace)=(this_is_very_large_exactly_maxnamelen_index_name_wink_win_ccold,
>>  2200) already exists.
> 
> CommandCounterIncrement() in (or after) index_concurrently_swap will fix this 
> issue.

fixed

>> ReindexPartitionedIndex(Relation parentIdx)
>>  ereport(ERROR,
>>  (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
>>   errmsg("REINDEX is not yet implemented for partitioned 
>> indexes")));
> 
> I think we need add errhint("you can REINDEX each partition separately") or 
> something similar. 
> Also can we omit this warning for reindex database? All partition must be in 
> same database and warning in such case is useless: we have warning, but doing 
> reindex for each partition => we reindex partitioned table correctly.

fixed by skipping in ReindexRelationConcurrently(), same as other
unsupported relkinds

> Another behavior issue i found with reindex (verbose) schema/database: INFO 
> ereport is printed twice for each table.
> 
>> INFO:  relation "measurement_y2006m02" was reindexed
>> DETAIL:  CPU: user: 0.00 s, system: 0.00 s, elapsed: 0.07 s.
>> INFO:  table "public.measurement_y2006m02" was reindexed
> 
> One from ReindexMultipleTables and another (with pg_rusage_show) from 
> ReindexRelationConcurrently.

fixed with some restructuring

>> ReindexRelationConcurrently
>> if (!indexRelation->rd_index->indisvalid)
> 
> it is better use IndexIsValid macro here? And same question about added 
> indexform->indisvalid in src/backend/commands/tablecmds.c

IndexIsValid() has been removed in the meantime.

>>   
>>   An index build with the CONCURRENTLY option failed, 
>> leaving
>>   an invalid index. Such indexes are useless but it can be
>> -  convenient to use REINDEX to rebuild them. Note 
>> that
>> -  REINDEX will not perform a concurrent build. To 
>> build the
>> -  index without interfering with production you should drop the index 
>> and
>> -  reissue the CREATE INDEX CONCURRENTLY command.
>> +  convenient to use REINDEX to rebuild them.
>>  
> 
> This documentation change seems wrong for me: reindex concurrently does not 
> rebuild invalid indexes. To fix invalid indexes we still need reindex with 
> lock table or recreate this index concurrently.

still being discussed elsewhere in this thread

>> +   A first pass to build the index is done for each new index entry.
>> +   Once the index is built, its flag 
>> pg_class.isready is
>> +   switched to true
>> +   At this point pg_class.indisvalid is switched to
>> +   true for the new index and to false 
>> for the old, and
>> +   Old indexes have pg_class.isready switched to 
>> false
> 
> Should be pg_index.indisvalid and pg_index.indisready, right?

fixed

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From 55345211e7d6026b573142b8cbe8fe24f7692285 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Thu, 3 Jan 2019 22:43:30 +0100
Subject: [PATCH v6] REINDEX CONCURRENTLY

---
 doc/src/sgml/mvcc.sgml|   1 +
 doc/src/sgml/ref/reindex.sgml | 184 +++-
 src/backend/catalog/index.c   | 501 +-
 src/backend/catalog/pg_depend.c   | 143 +++
 src/backend/catalog/toasting.c|   2 +-
 src/backend/commands/indexcmds.c  | 885 +++---
 src/backend/commands/tablecmds.c  |  32 +-
 src/backend/nodes/copyfuncs.c |   1 +
 src/backend/nodes/equalfuncs.c|   1 +
 src/backend/parser/gram.y |  22 +-
 src/backend/tcop/utility.c|  10 +-
 src/bin/psql/common.c |  16 +
 src/bin/psql/tab-complete.c   |  18 +-
 src/include/catalog/dependency.h  |   5 +
 src/include/catalog/index.h   |  18 +
 src/include/commands/defrem.h |   6 +-
 src/include/nodes/parsenodes.h|   1 +
 .../expected/reindex-concurrently.out |  78 ++
 src/test/isolation/isolation_schedule |   1 +
 .../isolation/specs/reindex-concurrently.spec |  40 +
 src/test/regress/expected/create_index.out|  70 ++
 src/test/regress/sql/create_index.sql |  51 +
 22 files changed, 1912 insertions(+), 1

Re: Unified logging system for command-line programs

[Peter Eisentraut]

On 03/01/2019 22:01, Tom Lane wrote:
> I envisioned that we'd have a wrapper in which
> non-error ereports() map directly onto what you're calling
> pg_log_debug, pg_log_warning, etc,

My code does that, but the other way around.  (It's easier that way than
to unpack ereport() invocations.)

> while ereport(ERROR) has the
> effect of writing a message and then calling exit(1).

The problem is that in majority of cases the FRONTEND code, as it is
written today, doesn't want to exit() after an error.

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

Re: [PATCH][PROPOSAL] Add enum releation option type

[Alvaro Herrera]

Attached version 7, with some renaming and rewording of comments.
(I also pgindented.  Some things are not pretty because of lack of
typedefs.list patching ... a minor issue at worst.)

I'm still not satisfied with the way the builtin enum tables are passed.
Can we settle for using add_enum_reloption instead at initialization
time?  Maybe that would be less ugly.  Alternatively, we can have
another member in relopt_enum which is a function pointer that returns
the array of relopt_enum_elt_def.  Not sure at which point to call that
function, though.

I think it would be great to have a new enum option in, say,
contrib/bloom, to make sure the add_enum_reloption stuff works
correctly.  If there's nothing obvious to add there, let's add something
to src/test/modules.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index f5efe94b7b..0aebb8ffc3 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -422,32 +422,41 @@ static relopt_real realRelOpts[] =
 	{{NULL}}
 };
 
-static relopt_string stringRelOpts[] =
+static relopt_enum_elt_def gist_buffering_enum_def[] =
+GIST_OPTION_BUFFERING_ENUM_DEF;
+static relopt_enum_elt_def view_check_option_enum_def[] =
+VIEW_OPTION_CHECK_OPTION_ENUM_DEF;
+static relopt_enum enumRelOpts[] =
 {
 	{
 		{
 			"buffering",
-			"Enables buffering build for this GiST index",
-			RELOPT_KIND_GIST,
-			AccessExclusiveLock
+"Enables buffering build for this GiST index",
+RELOPT_KIND_GIST,
+AccessExclusiveLock
 		},
-		4,
-		false,
-		gistValidateBufferingOption,
-		"auto"
+			gist_buffering_enum_def,
+			GIST_OPTION_BUFFERING_AUTO
 	},
 	{
 		{
 			"check_option",
-			"View has WITH CHECK OPTION defined (local or cascaded).",
-			RELOPT_KIND_VIEW,
-			AccessExclusiveLock
+"View has WITH CHECK OPTION defined (local or cascaded).",
+RELOPT_KIND_VIEW,
+AccessExclusiveLock
 		},
-		0,
-		true,
-		validateWithCheckOption,
-		NULL
+			view_check_option_enum_def,
+			VIEW_OPTION_CHECK_OPTION_NOT_SET
 	},
+	{
+		{
+			NULL
+		}
+	}
+};
+
+static relopt_string stringRelOpts[] =
+{
 	/* list terminator */
 	{{NULL}}
 };
@@ -494,6 +503,12 @@ initialize_reloptions(void)
    realRelOpts[i].gen.lockmode));
 		j++;
 	}
+	for (i = 0; enumRelOpts[i].gen.name; i++)
+	{
+		Assert(DoLockModesConflict(enumRelOpts[i].gen.lockmode,
+   enumRelOpts[i].gen.lockmode));
+		j++;
+	}
 	for (i = 0; stringRelOpts[i].gen.name; i++)
 	{
 		Assert(DoLockModesConflict(stringRelOpts[i].gen.lockmode,
@@ -532,6 +547,14 @@ initialize_reloptions(void)
 		j++;
 	}
 
+	for (i = 0; enumRelOpts[i].gen.name; i++)
+	{
+		relOpts[j] = &enumRelOpts[i].gen;
+		relOpts[j]->type = RELOPT_TYPE_ENUM;
+		relOpts[j]->namelen = strlen(relOpts[j]->name);
+		j++;
+	}
+
 	for (i = 0; stringRelOpts[i].gen.name; i++)
 	{
 		relOpts[j] = &stringRelOpts[i].gen;
@@ -629,6 +652,9 @@ allocate_reloption(bits32 kinds, int type, const char *name, const char *desc)
 		case RELOPT_TYPE_REAL:
 			size = sizeof(relopt_real);
 			break;
+		case RELOPT_TYPE_ENUM:
+			size = sizeof(relopt_enum);
+			break;
 		case RELOPT_TYPE_STRING:
 			size = sizeof(relopt_string);
 			break;
@@ -708,6 +734,24 @@ add_real_reloption(bits32 kinds, const char *name, const char *desc, double defa
 }
 
 /*
+ * add_enuum_reloption
+ *		Add a new enum reloption
+ */
+void
+add_enum_reloption(bits32 kinds, const char *name, const char *desc,
+   relopt_enum_elt_def *members, int default_val)
+{
+	relopt_enum *newoption;
+
+	newoption = (relopt_enum *) allocate_reloption(kinds, RELOPT_TYPE_ENUM,
+   name, desc);
+	newoption->members = members;
+	newoption->default_val = default_val;
+
+	add_reloption((relopt_gen *) newoption);
+}
+
+/*
  * add_string_reloption
  *		Add a new string reloption
  *
@@ -1223,6 +1267,51 @@ parse_one_reloption(relopt_value *option, char *text_str, int text_len,
 	   optreal->min, optreal->max)));
 			}
 			break;
+		case RELOPT_TYPE_ENUM:
+			{
+relopt_enum *optenum = (relopt_enum *) option->gen;
+relopt_enum_elt_def *elt;
+
+parsed = false;
+for (elt = optenum->members; elt->string_val; elt++)
+{
+	if (pg_strcasecmp(value, elt->string_val) == 0)
+	{
+		option->values.enum_val = elt->symbol_val;
+		parsed = true;
+		break;
+	}
+}
+if (!parsed)
+{
+	/*
+	 * If value is not among the allowed string values, but we
+	 * are not asked to validate, just use default numeric
+	 * value.  Otherwise raise an error.
+	 */
+	if (!validate)
+		option->values.enum_val = optenum->default_val;
+	else
+	{
+		StringInfoData buf;
+
+		initStringInfo(&buf);
+		for (elt = optenum->members; elt->string_val; elt++)
+		{
+			appendStringInfo(&buf, "\"%s\"", elt->string_val);
+			if (elt

Re: pgsql: Update ssl test certificates and keys

[Thomas Munro]

On Fri, Jan 4, 2019 at 3:36 AM Peter Eisentraut
 wrote:
> On 23/12/2018 09:04, Michael Paquier wrote:
> > On Tue, Nov 27, 2018 at 02:21:39PM +, Peter Eisentraut wrote:
> >> Update ssl test certificates and keys
> >>
> >> Debian testing and newer now require that RSA and DHE keys are at
> >> least 2048 bit long and no longer allow SHA-1 for signatures in
> >> certificates.  This is currently causing the ssl tests to fail there
> >> because the test certificates and keys have been created in violation
> >> of those conditions.
> >>
> >> Update the parameters to create the test files and create a new set of
> >> test files.
> >
> > Peter, would it make sense to back-patch this commit down to where the
> > SSL tests have been introduced?  If /etc/ssl/ is not correctly
> > configured, this results in failures across branches on Debian if the
> > default is used.
>
> done

Thanks.  FWIW I've just updated eelpout (a Debian testing BF animal
that runs all the extra tests including SSL) to use libssl-dev
(instead of libssl1.0-dev), and cleared its accache files.  Let's see
if that works...

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

Re: Unified logging system for command-line programs

[Tom Lane]

Peter Eisentraut  writes:
> On 03/01/2019 19:03, Andres Freund wrote:
>>> Relatedly, rewriting all the frontend programs to exception style would
>>> end up being a 10x project to rewrite everything for no particular
>>> benefit.  Going from 8 or so APIs to 2 is already an improvement, I
>>> think.  If someone wants to try going further, it can be considered, but
>>> it would be an entirely different project.

>> Why would it be 10x the effort,

> Because you would have to rewrite all the programs to handle elog(ERROR)
> jumping somewhere else.

FWIW, this argument has nothing to do with what I was actually
proposing.  I envisioned that we'd have a wrapper in which
non-error ereports() map directly onto what you're calling
pg_log_debug, pg_log_warning, etc, while ereport(ERROR) has the
effect of writing a message and then calling exit(1).  We would
use ereport(ERROR) in exactly the places where we're now writing
a message and calling exit(1).  No change at all in program
flow control, but an opportunity to consolidate code in places
that are currently doing this sort of thing:

#ifndef FRONTEND
ereport(ERROR,
(errcode_for_file_access(),
 errmsg("could not open file \"%s\" for reading: %m",
ControlFilePath)));
#else
{
fprintf(stderr, _("%s: could not open file \"%s\" for reading: %s\n"),
progname, ControlFilePath, strerror(errno));
exit(EXIT_FAILURE);
}
#endif

regards, tom lane

Re: [PATCH] get rid of StdRdOptions, use individual binary reloptions representation for each relation kind instead

[Alvaro Herrera]

On 2019-Jan-03, Nikolay Shaplov wrote:

> Can we think about backward compatibility aliases? 
> 
> #define ViewHasCheckOption(relation)   \
>   ((relation)->rd_options &&  \
>   ((ViewOptions *) (relation)->rd_options)->check_option_offset 
> != 0)
> 
> /* Alias for backward compatibility */
> #define RelationHasCheckOption(relation) ViewHasCheckOption(relation)
> 
> And keep them for as log as needed to avoid #if VERSION in thirdparty code?

Well, if you do this, at some point you need to tell the extension
author to change the code.  Or they can just keep the code unchanged
forever.  I don't think it's worth the bother.

I would have liked to get a StaticAssert in the definition, but I don't
think it's possible.  A standard Assert() should be possible, though.

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

Re: Changing SQL Inlining Behaviour (or...?)

[Paul Ramsey]

On Mon, Dec 31, 2018 at 2:23 PM Adam Brightwell <
adam.brightw...@crunchydata.com> wrote:

>
> > * Solution #2 - Quick and dirty and invisible. Tom suggested a hack that
> achieves the aims of #1 but without adding syntax to CREATE FUNCTION: have
> the inlining logic look at the cost of the wrapper and the cost of
> parameters, and if the cost of the wrapper "greatly exceeded" the cost of
> the parameters, then inline. So the PostGIS project would just set the cost
> of our wrappers very high, and we'd get the behaviour we want, while other
> users who want to use wrappers to force caching of calculations would have
> zero coded wrapper functions.  Pros: Solves the problem and easy to
> implement, I'm happy to contribute. Cons: it's so clearly a hack involving
> hidden (from users) magic.
> > ...
> > So my question to hackers is: which is less worse, #1 or #2, to
> implement and submit to commitfest, in case #3 does not materialize in time
> for PgSQL 12?
>
> I've been working with Paul to create and test a patch (attached) that
> addresses Solution #2. This patch essentially modifies the inlining
> logic to compare the cost of the function with the total cost of the
> parameters. The goal as stated above, is that for these kinds of
> functions, they would be assigned relatively high cost to trigger the
> inlining case.
>

I've tested this patch for both the internal types case and for the PostGIS
functions case, and in both cases it provides a way to get around the
undesirable inlining behaviour for our case without impacting people for
whom the behaviour has been desirable in the past.

Is this already in the commitfest?

P.

>
>

Re: [PATCH] get rid of StdRdOptions, use individual binary reloptions representation for each relation kind instead

[Nikolay Shaplov]

В письме от четверг, 3 января 2019 г. 16:10:20 MSK пользователь Alvaro Herrera 
написал:
> On 2019-Jan-02, Nikolay Shaplov wrote:
> > This is internal API, right? If we change it everywhere, then it is
> > changed and nothing will be broken?
> 
> No, it's exported for extensions to use.  If we change it unnecessarily,
> extension authors will hate me (not you) for breaking the compile and
> requiring an #if VERSION patch.

Ok, that's a good reason...

Can we think about backward compatibility aliases? 

#define ViewHasCheckOption(relation)   \
((relation)->rd_options &&  \
((ViewOptions *) (relation)->rd_options)->check_option_offset 
!= 0)

/* Alias for backward compatibility */
#define RelationHasCheckOption(relation) ViewHasCheckOption(relation)

And keep them for as log as needed to avoid #if VERSION in thirdparty code?

Or that is not the case?

Re: [PATCH] get rid of StdRdOptions, use individual binary reloptions representation for each relation kind instead

[Alvaro Herrera]

On 2019-Jan-02, Nikolay Shaplov wrote:

> This is internal API, right? If we change it everywhere, then it is changed 
> and nothing will be broken?

No, it's exported for extensions to use.  If we change it unnecessarily,
extension authors will hate me (not you) for breaking the compile and
requiring an #if VERSION patch.

These may not be in common usage, but I don't need any additional
reasons for people to hate me.

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

Re: pg_upgrade: Pass -j down to vacuumdb

[Jerry Sievers]

Peter Eisentraut  writes:

> On 02/01/2019 20:47, Jesper Pedersen wrote:
>
>> Well, that really depends. The user passed -j to pg_upgrade in order for 
>> the upgrade to happen faster, so maybe they would expect, as I would, 
>> that the ANALYZE phase would happen in parallel too.
>
> pg_upgrade -j reduces the *downtime* caused by pg_upgrade during the
> upgrade process.  Also, during said downtime, nothing else is happening,
> so you can use all the resources of the machine.
>
> Once the system is back up, you don't necessarily want to use all the
> resources.  The analyze script is specifically written to run while
> production traffic is active.  If you just want to run the analyze as
> fast as possible, you can just run vacuumdb -j directly, without using
> the script.

Peter, I'm skeptical here.

I might permit a connection to a just pg_upgraded DB prior to any
analyze being known finished only for the most trivial case.  At my site
however, *trivial* systems are a small minority.

In fact, our automated upgrade workflow uses our home-built parallel
analyzer which predates vacuumdb -j.  Apps are not allowed into the DB
until a fast 1st pass has been done.

We run it in 2 phases...

$all preceeding upgrade steps w/system locked out
analyze-lite (reduced stats target)
open DB for application traffic
analyze-full

Of course we are increasing downtime by disallowing app traffic till
finish of analyze-lite however the assumption is that many queries would
be too slow to attempt without full analyzer coverage, albiet at a
reduced stats target.

IMO this is certainly a case of no 1-size-fits-all solution so perhaps a
--analyze-jobs option :-)

FWIW

Thanks

> Moreover, it's not clear that pg_upgrade and vacuumdb are bound the same
> way, so it's not a given that the same -j number should be used.
>
> Perhaps more documentation would be useful here.

-- 
Jerry Sievers
Postgres DBA/Development Consulting
e: postgres.consult...@comcast.net

Re: Unified logging system for command-line programs

[Peter Eisentraut]

On 03/01/2019 19:03, Andres Freund wrote:
>> My goal was to make logging smaller and more
>> compact.  Two, I think tying error reporting to flow control does not
>> always work well and leads to bad code and a bad user experience.
> 
> Not sure I can buy that, given that we seem to be doing quite OK in the 
> backend.

Consider the numerous places where we do elog(LOG) for an *error*
because we don't want to jump away.

>> Relatedly, rewriting all the frontend programs to exception style would
>> end up being a 10x project to rewrite everything for no particular
>> benefit.  Going from 8 or so APIs to 2 is already an improvement, I
>> think.  If someone wants to try going further, it can be considered, but
>> it would be an entirely different project.
> 
> Why would it be 10x the effort,

Because you would have to rewrite all the programs to handle elog(ERROR)
jumping somewhere else.

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

Re: GiST VACUUM

[Andrey Borodin]

Cool, thanks!

> 2 янв. 2019 г., в 20:35, Heikki Linnakangas  написал(а):
> 
> In patch #1, to do the vacuum scan in physical order:
> ...
> I think this is ready to be committed, except that I didn't do any testing. 
> We discussed the need for testing earlier. Did you write some test scripts 
> for this, or how have you been testing?
Please see test I used to check left jumps for v18:
0001-Physical-GiST-scan-in-VACUUM-v18-with-test-modificat.patch
0002-Test-left-jumps-v18.patch


0002-Test-left-jumps-v18.patch
Description: Binary data


0001-Physical-GiST-scan-in-VACUUM-v18-with-test-modificat.patch
Description: Binary data

To trigger FollowRight GiST sometimes forget to clear follow-right marker 
simulating crash of an insert. This fills logs with "fixing incomplete split" 
messages. Search for "REMOVE THIS" to disable these ill-behavior triggers.
To trigger NSN jump GiST allocate empty page after every real allocation.

In log output I see
2019-01-03 22:27:30.028 +05 [54596] WARNING:  RESCAN TRIGGERED BY NSN
WARNING:  RESCAN TRIGGERED BY NSN
2019-01-03 22:27:30.104 +05 [54596] WARNING:  RESCAN TRIGGERED BY FollowRight
This means that code paths were really executed (for v18).

> 
> Patch #2:

> 
> * Bitmapset stores 32 bit signed integers, but BlockNumber is unsigned. So 
> this will fail with an index larger than 2^31 blocks. That's perhaps 
> academical, I don't think anyone will try to create a 16 TB GiST index any 
> time soon. But it feels wrong to introduce an arbitrary limitation like that.
Looks like bitmapset is unsuitable for collecting block numbers due to the 
interface. Let's just create custom container for this?
Or there is one other option: for each block number throw away sign bit and 
consider page potentially internal and potentially empty leaf if (blkno & 
0x7FF) is in bitmapset.
And then we will have to create at least one 17Tb GiST to check it actually 
works.

> * I was surprised that the bms_make_empty() function doesn't set the 'nwords' 
> field to match the allocated size. Perhaps that was intentional, so that you 
> don't need to scan the empty region at the end, when you scan through all 
> matching bits? Still, seems surprising, and needs a comment at least.
Explicitly set nwords to zero. Looking at the code now, I do not see this 
nwords==0 as a very good idea. Probably, it's effective, but it's hacky, 
creates implicit expectations in code.

> * We're now scanning all internal pages in the 2nd phase. Not only those 
> internal pages that contained downlinks to empty leaf pages. That's probably 
> OK, but the comments need updating on that.
Adjusted comments. That if before loop
> if (vstate.emptyPages > 0)
seems superfluous. But I kept it until we solve the problem with 31-bit 
bitmapset.
> * In general, comments need to be fixed and added in several places. For 
> example, there's no clear explanation on what the "delete XID", stored in 
> pd_prune_xid, means. (I know what it is, because I'm familiar with the same 
> mechanism in B-tree, but it's not clear from the code itself.)
I've added comment to GistPageSetDeleteXid()

* In this check
> if (GistPageIsDeleted(page) && 
> TransactionIdPrecedes(GistPageGetDeleteXid(page), RecentGlobalXmin))
I've switched using RecentGlobalDataXmin to RecentGlobalXmin, because we have 
done so in similar mechanics for GIN (for uniformity with B-tree).


Thanks for working on this!


Best regards, Andrey Borodin.




0002-Delete-pages-during-GiST-VACUUM-v19.patch
Description: Binary data


0001-Physical-GiST-scan-in-VACUUM-v19.patch
Description: Binary data

Re: pgsql: Remove WITH OIDS support, change oid catalog column visibility.

[Tom Lane]

I noticed that this patch has broken restores of existing dump files:

psql:testbed.public.backup:82: ERROR:  unrecognized configuration parameter 
"default_with_oids"

Quite aside from the fact that this won't work if the user tries to
restore in single-transaction or no-error mode, this is really really
unhelpful because it's impossible to tell whether the error is
ignorable or not, except by digging through the dump file.

What you should do is restore that GUC, but add a check-hook that throws
an error for an attempt to set it to "true".

regards, tom lane

Re: Logical decoding for operations on zheap tables

[Alvaro Herrera]

On 2019-Jan-03, Andres Freund wrote:

> Hi,
> 
> On 2019-01-03 15:13:42 -0300, Alvaro Herrera wrote:

> > Hmm, without looking at the patches, I agree that the tuples should be
> > given as slots to the logical decoding interface.  I wonder if we need a
> > further function in the TTS interface to help decoding, or is the
> > "getattr" stuff sufficient.
> 
> What precisely do you mean with "getattr stuff"? I'd assume that you'd
> normally do a slot_getallattrs() and then access tts_values/nulls
> directly.

Ah, yeah, you deform the tuple first and then access the arrays
directly, right.  I was just agreeing with your point that forming a
heaptuple only to have logical decoding grab individual attrs from there
didn't sound terribly optimal.

> I don't think there's anything missing in the slot interface itself,
> but using slots probably would require some careful considerations
> around memory management, possibly a decoding specific slot
> implementation even.

A specific slot implementation sounds like more work than I was
envisioning.  Can't we just "pin" a slot to a memory context or
something like that, to keep it alive until decoding is done with it?
It seems useful to avoid creating another copy of the tuple in memory
(which we would need if, if I understand you correctly, we need to form
the tuple under a different slot implementation from whatever the origin
is).

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

Re: Logical decoding for operations on zheap tables

[Andres Freund]

Hi,

On 2019-01-03 15:13:42 -0300, Alvaro Herrera wrote:
> On 2019-Jan-03, Andres Freund wrote:
> 
> > > Apart from this, we need to define different decode functions for
> > > zheap operations as the WAL data is different for heap and zheap, so
> > > same functions can't be used to decode.
> > 
> > I'm very strongly opposed to that. We shouldn't have expose every
> > possible storage method to output plugins, that'll make extensibility
> > a farce.  I think we'll either have to re-form a HeapTuple or decide
> > to bite the bullet and start exposing tuples via slots.
> 
> Hmm, without looking at the patches, I agree that the tuples should be
> given as slots to the logical decoding interface.  I wonder if we need a
> further function in the TTS interface to help decoding, or is the
> "getattr" stuff sufficient.

What precisely do you mean with "getattr stuff"? I'd assume that you'd
normally do a slot_getallattrs() and then access tts_values/nulls
directly. I don't think there's anything missing in the slot interface
itself, but using slots probably would require some careful
considerations around memory management, possibly a decoding specific
slot implementation even.

Greetings,

Andres Freund

Re: Logical decoding for operations on zheap tables

[Alvaro Herrera]

On 2019-Jan-03, Andres Freund wrote:

> > Apart from this, we need to define different decode functions for
> > zheap operations as the WAL data is different for heap and zheap, so
> > same functions can't be used to decode.
> 
> I'm very strongly opposed to that. We shouldn't have expose every
> possible storage method to output plugins, that'll make extensibility
> a farce.  I think we'll either have to re-form a HeapTuple or decide
> to bite the bullet and start exposing tuples via slots.

Hmm, without looking at the patches, I agree that the tuples should be
given as slots to the logical decoding interface.  I wonder if we need a
further function in the TTS interface to help decoding, or is the
"getattr" stuff sufficient.


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

Re: Unified logging system for command-line programs

[Andres Freund]

Hi,

On 2019-01-03 14:28:51 +0100, Peter Eisentraut wrote:
> On 31/12/2018 16:55, Andres Freund wrote:
> > I think we should aim to unify the use (in contrast to the
> > implementation) of logging as much as possible, rather than having a
> > separate API for it for client programs.
> 
> I opted against doing that, for mainly two reasons: One, I think the
> ereport() API is too verbose for this purpose, an invocation is usually
> two to three lines.

Well, then elog() could be used.


> My goal was to make logging smaller and more
> compact.  Two, I think tying error reporting to flow control does not
> always work well and leads to bad code and a bad user experience.

Not sure I can buy that, given that we seem to be doing quite OK in the backend.


> Relatedly, rewriting all the frontend programs to exception style would
> end up being a 10x project to rewrite everything for no particular
> benefit.  Going from 8 or so APIs to 2 is already an improvement, I
> think.  If someone wants to try going further, it can be considered, but
> it would be an entirely different project.

Why would it be 10x the effort, if you already touch all the relevant
log invocations? This'll just mean that the same lines will
mechanically need to be changed again.

Greetings,

Andres Freund

Re: Logical decoding for operations on zheap tables

[Andres Freund]

Hi,

On 2018-12-31 09:56:48 +0530, Amit Kapila wrote:
> To support logical decoding for zheap operations, we need a way to
> ensure zheap tuples can be registered as change streams.   One idea
> could be that we make ReorderBufferChange aware of another kind of
> tuples as well, something like this:
> 
> @@ -100,6 +123,20 @@ typedef struct ReorderBufferChange
>   ReorderBufferTupleBuf *newtuple;
>   } tp;
> + struct
> + {
> + /* relation that has been changed */
> + RelFileNode relnode;
> +
> + /* no previously reassembled toast chunks are necessary anymore */
> + bool clear_toast_afterwards;
> +
> + /* valid for DELETE || UPDATE */
> + ReorderBufferZTupleBuf *oldtuple;
> + /* valid for INSERT || UPDATE */
> + ReorderBufferZTupleBuf *newtuple;
> + } ztp;
> +
> 
> 
> +/* an individual zheap tuple, stored in one chunk of memory */
> +typedef struct ReorderBufferZTupleBuf
> +{
> ..
> + /* tuple header, the interesting bit for users of logical decoding */
> + ZHeapTupleData tuple;
> ..
> +} ReorderBufferZTupleBuf;
> 
> Apart from this, we need to define different decode functions for
> zheap operations as the WAL data is different for heap and zheap, so
> same functions can't be used to decode.

I'm very strongly opposed to that. We shouldn't have expose every
possible storage method to output plugins, that'll make extensibility
a farce.  I think we'll either have to re-form a HeapTuple or decide
to bite the bullet and start exposing tuples via slots.


> This email is primarily to discuss about how the logical decoding for
> basic DML operations (Insert/Update/Delete) will work in zheap.  We
> might need some special mechanism to deal with sub-transactions as
> zheap doesn't generate a transaction id for sub-transactions, but we
> can discuss that separately.

Subtransactions seems to be the hardest part besides the tuple format
issue, so I think we should discuss that very soon.

> +/*
> + * Write zheap's INSERT to the output stream.
> + */
> +void
> +logicalrep_write_zinsert(StringInfo out, Relation rel, ZHeapTuple newtuple)
> +{
> + pq_sendbyte(out, 'I');  /* action INSERT */
> +
> + Assert(rel->rd_rel->relreplident == REPLICA_IDENTITY_DEFAULT ||
> +rel->rd_rel->relreplident == REPLICA_IDENTITY_FULL ||
> +rel->rd_rel->relreplident == REPLICA_IDENTITY_INDEX);
> +
> + /* use Oid as relation identifier */
> + pq_sendint32(out, RelationGetRelid(rel));
> +
> + pq_sendbyte(out, 'N');  /* new tuple follows */
> + //logicalrep_write_tuple(out, rel, newtuple);
> +}

Obviously we need to do better - I don't think we should have
tuple-specific replication messages.


>  /*
>   * Write relation description to the output stream.
>   */
> diff --git a/src/backend/replication/logical/reorderbuffer.c 
> b/src/backend/replication/logical/reorderbuffer.c
> index 23466bade2..70fb5e2934 100644
> --- a/src/backend/replication/logical/reorderbuffer.c
> +++ b/src/backend/replication/logical/reorderbuffer.c
> @@ -393,6 +393,19 @@ ReorderBufferReturnChange(ReorderBuffer *rb, 
> ReorderBufferChange *change)
>   change->data.tp.oldtuple = NULL;
>   }
>   break;
> + case REORDER_BUFFER_CHANGE_ZINSERT:

This really needs to be undistinguishable from normal CHANGE_INSERT...

Greetings,

Andres Freund

Re: Python versions (was Re: RHEL 8.0 build)

[Tom Lane]

Peter Eisentraut  writes:
> On 29/11/2018 16:34, Tom Lane wrote:
>> After sleeping on it for awhile, I am liking the idea of probing
>> python, python3, python2 (while keeping the $PYTHON override of
>> course).

> I think this was the option with the most support.  Here is a patch.
> I agree we can backport this.

Patch looks fine as far as it goes, but do we need to adjust anything
in installation.sgml or plpython.sgml to explain it?

regards, tom lane

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

[Alvaro Herrera]

I revised this patch a bit.  Here's v25, where some finishing touches
are needed -- see below.  I think with these changes the patch would
become committable, at least for me.

I didn't like that you were adding an #include of psqlscan_int.h into
pgbench.c, when there's a specific comment in the header saying not to
do that, so I opted for adding a new accessor function on psqlscan.h.

I renamed some of your parameter additions.  I think the new names are
clearer, but they meant the +1's in your code are now in illogical
places.  (I moved some; probably not enough).  Please review/fix that.

I think "gset" is not a great name for the new struct member; please
find a better name.  I suggest "targetvar" but feel free to choose a
name that suits your taste.

There are two XXX comments.  One is about a function needing a comment
atop it.  The other is about realloc behavior.  To fix this one I would
add a new struct member indicating the allocated size of the array, then
growing exponentially instead of one at a time.  For most cases you can
probably get away with never reallocating beyond an initial allocation
of, say, 8 members.

In the docs, the [prefix] part needs to be explained in the \cset entry;
right now it's in \gset, which comes afterwards.  Let's move the
explanation up, and then in \gset say "prefix behaves as in \cset".

Thanks

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml
index b5e3a62a33..246944ea79 100644
--- a/doc/src/sgml/ref/pgbench.sgml
+++ b/doc/src/sgml/ref/pgbench.sgml
@@ -954,6 +954,87 @@ pgbench  options  d
   
 
   
+   
+
+ \cset [prefix]
+
+
+
+ 
+  This command may be used to end SQL queries, replacing an embedded
+  semicolon (\;) within a compound SQL command.
+ 
+
+ 
+  When this command is used, the preceding SQL query is expected to
+  return one row, the columns of which are stored into variables named after
+  column names, and prefixed with prefix if provided.
+ 
+
+ 
+  The following example sends three queries as one compound SQL command,
+  inducing one message sent at the protocol level.
+  The result of the second query is stored into variable two,
+  whereas the results of the other queries are discarded.
+
+-- compound of 3 queries
+SELECT 1 AS one \; SELECT 2 AS two \cset
+SELECT 2;
+
+ 
+
+ 
+  
+\cset does not work when empty SQL queries appear
+within a compound SQL command.
+  
+ 
+
+   
+
+   
+
+ \gset [prefix]
+
+
+
+ 
+  This commands may be used to end SQL queries, replacing a final semicolon
+  (;). 
+ 
+
+ 
+  When this command is used, the preceding SQL query is expected to
+  return one row, the columns of which are stored into variables named after
+  column names, and prefixed with prefix if provided.
+ 
+
+ 
+  The following example puts the final account balance from the first query
+  into variable abalance, and fills variables
+  p_two and p_three 
+  with integers from the last query.
+  The result of the second query is discarded.
+
+UPDATE pgbench_accounts
+  SET abalance = abalance + :delta
+  WHERE aid = :aid
+  RETURNING abalance \gset
+-- compound of two queries
+SELECT 1 \;
+SELECT 2 AS two, 3 AS three \gset p_
+
+ 
+
+ 
+  
+\gset does not work when empty SQL queries appear
+within a compound SQL command.
+  
+ 
+
+   
+

 \if expression
 \elif expression
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 649297ae4f..7901fcffca 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -499,14 +499,16 @@ typedef enum QueryMode
 static QueryMode querymode = QUERY_SIMPLE;
 static const char *QUERYMODE[] = {"simple", "extended", "prepared"};
 
-typedef struct
+typedef struct Command
 {
-	char	   *line;			/* text of command line */
-	int			command_num;	/* unique index of this Command struct */
+	PQExpBufferData lines;		/* raw command text (possibly multi-line) */
+	char	   *first_line;		/* first line for short display */
 	int			type;			/* command type (SQL_COMMAND or META_COMMAND) */
 	MetaCommand meta;			/* meta command identifier, or META_NONE */
 	int			argc;			/* number of command words */
 	char	   *argv[MAX_ARGS]; /* command word list */
+	int			last_cmd;		/* last command (number of \;) */
+	char	  **gset;			/* per-compound command prefix */
 	PgBenchExpr *expr;			/* parsed expression, if needed */
 	SimpleStats stats;			/* time spent in this command */
 } Command;
@@ -521,7 +523,6 @@ typedef struct ParsedScript
 
 static ParsedScript sql_script[MAX_SCRIPTS];	/* SQL script files */
 static int	num_scripts;		/* number of scripts in sql_script[] */
-static int	num_commands = 0;	/* total number of Command 

Re: SQL/JSON: functions

[Andrew Alsup]

> Attached 21st version of the patches.
>
> I decided to include here patch  with complete jsonpath 
implementation (it
> is a squash of all 6 jsonpath-v21 patches). I hope this will simplify 
reviewing

> and testing in cfbot.cputube.org.

I'd like to help in reviewing this patch. Please let me know if there's 
something in particular I should focus on such as documentation, 
functionality, or source. If not, I'll probably just proceed in that order.


Regards, Andy Alsup

Re: reducing the footprint of ScanKeyword (was Re: Large writable variables)

[Joerg Sonnenberger]

On Sun, Dec 16, 2018 at 11:50:15AM -0500, John Naylor wrote:
> A few months ago I was looking into faster search algorithms for
> ScanKeywordLookup(), so this is interesting to me. While an optimal
> full replacement would be a lot of work, the above ideas are much less
> invasive and would still have some benefit. Unless anyone intends to
> work on this, I'd like to flesh out the offset-into-giant-string
> approach a bit further:

Hello John,
I was pointed at your patch on IRC and decided to look into adding my
own pieces. What I can provide you is a fast perfect hash function
generator.  I've attached a sample hash function based on the current
main keyword list. hash() essentially gives you the number of the only
possible match, a final strcmp/memcmp is still necessary to verify that
it is an actual keyword though. The |0x20 can be dropped if all cases
have pre-lower-cased the input already. This would replace the binary
search in the lookup functions. Returning offsets directly would be easy
as well. That allows writing a single string where each entry is prefixed
with a type mask, the token id, the length of the keyword and the actual
keyword text. Does that sound useful to you?

Joerg
#include 
#include 

uint32_t
hash(const void * __restrict key, size_t keylen)
{
static const uint16_t g[881] = {
0x015b, 0x, 0x0070, 0x01b2, 0x, 0x0078, 0x0020, 0x,
0x, 0x0193, 0x, 0x, 0x, 0x01ac, 0x, 0x0122,
0x00b9, 0x0176, 0x013b, 0x, 0x, 0x, 0x0150, 0x,
0x, 0x, 0x008b, 0x00ea, 0x00b3, 0x0197, 0x, 0x0118,
0x012d, 0x0102, 0x, 0x0091, 0x0061, 0x008c, 0x, 0x,
0x0144, 0x01b4, 0x, 0x, 0x01a8, 0x019e, 0x, 0x00da,
0x, 0x, 0x0122, 0x0176, 0x00f3, 0x016a, 0x00f4, 0x00c0,
0x0111, 0x, 0x0103, 0x0028, 0x001a, 0x0180, 0x, 0x,
0x005f, 0x, 0x00d9, 0x, 0x016d, 0x, 0x0170, 0x0007,
0x016f, 0x, 0x014e, 0x0098, 0x00a8, 0x004b, 0x, 0x0056,
0x, 0x0121, 0x0012, 0x0102, 0x0192, 0x, 0x00f2, 0x0066,
0x, 0x003a, 0x, 0x, 0x0144, 0x, 0x, 0x0133,
0x0067, 0x0169, 0x, 0x, 0x0152, 0x0122, 0x, 0x0058,
0x0135, 0x0045, 0x0193, 0x00d2, 0x007e, 0x, 0x00ae, 0x012c,
0x, 0x, 0x, 0x, 0x0124, 0x, 0x0046, 0x0018,
0x, 0x00ba, 0x00d1, 0x004a, 0x, 0x, 0x, 0x,
0x, 0x001f, 0x, 0x0101, 0x, 0x, 0x, 0x01b5,
0x016e, 0x0173, 0x008a, 0x, 0x0173, 0x000b, 0x, 0x00d5,
0x005e, 0x, 0x00ac, 0x, 0x, 0x, 0x01a1, 0x,
0x, 0x0127, 0x, 0x005e, 0x, 0x016f, 0x, 0x012b,
0x01a4, 0x01b4, 0x, 0x, 0x003a, 0x, 0x, 0x00f5,
0x00b1, 0x0003, 0x0123, 0x001b, 0x, 0x004f, 0x, 0x,
0x, 0x, 0x, 0x007a, 0x, 0x, 0x, 0x,
0x00c2, 0x00a2, 0x00b9, 0x, 0x00cb, 0x, 0x00d2, 0x,
0x0197, 0x0121, 0x, 0x00d6, 0x0107, 0x, 0x, 0x,
0x, 0x, 0x, 0x0165, 0x00df, 0x0121, 0x, 0x,
0x, 0x, 0x, 0x019b, 0x, 0x01ad, 0x, 0x014f,
0x018d, 0x, 0x015f, 0x0168, 0x, 0x0199, 0x, 0x,
0x, 0x00a1, 0x, 0x, 0x0109, 0x, 0x, 0x01a6,
0x0097, 0x, 0x0018, 0x, 0x00d1, 0x, 0x, 0x,
0x0187, 0x0018, 0x, 0x00aa, 0x, 0x, 0x, 0x,
0x0136, 0x0063, 0x00b8, 0x, 0x0067, 0x0114, 0x, 0x,
0x0151, 0x, 0x, 0x, 0x00bf, 0x, 0x, 0x,
0x01b4, 0x00d4, 0x, 0x0006, 0x017e, 0x0167, 0x003a, 0x017f,
0x0183, 0x00c9, 0x01a2, 0x, 0x, 0x0153, 0x00ce, 0x,
0x, 0x, 0x, 0x, 0x, 0x0051, 0x, 0x0086,
0x, 0x0083, 0x0137, 0x, 0x, 0x0050, 0x, 0x00d7,
0x, 0x, 0x, 0x0129, 0x00f1, 0x, 0x009b, 0x01a7,
0x, 0x00b4, 0x, 0x00e0, 0x0046, 0x0025, 0x, 0x,
0x, 0x0144, 0x, 0x01a5, 0x0044, 0x0096, 0x0078, 0x0166,
0x, 0x, 0x, 0x0143, 0x, 0x00b8, 0x, 0x009e,
0x, 0x008c, 0x, 0x, 0x, 0x, 0x, 0x00fe,
0x, 0x, 0x0037, 0x0057, 0x, 0x00c3, 0x, 0x,
0x, 0x00bf, 0x014b, 0x0069, 0x00ce, 0x, 0x019d, 0x007f,
0x0186, 0x, 0x0119, 0x0015, 0x, 0x000e, 0x0113, 0x0139,
0x008e, 0x01ab, 0x, 0x005c, 0x, 0x0095, 0x, 0x019d,
0x, 0x0195, 0x0036, 0x, 0x, 0x00e0, 0x0146, 0x,
0x0033, 0x, 0x, 0x0035, 0x, 0x, 0x, 0x,
0x00d2,

Re: Unable to `make install` on MacOS in the latest master (68a13f28be)

[Andrew Alsup]

> As a general rule, it's wise to do "make distclean" before "git pull"
> when you're tracking master.  This saves a lot of grief when someone
> rearranges the set of generated files, as happened here.  (If things
> are really messed up, you might need "git clean -dfx" to get rid of
> everything not in git.)
>
> You might worry that this will greatly increase the rebuild time,
> which it will if you don't take precautions.  The way to fix that
> is (1) use ccache and (2) set the configure script to use a cache
> file.
>
> regards, tom lane

Tom and Daniel,

Thanks for the help on "make distclean". That did the trick. I will be
more careful when pulling master. Somehow, I hadn't been hit with this
before, which was just dumb luck. Thanks for helping me out.

-- Andy

Re: Unable to `make install` on MacOS in the latest master (68a13f28be)

[Tom Lane]

Daniel Gustafsson  writes:
>> On 3 Jan 2019, at 16:54, Chapman Flack  wrote:
>> Perhaps influenced by commit 842cb9f ?

> It is indeed related to that commit.  You will need to do make distclean, or
> remove dynloader.h manually.

As a general rule, it's wise to do "make distclean" before "git pull"
when you're tracking master.  This saves a lot of grief when someone
rearranges the set of generated files, as happened here.  (If things
are really messed up, you might need "git clean -dfx" to get rid of
everything not in git.)

You might worry that this will greatly increase the rebuild time,
which it will if you don't take precautions.  The way to fix that
is (1) use ccache and (2) set the configure script to use a cache
file.

regards, tom lane

Re: Unable to `make install` on MacOS in the latest master (68a13f28be)

[Daniel Gustafsson]

> On 3 Jan 2019, at 16:54, Chapman Flack  wrote:
> 
> On 1/3/19 10:47 AM, Andrew Alsup wrote:
>> cp ./*.h '/usr/local/pgsql/include/server'/
>> cp: ./dynloader.h: No such file or directory
> 
> Has dynloader.h somehow ended up as a symbolic link to a file
> no longer present?
> 
> Perhaps influenced by commit 842cb9f ?

It is indeed related to that commit.  You will need to do make distclean, or
remove dynloader.h manually.

cheers ./daniel

Re: Unable to `make install` on MacOS in the latest master (68a13f28be)

[Chapman Flack]

On 1/3/19 10:47 AM, Andrew Alsup wrote:
> cp ./*.h '/usr/local/pgsql/include/server'/
> cp: ./dynloader.h: No such file or directory

Has dynloader.h somehow ended up as a symbolic link to a file
no longer present?

Perhaps influenced by commit 842cb9f ?

-Chap

Unable to `make install` on MacOS in the latest master (68a13f28be)

[Andrew Alsup]

I am unable to `make install` on MacOS in the latest master (68a13f28be).

Here are the steps to reproduce.

OS: MacOSX 10.14.2
Branch: master:HEAD (68a13f28be)

$ git log --pretty=format:'%h' -n 1
68a13f28be
$ ./configure --with-bonjour
$ make
$ sudo make install
...
/usr/bin/install -c -m 644 utils/errcodes.h
'/usr/local/pgsql/include/server/utils'
/usr/bin/install -c -m 644 utils/fmgroids.h
'/usr/local/pgsql/include/server/utils'
/usr/bin/install -c -m 644 utils/fmgrprotos.h
'/usr/local/pgsql/include/server/utils'
cp ./*.h '/usr/local/pgsql/include/server'/
cp: ./dynloader.h: No such file or directory
make[2]: *** [install] Error 1
make[1]: *** [install-include-recurse] Error 2
make: *** [install-src-recurse] Error 2

FWIW, I've also tried `./configure` without any flags, but that didn't
effect the results.

I am able to successfully build/install from branch `REL_11_STABLE`
(ad425aaf06)

Re: speeding up planning with partitions

[Justin Pryzby]

Hi,

I don't think I can't help with code review, but I loaded our largest
customer's schema into pg12dev and tested with this patch.  It's working well -
thanks for your work.

Details follow.

We have tooo many tables+indices so this vastly improves planning speed.  Our
large customers have ~300 parent tables and total ~20k child tables with total
~80k indices.  Our largest tables heirarchies have ~1200 child tables, which
may have as many as 6-8 indices each.  And 5-10 of the largest heirarchies are
unioned together in a view.

Running pg11.1, explaining query for the largest view with condition eliminates
all but today's tables can take several minutes with a cold cache, due to not
only stat()ing every file in every table in a partition heirarchy, before
pruning, but also actually open()ing all their indices.

Running 11dev with your v10 patch applied, this takes 2244ms with empty buffer
cache after postmaster restarted on a totally untuned instance (and a new
backend, with no cached opened files).

I was curious why it took even 2sec, and why it did so many opens() (but not
20k of them that PG11 does):

[pryzbyj@database postgresql]$ cut -d'(' -f1 /tmp/strace-12dev-explain |sort 
|uniq -c |sort -nr
   2544 lseek
   1263 open
   ...

It turns out 1050 open()s are due to historic data which is no longer being
loaded and therefor never converted to relkind=p (but hasn't exceeded the
retention interval so not yet DROPped, either).

Cheers,
Justin

Re: pgsql: Update ssl test certificates and keys

[Peter Eisentraut]

On 23/12/2018 09:04, Michael Paquier wrote:
> On Tue, Nov 27, 2018 at 02:21:39PM +, Peter Eisentraut wrote:
>> Update ssl test certificates and keys
>>
>> Debian testing and newer now require that RSA and DHE keys are at
>> least 2048 bit long and no longer allow SHA-1 for signatures in
>> certificates.  This is currently causing the ssl tests to fail there
>> because the test certificates and keys have been created in violation
>> of those conditions.
>>
>> Update the parameters to create the test files and create a new set of
>> test files.
> 
> Peter, would it make sense to back-patch this commit down to where the
> SSL tests have been introduced?  If /etc/ssl/ is not correctly
> configured, this results in failures across branches on Debian if the
> default is used.

done

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

Re: Is MinMaxExpr really leakproof?

[Stephen Frost]

Greetings,

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Stephen Frost  writes:
> > * Tom Lane (t...@sss.pgh.pa.us) wrote:
> >> So I'd like to get to a point where we're comfortable marking these
> >> functions leakproof despite the possibility of corner-case failures.
> >> We could just decide that the existing failure cases in varstr_cmp are
> >> not usefully exploitable for information leakage, or perhaps we could
> >> dumb down the error messages some more to make them even less so.
> >> It'd also be nice to have some articulatable policy that encompasses
> >> a choice like this.
> 
> > I'd rather not say "well, these are mostly leakproof and therefore it's
> > good enough" unless those corner-case failures you're referring to are
> > really "this system call isn't documented to ever fail in a way we can't
> > handle, but somehow it did and we're blowing up because of it."
> 
> Well, that's pretty much what we've got here.

Good.  Those all almost certainly fall under the category of 'covert
channels' and provided they're low bandwidth and hard to control, as
seems to be the case here, then I believe we can accept them.  I'm
afraid there isn't really any hard-and-fast definition that could be
used as a basis for a project policy around this, unfortunately.  We
certainly shouldn't be returning direct data from the heap or indexes as
part of error messages in leakproof functions, and we should do our best
to ensure that anything from system calls we make also don't, but
strerror-like results or the error codes themselves should be fine.

> 1. As Noah noted, every failure case in varstr_cmp is ideally a can't
> happen case, since if it could happen on valid data then that data
> couldn't be put into a btree index.

That's certainly a good point.

> 2. AFAICS, all the error messages in question just report that a system
> operation failed, with some errno string or equivalent; none of the
> original data is reported.  (Obviously, we'd want to add comments
> discouraging people from changing that ...)

Agreed, we should definitely add comments here (and, really, in any
other cases where we need to be thinking about similar issues..).

> Conceivably, an attacker could learn the length of some long string
> by noting a palloc failure report --- but we've already accepted an
> equivalent hazard in texteq or byteaeq, I believe, and anyway it's
> pretty hard to believe that an attacker could control such failures
> well enough to weaponize it.

Right, that's a low bandwidth covert channel and as such should be
acceptable.

> So the question boils down to whether you think that somebody could
> infer something else useful about the contents of a string from
> the strerror (or ICU u_errorName()) summary of a system function
> failure.  This seems like a pretty thin argument to begin with,
> and then the presumed difficulty of making such a failure happen
> repeatably makes it even harder to credit as a useful information
> leak.
> 
> So I'm personally satisfied that we could mark text_cmp et al as
> leakproof, but I'm not sure how we define a project policy that
> supports such a determination.

I'm not sure how to formalize such a policy either though perhaps we
could discuss specific "don't do this" things and have a light
dicussion about what covert channel are.

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: Python versions (was Re: RHEL 8.0 build)

[Peter Eisentraut]

On 29/11/2018 16:34, Tom Lane wrote:
> After sleeping on it for awhile, I am liking the idea of probing
> python, python3, python2 (while keeping the $PYTHON override of
> course).

I think this was the option with the most support.  Here is a patch.

I agree we can backport this.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From 4a04eb5ced0e6b3255d6d2044cd6699e459d7c28 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Thu, 3 Jan 2019 14:58:11 +0100
Subject: [PATCH] configure: Update python search order

Some systems don't ship with "python" by default anymore, only
"python3" or "python2" or some combination, so include those in the
configure search.

Discussion: 
https://www.postgresql.org/message-id/flat/1457.1543184081%40sss.pgh.pa.us#c9cc1199338fd6a257589c6dcea6cf8d
---
 config/python.m4 | 9 -
 configure| 2 +-
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/config/python.m4 b/config/python.m4
index 587bca99d5..9a4d12112e 100644
--- a/config/python.m4
+++ b/config/python.m4
@@ -8,8 +8,15 @@
 # 
 # Look for Python and set the output variable 'PYTHON' if found,
 # fail otherwise.
+#
+# As the Python 3 transition happens and PEP 394 isn't updated, we
+# need to cater to systems that don't have unversioned "python" by
+# default.  Some systems ship with "python3" by default and perhaps
+# have "python" in an optional package.  Some systems only have
+# "python2" and "python3", in which case it's reasonable to prefer the
+# newer version.
 AC_DEFUN([PGAC_PATH_PYTHON],
-[PGAC_PATH_PROGS(PYTHON, python)
+[PGAC_PATH_PROGS(PYTHON, [python python3 python2])
 if test x"$PYTHON" = x""; then
   AC_MSG_ERROR([Python not found])
 fi
diff --git a/configure b/configure
index 79a18717d3..821964601d 100755
--- a/configure
+++ b/configure
@@ -9699,7 +9699,7 @@ fi
 
 if test "$with_python" = yes; then
   if test -z "$PYTHON"; then
-  for ac_prog in python
+  for ac_prog in python python3 python2
 do
   # Extract the first word of "$ac_prog", so it can be a program name with 
args.
 set dummy $ac_prog; ac_word=$2
-- 
2.20.1

Re: FETCH FIRST clause PERCENT option

[Tomas Vondra]

On 1/3/19 1:00 PM, Surafel Temesgen wrote:
> Hi
> 
> On Tue, Jan 1, 2019 at 10:08 PM Tomas Vondra
> mailto:tomas.von...@2ndquadrant.com>> wrote:
> 
> ...
> 
> Firstly, the estimated number of rows should be about the same (10k) in
> both cases, but the "percent" case incorrectly uses the total row count
> (i.e. as if 100% rows were returned).
> 
> Ok I will correct it
> 
> 
> It's correct that the total cost for the "percent" case is based on 100%
> of rows, of course, because the code has to fetch everything and stash
> it into the tuplestore in LIMIT_INITIAL phase.
> 
> But that implies the startup cost for the "percent" case can't be 0,
> because all this work has to be done before emitting the first row.
> 
> 
> Correct, startup cost must be equal to total cost of source data path
> and total cost have to be slightly greater than startup cost . I am
> planing to increase the total cost by limitCount * 0.1(similar to the
> parallel_tuple_cost) because getting tuple from tuplestore are almost
> similar operation to passing a tuple from worker to master backend.
> 

OK, sounds good in principle.

> 
> So these costing aspects need fixing, IMHO.
> 
> 
> The execution part of the patch seems to be working correctly, but I
> think there's an improvement - we don't need to execute the outer plan
> to completion before emitting the first row. For example, let's say the
> outer plan produces 1 rows in total and we're supposed to return the
> first 1% of those rows. We can emit the first row after fetching the
> first 100 rows, we don't have to wait for fetching all 10k rows.
> 
> 
> but total rows count is not given how can we determine safe to return row
> 

But you know how many rows were fetched from the outer plan, and this
number only grows grows. So the number of rows returned by FETCH FIRST
... PERCENT also only grows. For example with 10% of rows, you know that
once you reach 100 rows you should emit ~10 rows, with 200 rows you know
you should emit ~20 rows, etc. So you may track how many rows we're
supposed to return / returned so far, and emit them early.

regards

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

Re: Delay locking partitions during query execution

[Tomas Vondra]

On 12/3/18 12:42 PM, David Rowley wrote:
> ...
>
> Master: 1 parts
> 
> $ pgbench -n -f bench.sql -M prepared -T 60 postgres
> tps = 108.882749 (excluding connections establishing)
> tps = 108.245437 (excluding connections establishing)
> 
> delaylock: 1 parts
> 
> $ pgbench -n -f bench.sql -M prepared -T 60 postgres
> tps = 1068.289505 (excluding connections establishing)
> tps = 1092.797015 (excluding connections establishing)
> 

I'm a bit confused, because I can't reproduce any such speedup. I've
used the attached script that varies the number of partitions (which
worked quite nicely in the INSERT thread), but I'm getting results like
this:

partitions  0   100 1000   1

master 49  1214  186  11
patched53  1225  187  11

So I don't see any significant speedup, for some reason :-(

Before I start digging into this, is there something that needs to be
done to enable it?

regards

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


select.sql
Description: application/sql


run-select.sh
Description: application/shellscript

Re: Unified logging system for command-line programs

[Peter Eisentraut]

On 31/12/2018 16:55, Andres Freund wrote:
> I think we should aim to unify the use (in contrast to the
> implementation) of logging as much as possible, rather than having a
> separate API for it for client programs.

I opted against doing that, for mainly two reasons: One, I think the
ereport() API is too verbose for this purpose, an invocation is usually
two to three lines.  My goal was to make logging smaller and more
compact.  Two, I think tying error reporting to flow control does not
always work well and leads to bad code and a bad user experience.
Relatedly, rewriting all the frontend programs to exception style would
end up being a 10x project to rewrite everything for no particular
benefit.  Going from 8 or so APIs to 2 is already an improvement, I
think.  If someone wants to try going further, it can be considered, but
it would be an entirely different project.

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

Re: Unified logging system for command-line programs

[Peter Eisentraut]

On 30/12/2018 20:45, Tom Lane wrote:
> It seems like a shame that src/common files still need to have
> #ifdef FRONTEND variant code to deal with frontend vs. backend
> conventions.  I wonder how hard it would be to layer some subset of
> ereport() functionality on top of what you have here, so as to get
> rid of those #ifdef stanzas.

The patch does address that in some places:

@@ -39,12 +45,7 @@ pgfnames(const char *path)
dir = opendir(path);
if (dir == NULL)
{
-#ifndef FRONTEND
-   elog(WARNING, "could not open directory \"%s\": %m", path);
-#else
-   fprintf(stderr, _("could not open directory \"%s\": %s\n"),
-   path, strerror(errno));
-#endif
+   pg_log_warning("could not open directory \"%s\": %m", path);
return NULL;
}

It's worth noting that less than 5 files are of concern for this, so
creating a more elaborate system would probably be more code than you'd
save at the other end.

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

Re: "SELECT ... FROM DUAL" is not quite as silly as it appears

[David Rowley]

I've just looked over the v4 patch. I agree that having an RTE for a
from-less SELECT seems like a good idea.

While reading the patch, I noted the following:

1. It's more efficient to use bms_next_member as it does not need to
re-skip 0 words on each iteration. (Likely bms_first_member() is not
needed anywhere in the code base)

int varno;

while ((varno = bms_first_member(result_relids)) >= 0)
remove_result_refs(root, varno, (Node *) f);

can also make the loop condition > 0, rather than  >= 0 to save the
final futile attempt at finding a value that'll never be there.

2. The following comment seems to indicate that we can go ahead and
make parallelise the result processing, but the code still defers to
the checks below and may still end up not parallelising if say,
there's a non-parallel safe function call in the SELECT's target list.

case RTE_RESULT:
/* Sure, execute it in a worker if you want. */
break;

3. You may as well just ditch the variable and just do:

Assert(rel->relid > 0);
Assert(planner_rt_fetch(rel->relid, root)->rtekind == RTE_RESULT);

instead of:

RangeTblEntry *rte PG_USED_FOR_ASSERTS_ONLY;

/* Should only be applied to RTE_RESULT base relations */
Assert(rel->relid > 0);
rte = planner_rt_fetch(rel->relid, root);
Assert(rte->rtekind == RTE_RESULT);

There are a few other cases doing just that in costsize.c

4. I don't quite understand why this changed in join.out

@@ -3596,7 +3588,7 @@ select t1.* from
>  Hash Right Join
  Output: i8.q2
  Hash Cond: ((NULL::integer) = i8b1.q2)
- ->  Hash Left Join
+ ->  Hash Join

Can you explain why that's valid?  I understand this normally occurs
when a qual exists which would filter out the NULL rows produced by
the join, but I don't see that in this case.

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

Re: Delay locking partitions during INSERT and UPDATE

[Tomas Vondra]

Hi,

On 11/23/18 1:14 AM, David Rowley wrote:
> As a follow-on from [1] and also discussed in [2], I'd like to propose
> that we don't obtain locks on all partitions during INSERT into a
> partitioned table and UPDATE of a partitioned key and instead, only
> lock the partition when we first route a tuple to it. This means that
> the order that the locks are obtained is no longer well defined and is
> at the mercy of the order that tuples are INSERTed or UPDATEd.  It
> seems worth relaxing this a bit for gains in performance, as when a
> partitioned table contains many partitions, the overhead of locking
> all partitions when inserting a single row, or just a few rows is
> often significantly higher than the cost of doing the actual insert.
> 

Yep, the locking seems like a significant bottleneck. I've done quite a
bit of testing on two machines, using a slightly modified version of
your test script with variable number of partitions (0 means not
partitioned), and the results look like this:

1) xeon e5-2620v4

partitions0 100 10001
-
master166436956 1039  108
patched   16398   155221522213228

2) i5-2500k

partitions 0 10010001
-
master  39012892 920   76
patched 389438383845 3522

When using UNLOGGED tables to minimize the external noise, it looks like
this:

3) xeon e5-2620v4

partitions  0  100 10001

master  30806 8740 1091  107
patched 30455281372758224985

partitions  0  100 10001

master  27662 9013 1277   79
patched 28263264742579422434


So the performance benefit is pretty clear - up to 2 orders of magnitude
with 10k partitions, and gets us fairly close to non-partitioned table.

Me gusta.

> The current behaviour was added in 54cde0c4c058073 in order to
> minimise deadlock risk.  It seems that the risk there only comes from
> AELs that could be taken when a partition directly receives a TRUNCATE
> / CREATE INDEX / VACUUM FULL / CLUSTER. There's obviously no conflict
> with other DML operations since two RowExclusiveLocks don't conflict
> with each other.  I think all other AEL obtaining DDL must be
> performed on the top level partitioned table, for example, ADD COLUMN
> can't be done directly on a partition, so there's no added deadlock
> risk from those. For a deadlock to occur one of the above DDL commands
> would have to be executed inside a transaction in an order opposite to
> the order rows are being INSERTed or UPDATEd in the partitioned table.
> If required, such operations could LOCK TABLE the top partitioned
> table to block the DML operation. There's already a risk of similar
> deadlocks from such operations done on multiple separate tables when
> the order they're done is not the same as the order the tables are
> written in a query, although, in that case, the window for the
> deadlock is likely to be much smaller.
> 

Hmmm, yeah.

Per the discussion in [1] the locking was necessary also to ensure
partitions can't disappear while we're building the descriptors in
RelationBuildPartitionDesc(). But AFAICS 3f2393edef fixed this.

The other issue - as you note - is ensuring locking order, to prevent
(or rather reduce the risk of) deadlocks. I agree with your assessment
here, i.e. that locking the parent is a sufficient protection.

Maybe there's an alternative solution with the same benefits and not
sacrificing the lock ordering, but I fail to see how it would work.

> 
> [1] 
> https://www.postgresql.org/message-id/flat/CAKJS1f_1RJyFquuCKRFHTdcXqoPX-PYqAd7nz=gvbwvgh4a...@mail.gmail.com

regards

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

Re: pg_upgrade: Pass -j down to vacuumdb

[Peter Eisentraut]

On 02/01/2019 20:47, Jesper Pedersen wrote:
> Well, that really depends. The user passed -j to pg_upgrade in order for 
> the upgrade to happen faster, so maybe they would expect, as I would, 
> that the ANALYZE phase would happen in parallel too.

pg_upgrade -j reduces the *downtime* caused by pg_upgrade during the
upgrade process.  Also, during said downtime, nothing else is happening,
so you can use all the resources of the machine.

Once the system is back up, you don't necessarily want to use all the
resources.  The analyze script is specifically written to run while
production traffic is active.  If you just want to run the analyze as
fast as possible, you can just run vacuumdb -j directly, without using
the script.

Moreover, it's not clear that pg_upgrade and vacuumdb are bound the same
way, so it's not a given that the same -j number should be used.

Perhaps more documentation would be useful here.

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

Re: FETCH FIRST clause PERCENT option

[Surafel Temesgen]

Hi

On Tue, Jan 1, 2019 at 10:08 PM Tomas Vondra 
wrote:

> Hi,
>
> I've been looking at this patch today, and I think there's a couple of
> issues with the costing and execution. Consider a simple table with 1M
> rows:
>
>   create table t as select i from generate_series(1,100) s(i);
>
> and these two queries, that both select 1% of rows
>
>   select * from t fetch first 1 rows only;
>
>   select * from t fetch first 1 percent rows only;
>
> which are costed like this
>
>   test=# explain select * from t fetch first 1 rows only;
>  QUERY PLAN
>   -
>Limit  (cost=0.00..144.25 rows=1 width=4)
>  ->  Seq Scan on t  (cost=0.00..14425.00 rows=100 width=4)
>   (2 rows)
>
>   test=# explain select * from t fetch first 1 percent rows only;
>  QUERY PLAN
>   -
>Limit  (cost=0.00..14425.00 rows=100 width=4)
>  ->  Seq Scan on t  (cost=0.00..14425.00 rows=100 width=4)
>   (2 rows)
>
> There are two issues with the costs in the "percent" case.
>
> Firstly, the estimated number of rows should be about the same (10k) in
> both cases, but the "percent" case incorrectly uses the total row count
> (i.e. as if 100% rows were returned).
>
> Ok I will correct it


It's correct that the total cost for the "percent" case is based on 100%
> of rows, of course, because the code has to fetch everything and stash
> it into the tuplestore in LIMIT_INITIAL phase.
>
> But that implies the startup cost for the "percent" case can't be 0,
> because all this work has to be done before emitting the first row.
>
>
Correct, startup cost must be equal to total cost of source data path and
total cost have to be slightly greater than startup cost . I am planing to
increase the total cost by limitCount * 0.1(similar to the
parallel_tuple_cost) because getting tuple from tuplestore are almost
similar operation to passing a tuple from worker to master backend.


> So these costing aspects need fixing, IMHO.
>
>
> The execution part of the patch seems to be working correctly, but I
> think there's an improvement - we don't need to execute the outer plan
> to completion before emitting the first row. For example, let's say the
> outer plan produces 1 rows in total and we're supposed to return the
> first 1% of those rows. We can emit the first row after fetching the
> first 100 rows, we don't have to wait for fetching all 10k rows.
>
>
but total rows count is not given how can we determine safe to return row

Regards
Surafel

Re: log bind parameter values on error

[Peter Eisentraut]

On 02/01/2019 23:53, Alexey Bashtanov wrote:
>> In fact, maybe don't use the Portal structure at all and just store the
>> saved textualized values inside postgres.c in a static variable.
> 
> Unlike SQL, parameters may spend much more memory, so I'd have them
> in portal memory context to make sure the memory is released earlier 
> rather than later.

Having them in the portal structure is different from having it in the
portal memory context.  Although there is perhaps value in keeping them
together.

> Do you think it would be acceptable to leave them cached in parameters 
> structure?

Let's see how it looks.

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

Re: Using vim for developing porstres wiki article

[Nikolay Shaplov]

В письме от среда, 2 января 2019 г. 8:59:13 MSK пользователь James Coleman 
написал:

> > So I decided to write it down to a wiki article, while I am restoring the
> > configuration so I do not have to remember them for the third time, if I
> > loose .vimrc again. :-)
> 
> I like expanding the small section in the Developer FAQ into a more
> detailed article.
> 
> But the new article is missing several things relative to the old
> instructions, and I don't think the changes should have been made to
> that page until this was more fully baked.

Actually I've kept most of the old instructions in "Old staff" section. So all 
data is available as it id (I've just removed filestyle paragraph, form the 
FAQ, as I added it there myself, and it is properly described in a new 
article). So nothing is lost, only new info is added.


> 
> A few specific thoughts:
> 
> 1. The new article makes it more difficult to get started since every
> setting would need to be copied separately. I think there should be a
> cohesive block of options that we recommend for copy/paste along with
> inline comments explaining what each one does.
As far as I can understand wiki is first of all for explaining, not for ready 
recipes.  So I explained.
We have a better place for ready recipe, but nobody uses it. 
It is src/tools/editors/vim.samples so it you want to make ready recipe, I 
would suggest to but it there, and just tell about it in the wiki.

> 2. There's a bit of conflation between general Vim setup and Postgres
> specific development. The old section I think was mostly geared toward
> someone who uses Vim but wants the Postgres-specific parts, and that's
> a valuable use case. Perhaps we could split the article into a section
> on general Vim setup (for example, turning on syntax) and a section on
> "if you also already use Vim, there's a way to do project-specific
> settings and the ones you should use".
I've been thinking about it. My idea was: an experienced vim user knows better 
what he really wants, he does not need any advises. And even less he needs 
ready recipes. 

A vim beginner needs understanding first of all. He can copy ready recipe, but 
it gives him nothing. So every options needs an explanation. 

So I'd keep article style as it is, just allow experienced user to scan it and 
choose tips he really wants. And let the beginner get what understanding he 
can get.

> 3. Several of the old specified options didn't make it into the new
> article's details and are a loss. I noticed this particularly since
> since just 2 or 3 days ago I myself had edited this section to add the
> softtabstop=0 option (the Vim default) so that if soft tabs are
> enabled in someone's general Vim config then hitting the tab key won't
> result in inserting 2 spaces while working in the Postgres source.

As far as I said above, I've kept old example. So if you have some expertise 
in options I've omitted, you can join explaining them.

As for softtabstop. From what I've heard, I'd just ignored this option. If 
user did something in a global config, then he is an experienced user, and 
needs no advises from us, he knows what he is doing. And for beginner this is 
information is useless, overriding default value with the same value is a 
strange thing. It is not the thing should think abut when he is starting

But it you think it is important, you can add an abstract about softtabstop, I 
do not mind. Different people sees importance in different things.

PS. I am beginner in vim, and for me config example in FAQ was totally useless. 
So I tried to create an article that would be useful for me, and tried not to 
destroy information that exists.

Re: [PATCH] check for ctags utility in make_ctags

[Peter Eisentraut]

On 03/01/2019 12:15, Nikolay Shaplov wrote:
>> +1, let's keep it simple.  I would just use "ctags/etags not found"
>> as error message.
> 
> Actually I was trying to say "Please install 'ctags' [utility] to run 
> make_ctags". But if all of you read it as "Please install 'ctags' [package] 
> to 
> run make_ctags", then it is really better to drop the advice.
> 
> So I removed it. See the patch.

A few more comments.

I don't know how portable command -v is.  Some systems have a /bin/sh
that is pre-POSIX.  Same with $(...).

If etags is not installed, the current script prints

xargs: etags: No such file or directory

I don't see the need to do more than that, especially if it makes the
script twice as long.

(Personally, I'd recommend removing make_etags altogether and using GNU
Global for Emacs.)

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

Re: [HACKERS] Time to change pg_regress diffs to unified by default?

[Peter Eisentraut]

On 03/01/2019 10:39, Christoph Berg wrote:
> It will especially say which _alternate.out file was used, which seems
> like a big win. So +1.

It already shows that in the existing diff output header.

Although if you have a really long absolute path, it might be hard to
find it.  So perhaps the attached patch to make it more readable.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From ac586a4fa2186b4cf85c4b7c7e0269fac1af402e Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Thu, 3 Jan 2019 12:12:21 +0100
Subject: [PATCH] pg_regress: Don't use absolute paths for the diff

Don't expand inputfile and outputfile to absolute paths globally, just
where needed.  In particular, pass them as is to the file name
arguments of the diff command, so that we don't see the full absolute
path in the diff header, which makes the diff unnecessarily verbose
and harder to read.
---
 src/test/regress/pg_regress.c | 14 +-
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/src/test/regress/pg_regress.c b/src/test/regress/pg_regress.c
index 9786e86211..4970b74f1b 100644
--- a/src/test/regress/pg_regress.c
+++ b/src/test/regress/pg_regress.c
@@ -488,7 +488,7 @@ convert_sourcefiles_in(const char *source_subdir, const 
char *dest_dir, const ch
/* Error logged in pgfnames */
exit(2);
 
-   snprintf(testtablespace, MAXPGPATH, "%s/testtablespace", outputdir);
+   snprintf(testtablespace, MAXPGPATH, "%s/testtablespace", 
make_absolute_path(outputdir));
 
 #ifdef WIN32
 
@@ -552,10 +552,10 @@ convert_sourcefiles_in(const char *source_subdir, const 
char *dest_dir, const ch
}
while (fgets(line, sizeof(line), infile))
{
-   replace_string(line, "@abs_srcdir@", inputdir);
-   replace_string(line, "@abs_builddir@", outputdir);
+   replace_string(line, "@abs_srcdir@", 
make_absolute_path(inputdir));
+   replace_string(line, "@abs_builddir@", 
make_absolute_path(outputdir));
replace_string(line, "@testtablespace@", 
testtablespace);
-   replace_string(line, "@libdir@", dlpath);
+   replace_string(line, "@libdir@", 
make_absolute_path(dlpath));
replace_string(line, "@DLSUFFIX@", DLSUFFIX);
fputs(line, outfile);
}
@@ -2220,10 +2220,6 @@ regression_main(int argc, char *argv[], init_function 
ifunc, test_function tfunc
 */
port = 0xC000 | (PG_VERSION_NUM & 0x3FFF);
 
-   inputdir = make_absolute_path(inputdir);
-   outputdir = make_absolute_path(outputdir);
-   dlpath = make_absolute_path(dlpath);
-
/*
 * Initialization
 */
@@ -2569,7 +2565,7 @@ regression_main(int argc, char *argv[], init_function 
ifunc, test_function tfunc
printf(_("The differences that caused some tests to fail can be 
viewed in the\n"
 "file \"%s\".  A copy of the test summary that 
you see\n"
 "above is saved in the file \"%s\".\n\n"),
-  difffilename, logfilename);
+  make_absolute_path(difffilename), 
make_absolute_path(logfilename));
}
else
{
-- 
2.20.1

Re: [PATCH] check for ctags utility in make_ctags

[Nikolay Shaplov]

В письме от четверг, 3 января 2019 г. 10:03:53 MSK пользователь Michael 
Paquier написал:
> On Wed, Jan 02, 2019 at 11:35:46AM -0500, Tom Lane wrote:
> > In fact, that's demonstrably not so: on my RHEL6 and Fedora boxes,
> > /usr/bin/etags isn't owned by any package, because it's a symlink
> > managed by the "alternatives" system.  It points to /usr/bin/etags.emacs
> > which is owned by the emacs-common package.  So dropping the advice
> > about how to fix the problem seems like a good plan.
> 
> +1, let's keep it simple.  I would just use "ctags/etags not found"
> as error message.

Actually I was trying to say "Please install 'ctags' [utility] to run 
make_ctags". But if all of you read it as "Please install 'ctags' [package] to 
run make_ctags", then it is really better to drop the advice.

So I removed it. See the patch.

diff --git a/src/tools/make_ctags b/src/tools/make_ctags
index 1609c07..0834468 100755
--- a/src/tools/make_ctags
+++ b/src/tools/make_ctags
@@ -2,6 +2,12 @@
 
 # src/tools/make_ctags
 
+if [ ! $(command -v ctags) ]
+then
+  echo "'ctags' utility is not found" 1>&2
+  exit 1
+fi
+
 trap "rm -f /tmp/$$" 0 1 2 3 15
 rm -f ./tags
 
diff --git a/src/tools/make_etags b/src/tools/make_etags
index 3ce96bc..cb44aed 100755
--- a/src/tools/make_etags
+++ b/src/tools/make_etags
@@ -2,6 +2,12 @@
 
 # src/tools/make_etags
 
+if [ ! $(command -v etags) ]
+then
+  echo "'etags' utility is not found" 1>&2
+  exit 1
+fi
+
 rm -f ./TAGS
 
 find `pwd`/ -type f -name '*.[chyl]' -print |

Re: [HACKERS] Time to change pg_regress diffs to unified by default?

[Peter Eisentraut]

On 02/01/2019 21:44, Tom Lane wrote:
> Peter Eisentraut  writes:
>> While we're considering the pg_regress output, what do you think about
>> replacing the ==... separator with a standard diff separator like
>> "diff %s %s %s\n".  This would make the file behave more like a proper
>> diff file, for use with other tools.  And it shows the diff options
>> used, for clarity.  See attached patch.
> 
> I'm confused by this patch.  Doesn't moving the diff call like that
> break the logic completely?

For clarification, I have attached a "before" and "after".

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
diff -u 
/Users/peter/devel/postgresql/postgresql/src/test/regress/expected/int4.out 
/Users/peter/devel/postgresql/postgresql/src/test/regress/results/int4.out
--- /Users/peter/devel/postgresql/postgresql/src/test/regress/expected/int4.out 
2018-12-07 15:10:58.0 +0100
+++ /Users/peter/devel/postgresql/postgresql/src/test/regress/results/int4.out  
2019-01-03 11:49:53.0 +0100
@@ -1,4 +1,4 @@
---
+--x
 -- INT4
 --
 CREATE TABLE INT4_TBL(f1 int4);
diff -u 
/Users/peter/devel/postgresql/postgresql/src/test/regress/expected/int8.out 
/Users/peter/devel/postgresql/postgresql/src/test/regress/results/int8.out
--- /Users/peter/devel/postgresql/postgresql/src/test/regress/expected/int8.out 
2018-12-07 15:10:58.0 +0100
+++ /Users/peter/devel/postgresql/postgresql/src/test/regress/results/int8.out  
2019-01-03 11:49:53.0 +0100
@@ -1,4 +1,4 @@
---
+--x
 -- INT8
 -- Test int8 64-bit integers.
 --
--- /Users/peter/devel/postgresql/postgresql/src/test/regress/expected/int4.out 
2018-12-07 15:10:58.0 +0100
+++ /Users/peter/devel/postgresql/postgresql/src/test/regress/results/int4.out  
2019-01-03 11:43:48.0 +0100
@@ -1,4 +1,4 @@
---
+--x
 -- INT4
 --
 CREATE TABLE INT4_TBL(f1 int4);

==

--- /Users/peter/devel/postgresql/postgresql/src/test/regress/expected/int8.out 
2018-12-07 15:10:58.0 +0100
+++ /Users/peter/devel/postgresql/postgresql/src/test/regress/results/int8.out  
2019-01-03 11:43:48.0 +0100
@@ -1,4 +1,4 @@
---
+--x
 -- INT8
 -- Test int8 64-bit integers.
 --

==

Re: [HACKERS] Time to change pg_regress diffs to unified by default?

[Daniel Gustafsson]

> On 3 Jan 2019, at 10:39, Christoph Berg  wrote:
> 
> Re: Peter Eisentraut 2019-01-02 
> <70440c81-37bb-76dd-e48b-b5a9550d5...@2ndquadrant.com>

>> While we're considering the pg_regress output, what do you think about
>> replacing the ==... separator with a standard diff separator like
>> "diff %s %s %s\n".  This would make the file behave more like a proper
>> diff file, for use with other tools.  And it shows the diff options
>> used, for clarity.  See attached patch.
> 
> It will especially say which _alternate.out file was used, which seems
> like a big win. So +1.

That has bitten more times than I’d like to admit, so definately +1 on being
explicit about that.

cheers ./daniel

Re: [HACKERS] Time to change pg_regress diffs to unified by default?

[Christoph Berg]

Re: Peter Eisentraut 2019-01-02 
<70440c81-37bb-76dd-e48b-b5a9550d5...@2ndquadrant.com>
> Committed.

\o/

> While we're considering the pg_regress output, what do you think about
> replacing the ==... separator with a standard diff separator like
> "diff %s %s %s\n".  This would make the file behave more like a proper
> diff file, for use with other tools.  And it shows the diff options
> used, for clarity.  See attached patch.

It will especially say which _alternate.out file was used, which seems
like a big win. So +1.

Christoph