Re: [HACKERS] Merge join for GiST

[Andrew Borodin]

2017-04-13 7:01 GMT+05:00 Jeff Davis :
> On Tue, Apr 11, 2017 at 8:35 AM, Alexander Korotkov
>  wrote:
>> On Tue, Apr 11, 2017 at 5:46 PM, Jeff Davis  wrote:
>>> Do you have a sense of how this might compare with range merge join?
>>
>>
>> If you have GiST indexes over ranges for both sides of join, then this
>> method could be used for range join.  Hence, it could be compared with range
>> merge join.
>> However, particular implementation in pgsphere uses hardcoded datatypes and
>> operations.
>> Thus, for range join we need either generalized version of GiST-based join
>> or special implementation for ranges.
>
> Alexander, Andrew,
>
> How do you think we should proceed? Which projects do you think should
> eventually be in core, versus which are fine as extensions?

Some points in favor of Range joins via nbtree:
1. It's more efficient than B-tree over GiST
2. It is already in a patch form

Point against:
1. Particular implementation is somewhat leaked abstraction. Inside
the planner, you check not for capabilities of operator and type, but
for specific op and type. But I do not know how to fix this.

So, here is my opinion: if we can inside the planner understand that
join condition involves range specifics (for all available ranges) and
do Range Merge Join, then this is preferred solution.

Yes, Spatial Join, when available, will provide roughly the same scan
performance. But B-trees are more well known to users new to
PostgreSQL, and B-trees are faster.

Best regards, Andrey Borodin.


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

Re: [HACKERS] Patch: Write Amplification Reduction Method (WARM)

[Pavan Deolasee]

On Wed, Apr 12, 2017 at 10:42 PM, Robert Haas  wrote:

> On Tue, Apr 11, 2017 at 1:20 PM, Pavan Deolasee
>


> > 5. Added code to set a CLEAR pointer to a WARM pointer when we know that
> the
> > entire chain is WARM. This should address the workload Dilip ran and
> found
> > regression (I don't think he got chance to confirm that)
>
> Which is clearly a thing that should happen before commit, and really,
> you ought to be leading the effort to confirm that, not him.  It's
> good for him to verify that your fix worked, but you should test it
> first.
>

Not sure why you think I did not do the tests. I did and reported that it
helps reduce the regression. Last para here:  https://www.postgresql.
org/message-id/CABOikdOTstHK2y0rDk%2BY3Wx9HRe%2BbZtj3zuYGU%
3DVngneiHo5KQ%40mail.gmail.com

I understand it might have got lost in the conversation and I possibly did
a poor job of explaining it. From my perspective, I did not want say that
everything is hunky-dory based on my own tests because 1. I probably do not
have access to the same kind of machine Dilip has and 2. It's better to get
it confirmed by someone who initially reported it. Again, I fully respect
that he would be busy with other things and I do not expect him or anyone
else to test/review my patch on priority. The only point I am trying to
make is that I did my own tests and made sure that it helps.

(Having said that, I am not sure if changing pointer state from CLEAR to
WARM is indeed a good change. Having thought more about it and after
looking at the page-split code, I now think that this might just confuse
the WARM cleanup code and make algorithm that much harder to prove)


> > 6. Enhanced stats collector to collect information about candidate WARM
> > chains and added mechanism to control WARM cleanup at the heap as well as
> > index level, based on configurable parameters. This gives user better
> > control over the additional work that is required for WARM cleanup.
>
> I haven't seen previous discussion of this; therefore I doubt whether
> we have agreement on these parameters.
>

Sure. I will bring these up in a more structured manner for everyone to
comment.


>
> > 7. Added table level option to disable WARM if nothing else works.
>
> -1 from me.
>

Ok. It's kinda last resort for me too. But at some point, we might want to
make that call if we find an important use case that regresses because of
WARM and we see no way to fix that or at least not without a whole lot of
complexity.


>
>
> > I may have missed something, but there is no intention to ignore known
> > regressions/reviews. Of course, I don't think that every regression will
> be
> > solvable, like if you run a CPU-bound workload, setting it up in a way
> such
> > that you repeatedly exercise the area where WARM is doing additional
> work,
> > without providing any benefit, may be you can still find regression. I am
> > willing to fix them as long as they are fixable and we are comfortable
> with
> > the additional code complexity. IMHO certain trade-offs are good, but I
> > understand that not everybody will agree with my views and that's ok.
>
> The point here is that we can't make intelligent decisions about
> whether to commit this feature unless we know which situations get
> better and which get worse and by how much.


Sure.


>   I don't accept as a
> general principle the idea that CPU-bound workloads don't matter.
> Obviously, I/O-bound workloads matter too, but we can't throw
> CPU-bound workloads under the bus.


Yeah, definitely not suggesting that.


>   Now, avoiding index bloat does
> also save CPU, so it is easy to imagine that WARM could come out ahead
> even if each update consumes slightly more CPU when actually updating,
> so we might not actually regress.  If we do, I guess I'd want to know
> why.


Well the kind of tests we did to look for regression were worst case
scenarios. For example, in the test where we found 10-15% regression, we
used a wide index (so recheck cost is high), WARM updated all rows,
disabled auto-vacuum (so no chain conversion) and then repeatedly selected
the rows from the index, thus incurring recheck overhead and in fact,
measuring only that.

When I measured WARM on tables with small scale factor so that everything
fits in memory, I found a modest 20% improvement in tps. So, you're right,
WARM might also help in-memory workloads. But that will show up only if we
measure UPDATEs and SELECTs both. If we measure only SELECTs and that too
in a state where we are paying all price for having done a WARM update,
obviously we will only see regression, if any. Not saying we should ignore
that. We should in fact measure all possible loads, and try to fix as many
as we can, especially if they resemble to a real-world use case,  but there
will be a trade-off to make. So I highly appreciate Amit and Dilip's help
with coming up additional tests. At least it gives us opportunity to think
how to fix them, even if we can't 

Re: [HACKERS] Patch: Write Amplification Reduction Method (WARM)

[Pavan Deolasee]

On Thu, Apr 13, 2017 at 2:04 AM, Peter Geoghegan  wrote:

> On Wed, Apr 12, 2017 at 10:12 AM, Robert Haas 
> wrote:
> >> I may have missed something, but there is no intention to ignore known
> >> regressions/reviews. Of course, I don't think that every regression
> will be
> >> solvable, like if you run a CPU-bound workload, setting it up in a way
> such
> >> that you repeatedly exercise the area where WARM is doing additional
> work,
> >> without providing any benefit, may be you can still find regression. I
> am
> >> willing to fix them as long as they are fixable and we are comfortable
> with
> >> the additional code complexity. IMHO certain trade-offs are good, but I
> >> understand that not everybody will agree with my views and that's ok.
> >
> > The point here is that we can't make intelligent decisions about
> > whether to commit this feature unless we know which situations get
> > better and which get worse and by how much.  I don't accept as a
> > general principle the idea that CPU-bound workloads don't matter.
> > Obviously, I/O-bound workloads matter too, but we can't throw
> > CPU-bound workloads under the bus.  Now, avoiding index bloat does
> > also save CPU, so it is easy to imagine that WARM could come out ahead
> > even if each update consumes slightly more CPU when actually updating,
> > so we might not actually regress.  If we do, I guess I'd want to know
> > why.
>
> I myself wonder if this CPU overhead is at all related to LP_DEAD
> recycling during page splits.


With the respect to the tests that myself, Dilip and others did for WARM, I
think we were kinda exercising the worst case scenario. Like in one case,
we created a table with 40% fill factor,  created an index with a large
text column, WARM updated all rows in the table, turned off autovacuum so
that chain conversion does not take place, and then repeatedly run select
query on those rows using the index which did not receive WARM insert.

IOW we were only measuring the overhead of doing recheck by constructing an
index tuple from the heap tuple and then comparing it against the existing
index tuple. And we did find regression, which is not entirely surprising
because obviously that code path does extra work when it needs to do
recheck. And we're only measuring that overhead without taking into account
the benefits of WARM to the system in general. I think counter-argument to
that is, such workload may exists somewhere which might be regressed.

I have my suspicions that the recyling
> has some relationship to locality, which leads me to want to
> investigate how Claudio Freire's patch to consistently treat heap TID
> as part of the B-Tree sort order could help, both in general, and for
> WARM.
>

It could be, especially if we re-redesign recheck solely based on the index
pointer state and the heap tuple state. That could be more performant for
selects and could also be more robust, but will require index inserts to
get hold of the old index pointer (based on root TID), compare it against
the new index tuple and either skip the insert (if everything matches) or
set a PREWARM flag on the old pointer, and insert the new tuple with
POSTWARM flag.

Searching for old index pointer will be non-starter for non-unique indexes,
unless they are also sorted by TID, something that Claudio's patch does.
What I am not sure is whether the patch on its own will stand the
performance implications because it increases the index tuple width (and
probably index maintenance cost too).

Thanks,
Pavan

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

Re: [HACKERS] snapbuild woes

[Andres Freund]

On April 12, 2017 9:58:12 PM PDT, Noah Misch  wrote:
>On Wed, Apr 12, 2017 at 10:21:51AM -0700, Andres Freund wrote:
>> On 2017-04-12 11:03:57 -0400, Peter Eisentraut wrote:
>> > On 4/12/17 02:31, Noah Misch wrote:
>> > >>> But I hope you mean to commit these snapbuild patches before
>the postgres 10
>> > >>> release?  As far as I know, logical replication is still very
>broken without
>> > >>> them (or at least some of that set of 5 patches - I don't know
>which ones
>> > >>> are essential and which may not be).
>> > >>
>> > >> Yes, these should go into 10 *and* earlier releases, and I don't
>plan to
>> > >> wait for 2017-07.
>> > > 
>> > > [Action required within three days.  This is a generic
>notification.]
>> > 
>> > I'm hoping for a word from Andres on this.
>> 
>> Feel free to reassign to me.
>
>Thanks for volunteering; I'll do that shortly.  Please observe the
>policy on
>open item ownership[1] and send a status update within three calendar
>days of
>this message.  Include a date for your subsequent status update.
>
>[1]
>https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com

Will, volunteering might be the wrong word.  These ate all my fault, although 
none look v10 specific.

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


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

Re: [HACKERS] Logical replication and inheritance

[Noah Misch]

On Wed, Apr 12, 2017 at 11:02:44AM -0400, Peter Eisentraut wrote:
> On 4/9/17 22:16, Noah Misch wrote:
> > On Wed, Apr 05, 2017 at 08:25:56AM -0400, Peter Eisentraut wrote:
> >> After thinking about it some more, I think the behavior we want would be
> >> that changes to inheritance would reflect in the publication membership.
> >>  So if you have a partitioned table, adding more partitions over time
> >> would automatically add them to the publication.
> >>
> >> We could implement this either by updating pg_publication_rel as
> >> inheritance changes, or perhaps more easily by checking and expanding
> >> inheritance in the output plugin/walsender.  There might be subtle
> >> behavioral differences between those two approaches that we should think
> >> through.  But I think one of these two should be done.
> > 
> > [Action required within three days.  This is a generic notification.]
> 
> Relative to some of the other open items I'm involved in, I consider
> this a low priority, so I'm not working on it right now.

By what day should the community look for your next update?


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

Re: [HACKERS] snapbuild woes

[Noah Misch]

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

Thanks for volunteering; I'll do that shortly.  Please observe the policy on
open item ownership[1] and send a status update within three calendar days of
this message.  Include a date for your subsequent status update.

[1] 
https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com


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

Re: [HACKERS] tablesync patch broke the assumption that logical rep depends on?

[Noah Misch]

On Tue, Apr 11, 2017 at 02:28:44AM +0900, Fujii Masao wrote:
>  src/backend/replication/logical/launcher.c
>  * Worker started and attached to our shmem. This check is safe
>  * because only launcher ever starts the workers, so nobody can steal
>  * the worker slot.
> 
> The tablesync patch enabled even worker to start another worker.
> So the above assumption is not valid for now.
> 
> This issue seems to cause the corner case where the launcher picks up
> the same worker slot that previously-started worker has already picked
> up to start another worker.

[Action required within three days.  This is a generic notification.]

The above-described topic is currently a PostgreSQL 10 open item.  Peter,
since you committed the patch believed to have created it, you own this open
item.  If some other commit is more relevant or if this does not belong as a
v10 open item, please let us know.  Otherwise, please observe the policy on
open item ownership[1] and send a status update within three calendar days of
this message.  Include a date for your subsequent status update.  Testers may
discover new open items at any time, and I want to plan to get them all fixed
well in advance of shipping v10.  Consequently, I will appreciate your efforts
toward speedy resolution.  Thanks.

[1] 
https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com


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

Re: [HACKERS] WAL logging problem in 9.4.3?

[Michael Paquier]

On Tue, Apr 11, 2017 at 5:38 PM, Kyotaro HORIGUCHI
 wrote:
> Sorry, what I have just sent was broken.

You can use PROVE_TESTS when running make check to select a subset of
tests you want to run. I use that all the time when working on patches
dedicated to certain code paths.

>> - Relation has new members no_pending_sync and pending_sync that
>>   works as instant cache of an entry in pendingSync hash.
>> - Commit-time synchronizing is restored as Michael's patch.
>> - If relfilenode is replaced, pending_sync for the old node is
>>   removed. Anyway this is ignored on abort and meaningless on
>>   commit.
>> - TAP test is renamed to 012 since some new files have been added.
>>
>> Accessing pending sync hash occurred on every calling of
>> HeapNeedsWAL() (per insertion/update/freeze of a tuple) if any of
>> accessing relations has pending sync.  Almost of them are
>> eliminated as the result.

Did you actually test this patch? One of the logs added makes the
tests a long time to run:
2017-04-13 12:11:27.065 JST [85441] t/102_vacuumdb_stages.pl
STATEMENT:  ANALYZE;
2017-04-13 12:12:25.766 JST [85492] LOG:  BufferNeedsWAL: pendingSyncs
= 0x0, no_pending_sync = 0

-   lsn = XLogInsert(RM_SMGR_ID,
-XLOG_SMGR_TRUNCATE | XLR_SPECIAL_REL_UPDATE);
+   rel->no_pending_sync= false;
+   rel->pending_sync = pending;
+   }
It seems to me that those flags and the pending_sync data should be
kept in the context of backend process and not be part of the Relation
data...

+void
+RecordPendingSync(Relation rel)
I don't think that I agree that this should be part of relcache.c. The
syncs are tracked should be tracked out of the relation context.

Seeing how invasive this change is, I would also advocate for this
patch as only being a HEAD-only change, not many people are
complaining about this optimization of TRUNCATE missing when wal_level
= minimal, and this needs a very careful review.

Should I code something? Or Horiguchi-san, would you take care of it?
The previous crash I saw has been taken care of, but it's been really
some time since I looked at this patch...
-- 
Michael


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

Re: [HACKERS] [sqlsmith] ERROR: badly formatted node string "RESTRICTINFO...

[Amit Kapila]

On Wed, Apr 12, 2017 at 10:30 PM, Tom Lane  wrote:
> Amit Kapila  writes:
>> On Wed, Apr 12, 2017 at 12:40 AM, Tom Lane  wrote:
>>> Anyone want to draft a patch for this?
>
>> Please find patch attached based on above discussion.
>
> This patch seems fairly incomplete: you can't just whack around major data
> structures like PlannedStmt and PlannerGlobal without doing the janitorial
> work of cleaning up support logic such as outfuncs/readfuncs.
>

Oops, missed it.

> Also, when you think about what will happen when doing a copyObject()
> on a completed plan, it seems like a pretty bad idea to link subplans
> into two independent lists.  We'll end up with two separate copies of
> those subtrees in places like the plan cache.
>
> I'm inclined to think the other approach of adding a parallel_safe
> flag to struct Plan is a better answer in the long run.  It's basically
> free to have it there because of alignment considerations, and it's
> hard to believe that we're not going to need that labeling at some
> point in the future anyway.
>
> I had been a bit concerned about having to have some magic in outfuncs
> and/or readfuncs to avoid transferring the unsafe subplan(s), but I see
> from your patch there's a better way: we can have ExecSerializePlan modify
> the subplan list as it transfers it into its private PlannedStmt struct.
> But I think iterating over the list and examining each subplan's
> parallel_safe marking is a better way to do that.
>
> Will work on this approach.
>

Thanks, I see that you have committed patch on those lines.



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


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

Re: [HACKERS] Inadequate parallel-safety check for SubPlans

[Amit Kapila]

On Thu, Apr 13, 2017 at 1:05 AM, Tom Lane  wrote:
> Robert Haas  writes:
>> On Wed, Apr 12, 2017 at 2:41 PM, Tom Lane  wrote:
>>> This is 100% wrong.  It's failing to recurse into the subexpressions of
>>> the SubPlan, which could very easily include parallel-unsafe function
>>> calls.
>
>> My understanding (apparently flawed?) is that the parallel_safe flag
>> on the SubPlan is supposed to encapsulate whether than SubPlan is
>> entirely parallel safe, so that no recursion is needed.  If the
>> parallel_safe flag on the SubPlan is being set wrong, doesn't that
>> imply that the Path from which the subplan was created had the wrong
>> value of this flag, too?
>
>> ...
>
>> Oh, I see.  The testexpr is separate from the Path.  Oops.
>
> Right.  Although you're nudging up against an alternate idea that
> I just had: we could define the parallel_safe flag on the SubPlan as
> encapsulating not only whether the child plan is safe, but whether
> the contained testexpr (and args) are safe.  If we were to make
> an is_parallel_safe() check on the testexpr before injecting
> PARAM_EXEC Params into it, and record the results in the SubPlan,
> maybe that would lead to a simpler answer.
>
> OTOH, that might not work out nicely; and I have a feeling that
> we'd end up needing a whitelist anyway later, to handle the
> case of correlated subplans.
>

The problem with supporting correlated subplans is that it is not easy
to decide about params that belong to whitelist because we need to
ensure that such params are available below the gather node.   It is
quite clear how whitelist of params can be useful for the case in hand
but it is not immediately clear to me how we will use it for
correlated sublans.   However, if you have ideas on how to make it
work, I can try to draft a patch as well on those lines as it can
certainly help some TPC-H queries.

>> Sounds reasonable.  Do you want to have a go at that?
>
> Yeah.  I'm knocking off for the day a bit early today, but I'll have
> a look at it tomorrow, unless anyone in the Far East beats me to it.
>

Thanks for looking into it.

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


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

Re: [HACKERS] Remove pg_stat_progress_vacuum from Table 28.2

[Amit Langote]

On 2017/04/13 12:11, Fujii Masao wrote:
> On Wed, Apr 12, 2017 at 10:32 AM, Amit Langote
>  wrote:
>> On 2017/04/12 0:22, Fujii Masao wrote:
>>> On Fri, Apr 7, 2017 at 9:53 AM, Amit Langote
>>>  wrote:
 On 2017/04/07 0:56, Fujii Masao wrote:
> On Tue, Apr 4, 2017 at 10:19 AM, Amit Langote
>  wrote:
>> It seems pg_stat_progress_vacuum is not supposed to appear in the table
>> titled "Collected Statistics Views".  It was added by c16dc1aca.  
>> Attached
>> patch fixes that.
>
> Instead, it should appear in the table of "Dynamic Statistics Views"
> because it reports dynamic info, i.e., progress, about VACUUM activity?

 I thought the same at first, but then realized we have a entirely separate
 section 28.4. Progress Reporting.
>>>
>>> Yes, but I don't think that removing that from 28.2 is an improvement
>>> for users.
>>
>> Perhaps you're right.  Here's a patch that moves pg_stat_progress_vacuum
>> to the Dynamic Statistics Views table.
> 
> Thanks for the patch! Pushed.

Thanks,

Regards,
Amit



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

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

[Michael Paquier]

On Thu, Apr 13, 2017 at 12:28 PM, Fujii Masao  wrote:
> On Thu, Apr 13, 2017 at 5:25 AM, Peter Eisentraut
>  wrote:
>> On 4/12/17 09:55, Fujii Masao wrote:
>>> To fix this issue, we should terminate walsender for logical replication
>>> before shutdown checkpoint starts. Of course walsender for physical
>>> replication still needs to keep running until shutdown checkpoint ends,
>>> though.
>>
>> Can we turn it into a kind of read-only or no-new-commands mode instead,
>> so it can keep streaming but not accept any new actions?
>
> So we make walsenders switch to that mode and wait for all the already-ongoing
> their "write" commands to finish, and then we start a shutdown checkpoint?
> This is an idea, but seems a bit complicated. ISTM that it's simpler to
> terminate only walsenders for logical rep before shutdown checkpoint.

Perhaps my memory is failing me here... But in clean shutdowns we do
shut down WAL senders after the checkpoint has completed so as we are
sure that they have flushed the LSN corresponding to the checkpoint
ending, right? Why introducing an inconsistency for logical workers?
It seems to me that logical workers should fail under the same rules.
-- 
Michael


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

Re: [HACKERS] GSOC'17 project introduction: Parallel COPY execution with errors handling

[Craig Ringer]

On 13 April 2017 at 01:57, Stas Kelvich  wrote:

> However I think it worth of quick research whether it is possible to create 
> special
> code path for COPY in which errors don’t cancel transaction.

Not really. Anything at any layer of the system expects to be able to ERROR:

* datatype input functions
* CHECK constraints
* FK constraints
* unique indexes
* user defined functions run by triggers
* interrupt signalling (think deadlock detector)
* ...

and we rely on ERROR unwinding any relevant memory contexts, releasing
lwlocks, etc.

When an xact aborts it may leave all sorts of mess on disk. Nothing
gets deleted, it's just ignored due to an aborted xmin.

Maybe some xid burn could be saved by trying harder to pre-validate
batches of data as much as possible before we write anything to the
heap, sorting obviously faulty data into buffers and doing as much
work as possible before allocating a new (sub)xid and writing to the
heap. We'd still abort but we'd only be aborting a vtxid.

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


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

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

[Fujii Masao]

On Thu, Apr 13, 2017 at 5:25 AM, Peter Eisentraut
 wrote:
> On 4/12/17 09:55, Fujii Masao wrote:
>> To fix this issue, we should terminate walsender for logical replication
>> before shutdown checkpoint starts. Of course walsender for physical
>> replication still needs to keep running until shutdown checkpoint ends,
>> though.
>
> Can we turn it into a kind of read-only or no-new-commands mode instead,
> so it can keep streaming but not accept any new actions?

So we make walsenders switch to that mode and wait for all the already-ongoing
their "write" commands to finish, and then we start a shutdown checkpoint?
This is an idea, but seems a bit complicated. ISTM that it's simpler to
terminate only walsenders for logical rep before shutdown checkpoint.

Regards,

-- 
Fujii Masao


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

Re: [HACKERS] Remove pg_stat_progress_vacuum from Table 28.2

[Fujii Masao]

On Wed, Apr 12, 2017 at 10:32 AM, Amit Langote
 wrote:
> On 2017/04/12 0:22, Fujii Masao wrote:
>> On Fri, Apr 7, 2017 at 9:53 AM, Amit Langote
>>  wrote:
>>> On 2017/04/07 0:56, Fujii Masao wrote:
 On Tue, Apr 4, 2017 at 10:19 AM, Amit Langote
  wrote:
> It seems pg_stat_progress_vacuum is not supposed to appear in the table
> titled "Collected Statistics Views".  It was added by c16dc1aca.  Attached
> patch fixes that.

 Instead, it should appear in the table of "Dynamic Statistics Views"
 because it reports dynamic info, i.e., progress, about VACUUM activity?
>>>
>>> I thought the same at first, but then realized we have a entirely separate
>>> section 28.4. Progress Reporting.
>>
>> Yes, but I don't think that removing that from 28.2 is an improvement
>> for users.
>
> Perhaps you're right.  Here's a patch that moves pg_stat_progress_vacuum
> to the Dynamic Statistics Views table.

Thanks for the patch! Pushed.

Regards,

-- 
Fujii Masao


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

[HACKERS] Different table schema in logical replication crashes

[Euler Taveira]

Hi,

If a certain table has different schemas and the subscriber table has an
unmatched column with a not null constraint, the logical replication
crashes with the above stack trace.

-- publisher
CREATE TABLE test (a integer, b varchar not null, c numeric not null,
PRIMARY KEY(a));
-- subscriber
CREATE TABLE test (a integer, b varchar not null, c numeric not null, d
integer not null, PRIMARY KEY(a));

Program terminated with signal SIGSEGV, Segmentation fault.
#0  list_nth_cell (n=0, list=0x0) at list.c:411
411{
(gdb) bt
#0  list_nth_cell (n=0, list=0x0) at list.c:411
#1  list_nth (list=0x0, n=0) at list.c:413
#2  0x005ddc6b in ExecConstraints
(resultRelInfo=resultRelInfo@entry=0xf96868,
slot=slot@entry=0xf984d8, estate=estate@entry=0xfc3808) at execMain.c:1881
#3  0x0057b0ba in CopyFrom (cstate=0xf980c8) at copy.c:2652
#4  0x006ae3bb in copy_table (rel=) at
tablesync.c:682
#5  LogicalRepSyncTableStart (origin_startpos=0x7ffe9c340640) at
tablesync.c:789
#6  0x006afb0f in ApplyWorkerMain (main_arg=) at
worker.c:1521
#7  0x00684813 in StartBackgroundWorker () at bgworker.c:838
#8  0x0068f6a2 in do_start_bgworker (rw=0xf0cbb0) at
postmaster.c:5577
#9  maybe_start_bgworker () at postmaster.c:5761
#10 0x00690195 in sigusr1_handler (postgres_signal_arg=) at postmaster.c:5015
#11 
#12 0x7fcd075f6873 in __select_nocancel () at
../sysdeps/unix/syscall-template.S:81
#13 0x00476c0c in ServerLoop () at postmaster.c:1693
#14 0x00691342 in PostmasterMain (argc=argc@entry=1,
argv=argv@entry=0xee4eb0)
at postmaster.c:1337
#15 0x00478684 in main (argc=1, argv=0xee4eb0) at main.c:228

Are we prepared to support different schemas in v10? Or should we disallow
it for v10 and add a TODO?


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

Re: [HACKERS] delta relations in AFTER triggers

[Corey Huinker]

>
> Great.  Thanks.  I wonder if there is some way we can automatically
> include code fragments in the documentation without keeping them in
> sync manually.
>
>
In whatever extra docs you add, could you include an example of an INSERT
ON CONFLICT, and potentially a CTE query that does two operations on the
same table. I'm not clear on what to expect when a statement does a mix of
INSERT, UPDATE, and DELETE? Will there be multiple firings of the trigger
in a single statement, or will the before/after sets be mashed together
regardless of which part of the query generated it?

Re: [HACKERS] pg_upgrade vs extension upgrades

[Bruce Momjian]

On Wed, Apr 12, 2017 at 06:59:32PM -0400, Robert Haas wrote:
> On Wed, Apr 12, 2017 at 6:30 PM, Peter Eisentraut
> > If I restore a dump into another instance, I need to upgrade all my
> > extensions to that installations's versions, no?  That's not particular
> > to pg_upgrade.
> 
> No, it's an optional step.  If we did the upgrade by default, it would
> greatly increase the chance of something failing.  For example,
> suppose the upgrade does a DROP and then CREATE of a function whose
> signature has changed.  If there's anything that depends on that
> function, this will fail.  But if we don't do it, there's no actual
> problem: the shared library is supposed to be prepared to cope with
> people still using the old SQL definition.  It is probably desirable
> to update where possible to gain access to new features, etc., but it
> shouldn't be required.
> 
> I do think there might be some value in a tool that looked for old
> extensions and tried to update them, but I'm not sure it should be
> pg_dump.

Is pg_upgrade the right place for an extension upgrade script?  When
pg_upgrade started creating an incremental-analyze script, people
thought it should be more generic so it was moved to vacuumdb
--analyze-in-stages.  Seems we should do the same thing for the
extension upgrade script.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +


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

Re: [HACKERS] Letting the client choose the protocol to use during a SASL exchange

[Michael Paquier]

On Thu, Apr 13, 2017 at 6:37 AM, Álvaro Hernández Tortosa
 wrote:
> By looking at the them, and unless I'm missing something, I don't see
> how the extra information for the future implementation of channel binding
> would be added (without changing the protocol). Relevant part is:
>
> The message body is a list of SASL authentication mechanisms, in the
> server's order of preference. A zero byte is required as terminator after
> the last authentication mechanism name. For each mechanism, there is the
> following:
> 
> 
> 
> String
> 
> 
> 
> Name of a SASL authentication mechanism.
> 
> 
> 
> 
> How do you plan to implement it, in future versions, without modifying
> the AuthenticationSASL message? Or is it OK to add new fields to a message
> in future PostgreSQL versions, without considering that a protocol change?

I don't quite understand the complain here, it is perfectly fine to
add as many null-terminated names as you want with this model. The
patches would make the server just send one mechanism name now, but
nothing prevents the addition of more.
-- 
Michael


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

Re: [HACKERS] Interval for launching the table sync worker

[Masahiko Sawada]

On Wed, Apr 12, 2017 at 11:46 PM, Peter Eisentraut
 wrote:
> On 4/12/17 00:48, Masahiko Sawada wrote:
>> On Wed, Apr 12, 2017 at 1:28 PM, Peter Eisentraut
>>> Perhaps instead of a global last_start_time, we store a per relation
>>> last_start_time in SubscriptionRelState?
>>
>> I was thinking the same. But a problem is that the list of
>> SubscriptionRelState is refreshed whenever the syncing table state
>> becomes invalid (table_state_valid = false). I guess we need to
>> improve these logic including GetSubscriptionNotReadyRelations().
>
> The table states are invalidated on a syscache callback from
> pg_subscription_rel, which happens roughly speaking when a table
> finishes the initial sync.  So if we're worried about failing tablesync
> workers relaunching to quickly, this would only be a problem if a
> tablesync of another table finishes right in that restart window.  That
> doesn't seem a terrible issue to me.
>

I think the table states are invalidated whenever the table sync
worker starts, because the table sync worker updates its status of
pg_subscription_rel and commits it before starting actual copy. So we
cannot rely on that. I thought we can store last_start_time into
pg_subscription_rel but it might be overkill. I'm now thinking to
change GetSubscriptionNotReadyRealtions so that last_start_time in
SubscriptionRelState is taken over to new list.

Regards,

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


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

Re: [HACKERS] Letting the client choose the protocol to use during a SASL exchange

[Michael Paquier]

On Thu, Apr 13, 2017 at 2:34 AM, Heikki Linnakangas  wrote:
> On 04/11/2017 02:32 PM, Álvaro Hernández Tortosa wrote:
>>
>>  So I still see your proposal more awkward and less clear, mixing
>> things that are separate. But again, your choice  :)
>
>
> So, here's my more full-fledged proposal.
>
> The first patch refactors libpq code, by moving the responsibility of
> reading the GSS/SSPI/SASL/MD5 specific data from the authentication request
> packet, from the enormous switch-case construct in PQConnectPoll(), into
> pg_fe_sendauth(). This isn't strictly necessary, but I think it's useful
> cleanup anyway, and now that there's a bit more structure in the
> AuthenticationSASL message, the old way was getting awkward.
>
> The second patch contains the protocol changes, and adds the documentation
> for it.

Thanks for the updated patches! I had a close look at them.

Let's begin with 0001...

/*
 * Negotiation generated data to be sent to the client.
 */
-   elog(DEBUG4, "sending SASL response token of length %u", outputlen);
+   elog(DEBUG4, "sending SASL challenge of length %u", outputlen);

sendAuthRequest(port, AUTH_REQ_SASL_CONT, output, outputlen);
+
+   pfree(output);
}
This will leak one byte if output is of length 0. I think that the
pfree call should be moved out of this if condition and only called if
output is not NULL. That's a nit, and in the code this scenario cannot
happen, but I would recommend to be correct.

+static int
+pg_SASL_init(PGconn *conn, int payloadLen)
 {
+   charauth_mechanism[21];
So this assumes that any SASL mechanism name will never be longer than
20 characters. Looking at the link of IANA that Alvaro is providing
upthread this is true, could you add a comment that this relies on
such an assumption?

+   if (initialresponselen != 0)
+   {
+   res = pqPacketSend(conn, 'p', initialresponse, initialresponselen);
+   free(initialresponse);
+
+   if (res != STATUS_OK)
+   return STATUS_ERROR;
+   }
Here also initialresponse could be free'd only if it is not NULL.

And now for 0002...

No much changes in the docs I like the split done for GSS and SSPI messages.

+   /*
+* The StringInfo guarantees that there's a \0 byte after the
+* response.
+*/
+   Assert(input[inputlen] == '\0');
+   pq_getmsgend();
Shouldn't this also check input == NULL? This could crash the
assertion and pg_be_scram_exchange is able to handle a NULL input
message.

+* Parse the list of SASL authentication mechanisms in the
+* AuthenticationSASL message, and select the best mechanism that we
+* support.  (Only SCRAM-SHA-256 is supported at the moment.)
 */
-   if (strcmp(auth_mechanism, SCRAM_SHA256_NAME) == 0)
+   for (;;)
Just an idea here: being able to enforce the selection with an
environment variable (useful for testing as well in the future).
-- 
Michael


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

Re: [HACKERS] Tab completion support for ALTER SUBSCRIPTION REFRESH PUBLICATION

[Fujii Masao]

On Thu, Apr 13, 2017 at 11:05 AM, Masahiko Sawada  wrote:
> On Wed, Apr 12, 2017 at 11:48 PM, Fujii Masao  wrote:
>> On Wed, Apr 12, 2017 at 5:12 PM, Masahiko Sawada  
>> wrote:
>>> Hi,
>>>
>>> I've attached a patch for $subject. Please check it.
>>
>> + COMPLETE_WITH_LIST8("WITH", "CONNECTION", "SET PUBLICATION", "ENABLE",
>> + "DISABLE", "OWNER TO", "RENAME TO", "REFRESH PUBLICATION WITH");
>>
>> "WITH" should be "WITH ("?
>> Also "REFRESH PUBLICATION WITH" should be "REFRESH PUBLICATION WITH ("?
>
> Agreed. I found other bugs of tab completion of PUB/SUB DDLs. Attached
> latest patch incorporated these fixes.

Thanks for the patch! Pushed.

>> BTW, I found that ALTER SUBSCRIPTION ... RENAME TO exists in tab-complete.c,
>> but not in the documentation. ALTER PUBLICATION has the same issue, too.
>> So I think that we need to apply the attached patch which improves the
>> documentation
>> for ALTER PUBLICATION and SUBSCRIPTION.
>
> +1

Also pushed.

Regards,

-- 
Fujii Masao


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

Re: [HACKERS] SUBSCRIPTIONS and pg_upgrade

[Peter Eisentraut]

On 4/11/17 22:16, Peter Eisentraut wrote:
> On 4/10/17 13:58, Peter Eisentraut wrote:
>> Proposal: Dump subscriptions if running as superuser.  If not, check if
>> there are subscriptions and warn about that.  Remove current pg_dump
>> --include-subscriptions option.
> 
> Patch for this part.

And patch for the second part.

I'll commit those in a day or two if there are no new insights.

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


0002-pg_dump-Always-dump-subscriptions-NOCONNECT.patch
Description: invalid/octet-stream

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

Re: [HACKERS] Some thoughts about SCRAM implementation

[Michael Paquier]

On Thu, Apr 13, 2017 at 3:21 AM, Robert Haas  wrote:
> On Wed, Apr 12, 2017 at 2:09 PM, Heikki Linnakangas  wrote:
>> Yes, we need to nail down the protocol and \password before beta. I am
>> working on them now.
>
> Good to hear.

FWIW, there are patches for each issue.
-- 
Michael


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

Re: [HACKERS] Tab completion support for ALTER SUBSCRIPTION REFRESH PUBLICATION

[Masahiko Sawada]

On Wed, Apr 12, 2017 at 11:48 PM, Fujii Masao  wrote:
> On Wed, Apr 12, 2017 at 5:12 PM, Masahiko Sawada  
> wrote:
>> Hi,
>>
>> I've attached a patch for $subject. Please check it.
>
> + COMPLETE_WITH_LIST8("WITH", "CONNECTION", "SET PUBLICATION", "ENABLE",
> + "DISABLE", "OWNER TO", "RENAME TO", "REFRESH PUBLICATION WITH");
>
> "WITH" should be "WITH ("?
> Also "REFRESH PUBLICATION WITH" should be "REFRESH PUBLICATION WITH ("?

Agreed. I found other bugs of tab completion of PUB/SUB DDLs. Attached
latest patch incorporated these fixes.

>
> BTW, I found that ALTER SUBSCRIPTION ... RENAME TO exists in tab-complete.c,
> but not in the documentation. ALTER PUBLICATION has the same issue, too.
> So I think that we need to apply the attached patch which improves the
> documentation
> for ALTER PUBLICATION and SUBSCRIPTION.

+1

Regards,

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


tab_completion_for_pub_sub_ddls_v2.patch
Description: Binary data

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

Re: [HACKERS] Foreign Join pushdowns not working properly for outer joins

[David Rowley]

On 13 April 2017 at 11:22, Peter Eisentraut
 wrote:
> Is this patch considered ready for review as a backpatch candidate?

Yes, however, the v5 patch is based on master. The v4 patch should
apply to 9.6. Diffing the two patches I see another tiny change to a
comment, of which I think needs re-worded anyway.

+ * This function should usually set FDW options in fpinfo after the join is
+ * deemed safe to push down to save some CPU cycles. But We need server
+ * specific options like extensions to decide push-down safety. For
+ * checking extension shippability, we need foreign server as well.
+ */

This might be better written as:

Ordinarily, we might be tempted into delaying the merging of the FDW
options until we've deemed the foreign join to be ok. However, we must
do this before performing this test so that we know which quals can be
evaluated on the foreign server. This may depend on the
shippable_extensions.

Apart from that, it all looks fine to me.

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


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

Re: [HACKERS] Merge join for GiST

[Jeff Davis]

On Tue, Apr 11, 2017 at 8:35 AM, Alexander Korotkov
 wrote:
> On Tue, Apr 11, 2017 at 5:46 PM, Jeff Davis  wrote:
>> Do you have a sense of how this might compare with range merge join?
>
>
> If you have GiST indexes over ranges for both sides of join, then this
> method could be used for range join.  Hence, it could be compared with range
> merge join.
> However, particular implementation in pgsphere uses hardcoded datatypes and
> operations.
> Thus, for range join we need either generalized version of GiST-based join
> or special implementation for ranges.

Alexander, Andrew,

How do you think we should proceed? Which projects do you think should
eventually be in core, versus which are fine as extensions?

Regards,
 Jeff Davis


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

Re: [HACKERS] pg_dump emits ALTER TABLE ONLY partitioned_table

[Amit Langote]

On 2017/04/13 6:22, Robert Haas wrote:
> On Wed, Apr 12, 2017 at 3:29 PM, Stephen Frost  wrote:
>> I'm not following what you're getting at here.
>>
>> There's already a constraint on the table, and ALTER TABLE ONLY doesn't
>> say anything about what happens later on (certainly it doesn't make new
>> tables created with 'LIKE' have bits omitted, if that's what you were
>> thinking).  Lastly, the error being thrown certainly seems to imply that
>> one needs to go fix all the child tables to have the constraint first
>> and then the constraint can be added to the parent (presumably using the
>> same ALTER TABLE ONLY command).  If there aren't any child tables, then
>> it should work, if there *are* child tables and they've got the
>> necessary constraint, then this should be allowed, so that future child
>> tables create will have the constraint.
> 
> So I think I was indeed confused before, and I think you're basically
> right here, but on one point I think you are not right -- ALTER TABLE
> ONLY .. CHECK () doesn't work on a table with inheritance children
> regardless of whether the children already have the matching
> constraint:
> 
> rhaas=# create table foo (a int, b text);
> CREATE TABLE
> rhaas=# create table bar () inherits (foo);
> CREATE TABLE
> rhaas=# alter table only foo add check (a = 1);
> ERROR:  constraint must be added to child tables too
> rhaas=# alter table only bar add check (a = 1);
> ALTER TABLE
> rhaas=# alter table only foo add check (a = 1);
> ERROR:  constraint must be added to child tables too
> 
> It looks like ALTER TABLE ONLY works find on a table with no children,
> but once it's got children it no longer works, period.

By the way, there is a workaround with traditional inheritance:

alter table only foo add constraint chka check (a > 0) no inherit;
ALTER TABLE

But we don't allow NO INHERIT constraints on partitioned tables, so we
will get an error with them anyway.

alter table only parted_parent add constraint chka check (a > 0) no inherit;
ERROR:  cannot add NO INHERIT constraint to partitioned table "parted_parent"

> However,
> you're right that you can add the constraint to the as-yet-childless
> table and then future children will inherit the constraint properly.
> Continuing the previous example:
> 
> rhaas=# drop table bar;
> DROP TABLE
> rhaas=# alter table only foo add check (a = 1);
> ALTER TABLE
> rhaas=# create table bar () inherits (foo);
> CREATE TABLE
> 
> So, regarding Amit's 0001:
> 
> - I think we should update the relevant hunk of the documentation
> rather than just removing it.

OK, I agree.  I tweaked the existing bullet point about differences from
traditional inheritance when using ONLY with partitioned tables.

> - Should we similarly allow TRUNCATE ONLY foo and ALTER TABLE ONLY foo
> .. to work on a partitioned table without partitions, or is that just
> pointless tinkering?  That seems to be the only case where, after this
> patch, an ONLY operation will fail on a childless partitioned table.

I fixed TRUNCATE ONLY to not complain when no partitions exist.  Patch
already takes care of the ALTER TABLE ONLY cases.

Updated patches attached (0002 and 0003 unchanged).

Thanks,
Amit

>From 730a71a1120d37065e26338566fcbca65d1a03e8 Mon Sep 17 00:00:00 2001
From: amit 
Date: Wed, 12 Apr 2017 17:58:07 +0900
Subject: [PATCH 1/3] Fix ALTER TABLE ONLY to avoid unnecessarily failures

Currently, ALTER TABLE ONLY would fail in certain cases when applied
to partitioned tables, even if no partitions yet exist.  Although,
it seems perfectly OK to allow the same, especially because pg_dump
sometimes outputs constraints separately using ALTER TABLE ONLY.
---
 doc/src/sgml/ddl.sgml | 13 +++---
 src/backend/commands/tablecmds.c  | 69 +++
 src/test/regress/expected/alter_table.out | 32 +-
 src/test/regress/expected/truncate.out|  5 +++
 src/test/regress/sql/alter_table.sql  | 24 ---
 src/test/regress/sql/truncate.sql |  4 ++
 6 files changed, 98 insertions(+), 49 deletions(-)

diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml
index 340c961b3f..ae2072b1de 100644
--- a/doc/src/sgml/ddl.sgml
+++ b/doc/src/sgml/ddl.sgml
@@ -2944,17 +2944,18 @@ VALUES ('Albany', NULL, NULL, 'NY');
Both CHECK and NOT NULL
constraints of a partitioned table are always inherited by all its
partitions.  CHECK constraints that are marked
-   NO INHERIT are not allowed.
+   NO INHERIT are not allowed to be created on
+   partitioned tables.
   
  
 
  
   
-   The ONLY notation used to exclude child tables
-   will cause an error for partitioned tables in the case of
-   schema-modifying commands such as most ALTER TABLE
-   commands.  For example, dropping a column from only the parent does
-   not make sense for partitioned tables.
+   Using ONLY to affect only the 

Re: [HACKERS] Partitioned tables and relfilenode

[Amit Langote]

On 2017/04/13 0:36, Robert Haas wrote:
> On Tue, Apr 11, 2017 at 10:15 PM, Amit Langote
>  wrote:
>> Alright.  So I made it into two patches instead: 0001 fixes the bug that
>> validateCheckConstraint() tries to scan partitioned tables and 0002 makes
>> trying to convert a partitioned table to a view a user error.
> 
> Committed together, after updating the regression test outputs to make
> the tests pass.  You forgot to update the expected output file in
> 0002.

Oops, sorry about that.  Thanks for fixing and committing.

Regards,
Amit



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

Re: [HACKERS] Function to control physical replication slot

[Andres Freund]

On 2017-04-12 20:15:52 -0400, Peter Eisentraut wrote:
> On 4/11/17 05:15, Magnus Hagander wrote:
> > Is there a particular reason we don't have a function to *set* the
> > restart_lsn of a replication slot, other than to drop it and recreate it?
> 
> I suppose there could be lots of problems if the LSN you specify isn't
> valid.  And it might be hard to determine whether a given LSN is valid.

As long as we're only talking about the LSN of a physical slot (and not
the xmin) I'm not sure it's that important that it's valid, as long as
it's not in the future.  But we could otherwise pretty easily assert
that the new value has to be old_value <= new_value <=
GetRedoRecPtr()/GetFlushRecPtr().  That should be sufficient for both of
your use-cases afaics?

- Andres


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

Re: [HACKERS] snapbuild woes

[Andres Freund]

Hi,
On 2017-03-03 01:30:11 +0100, Petr Jelinek wrote:

> From 7d5b48c8cb80e7c867b2096c999d08feda50b197 Mon Sep 17 00:00:00 2001
> From: Petr Jelinek 
> Date: Fri, 24 Feb 2017 21:39:03 +0100
> Subject: [PATCH 1/5] Reserve global xmin for create slot snasphot export
> 
> Otherwise the VACUUM or pruning might remove tuples still needed by the
> exported snapshot.
> ---
>  src/backend/replication/logical/logical.c | 21 -
>  1 file changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/src/backend/replication/logical/logical.c 
> b/src/backend/replication/logical/logical.c
> index 5529ac8..57c392c 100644
> --- a/src/backend/replication/logical/logical.c
> +++ b/src/backend/replication/logical/logical.c
> @@ -267,12 +267,18 @@ CreateInitDecodingContext(char *plugin,
>* the slot machinery about the new limit. Once that's done the
>* ProcArrayLock can be released as the slot machinery now is
>* protecting against vacuum.
> +  *
> +  * Note that we only store the global xmin temporarily in the in-memory
> +  * state so that the initial snapshot can be exported. After initial
> +  * snapshot is done global xmin should be reset and not tracked anymore
> +  * so we are fine with losing the global xmin after crash.
>* 
>*/
>   LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);
>  
>   slot->effective_catalog_xmin = GetOldestSafeDecodingTransactionId();
>   slot->data.catalog_xmin = slot->effective_catalog_xmin;
> + slot->effective_xmin = slot->effective_catalog_xmin;


>  void
>  FreeDecodingContext(LogicalDecodingContext *ctx)
>  {
> + ReplicationSlot *slot = MyReplicationSlot;
> +
>   if (ctx->callbacks.shutdown_cb != NULL)
>   shutdown_cb_wrapper(ctx);
>  
> + /*
> +  * Cleanup global xmin for the slot that we may have set in
> +  * CreateInitDecodingContext().

Hm.  Is that actually a meaningful point to do so? For one, it gets
called by pg_logical_slot_get_changes_guts(), but more importantly, the
snapshot is exported till SnapBuildClearExportedSnapshot(), which is the
next command?  If we rely on the snapshot magic done by ExportSnapshot()
it'd be worthwhile to mention that...


> We do not take ProcArrayLock or similar
> +  * since we only reset xmin here and there's not much harm done by a
> +  * concurrent computation missing that.
> +  */

Hum.  I was prepared to complain about this, but ISTM, that there's
absolutely no risk because the following
ReplicationSlotsComputeRequiredXmin(false); actually does all the
necessary locking?  But still, I don't see much point in the
optimization.



> This patch changes the code so that stored snapshots are only used for
> logical decoding restart but not for initial slot snapshot.

Yea, that's a very good point...

> @@ -1284,13 +1286,13 @@ SnapBuildFindSnapshot(SnapBuild *builder, XLogRecPtr 
> lsn, xl_running_xacts *runn
>  
>   return false;
>   }
> - /* c) valid on disk state */
> - else if (SnapBuildRestore(builder, lsn))
> + /* c) valid on disk state and not exported snapshot */
> + else if (!TransactionIdIsNormal(builder->initial_xmin_horizon) &&
> +  SnapBuildRestore(builder, lsn))

Hm. Is this a good signaling mechanism? It'll also trigger for the SQL
interface, where it'd strictly speaking not be required, right?


> From 3318a929e691870f3c1ca665bec3bfa8ea2af2a8 Mon Sep 17 00:00:00 2001
> From: Petr Jelinek 
> Date: Sun, 26 Feb 2017 01:07:33 +0100
> Subject: [PATCH 3/5] Prevent snapshot builder xmin from going backwards

A bit more commentary would be good. What does that protect us against?



> From 53193b40f26dd19c712f3b9b77af55f81eb31cc4 Mon Sep 17 00:00:00 2001
> From: Petr Jelinek 
> Date: Wed, 22 Feb 2017 00:57:33 +0100
> Subject: [PATCH 4/5] Fix xl_running_xacts usage in snapshot builder
> 
> Due to race condition, the xl_running_xacts might contain no longer
> running transactions. Previous coding tried to get around this by
> additional locking but that did not work correctly for committs. Instead
> try combining decoded commits and multiple xl_running_xacts to get the
> consistent snapshot.

Needs more explanation about approach.


> @@ -1221,7 +1221,12 @@ SnapBuildFindSnapshot(SnapBuild *builder, XLogRecPtr 
> lsn, xl_running_xacts *runn
>*simply track the number of in-progress toplevel transactions 
> and
>*lower it whenever one commits or aborts. When that number
>*(builder->running.xcnt) reaches zero, we can go from 
> FULL_SNAPSHOT
> -  *to CONSISTENT.
> +  *to CONSISTENT. Sometimes we might get xl_running_xacts which 
> has
> +  *all tracked transactions as finished. We'll need to restart 
> tracking
> +  *in that case and use previously collected committed 
> transactions to
> +  *

Re: [HACKERS] Function to control physical replication slot

[Peter Eisentraut]

On 4/11/17 05:15, Magnus Hagander wrote:
> Is there a particular reason we don't have a function to *set* the
> restart_lsn of a replication slot, other than to drop it and recreate it?

I suppose there could be lots of problems if the LSN you specify isn't
valid.  And it might be hard to determine whether a given LSN is valid.

If this could be made to work, this would actually offer an interesting
option for dumping and restoring subscriptions.

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


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

Re: [HACKERS] pg_upgrade vs extension upgrades

[Peter Eisentraut]

On 4/12/17 18:59, Robert Haas wrote:
> I do think there might be some value in a tool that looked for old
> extensions and tried to update them, but I'm not sure it should be
> pg_dump.

This reminds me a bit of the problem of upgrading all collations after
an upgrade.  Perhaps we can find similar solutions to both problems.

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


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

Re: [HACKERS] pg_statistic_ext.staenabled might not be the best column name

[Tomas Vondra]

On 04/12/2017 03:36 PM, David Rowley wrote:


"stakind" seems like a better name. I'd have personally gone with
"statype" but pg_statistic already thinks stakind is better.


+1 to stakind

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


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

Re: [HACKERS] Foreign Join pushdowns not working properly for outer joins

[Peter Eisentraut]

Is this patch considered ready for review as a backpatch candidate?

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


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

Re: [pgsql-www] [HACKERS] Small issue in online devel documentation build

[Peter Eisentraut]

On 4/12/17 09:50, Bruce Momjian wrote:
> On Wed, Apr 12, 2017 at 01:31:51PM +0200, Magnus Hagander wrote:
>> I think that only leaves the change to the javascript code that Bruce sent.
>> Let's see if we can figure out a way to do that one without requiring
>> javascript, but after that we have covered all listed issues I think?
> 
> Well, we have been using JavaScript for years, so my patch only fixes
> the JavaScript we already have.  Seems applying the fix now makes sense
> and then we can ponder other methods.

I have googled a bit.  These two articles describe what I think is the
issue we are trying to address:

http://code.stephenmorley.org/html-and-css/fixing-browsers-broken-monospace-font-handling/

http://meyerweb.com/eric/thoughts/2010/02/12/fixed-monospace-sizing/

The recipe in the first one seems to work somewhat, but not quite as
well as the current javascript.  (The recipe in the second article is
ultimately equivalent but more verbose.)

I tend to agree with Bruce that we could just fix this within the
current framework.  But if we want to make a change, there is the
information.

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


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

Re: [HACKERS] pg_upgrade vs extension upgrades

[Robert Haas]

On Wed, Apr 12, 2017 at 6:30 PM, Peter Eisentraut
 wrote:
> On 4/10/17 11:30, Magnus Hagander wrote:
>> After you've run pg_upgrade, you have to loop through all your databases
>> and do an "ALTER EXTENSION abc UPDATE" once for each extension.
>>
>> Is there a reason we shouldn't have pg_upgrade emit a script that does
>> this, similar to how it emits a script to run ANALYZE?
>
> Shouldn't pg_dump do this, and perhaps by default?
>
> If I restore a dump into another instance, I need to upgrade all my
> extensions to that installations's versions, no?  That's not particular
> to pg_upgrade.

No, it's an optional step.  If we did the upgrade by default, it would
greatly increase the chance of something failing.  For example,
suppose the upgrade does a DROP and then CREATE of a function whose
signature has changed.  If there's anything that depends on that
function, this will fail.  But if we don't do it, there's no actual
problem: the shared library is supposed to be prepared to cope with
people still using the old SQL definition.  It is probably desirable
to update where possible to gain access to new features, etc., but it
shouldn't be required.

I do think there might be some value in a tool that looked for old
extensions and tried to update them, but I'm not sure it should be
pg_dump.

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


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

Re: [HACKERS] Re: Query fails when SRFs are part of FROM clause (Commit id: 69f4b9c85f)

[Andres Freund]

On 2017-04-05 09:39:37 -0400, Tom Lane wrote:
> Andres Freund  writes:
> > On 2017-04-05 02:47:55 -0400, Noah Misch wrote:
> >> [Action required within three days.  This is a generic notification.]
>
> > I've a very preliminary patch.  I'd like to only start polishing it up
> > once the code freeze is over, so I can work on getting some patches in -
> > note that I myself have no pending patches.  Once frozen I'll polish it
> > up and send that within a few days.
>
> FWIW, I'm willing to help out on this.

Help would be appreciated.  I've pondered, and partially implemented,
several approaches so far, and my conclusion is that we should just do
nothing.  The amount of corner cases is just too big, and the utility of
the feature too small.

To recap, the issue is that in previous versions it was, by accident
(there's no test, code comments "supporting" the feature talk about a
different corner case, and the behaviour isn't correct in some cases)
allowed to do something like:
SELECT * FROM CAST(generate_series(1,3) * 5 AS int);
while
SELECT * FROM generate_series(1,3) * 5;
is not allowed.

The reason that that works from the gram.y perspective is that CAST etc
types of func_expr's.  The reason that it worked from a code perspective
is that execQual.c:ExecMakeTableFunctionResult() has the following:
/*
 * Normally the passed expression tree will be a FuncExprState, since 
the
 * grammar only allows a function call at the top level of a table
 * function reference.  However, if the function doesn't return set then
 * the planner might have replaced the function call via 
constant-folding
 * or inlining.  So if we see any other kind of expression node, execute
 * it via the general ExecEvalExpr() code; the only difference is that 
we
 * don't get a chance to pass a special ReturnSetInfo to any functions
 * buried in the expression.
 */
if (funcexpr && IsA(funcexpr, FuncExprState) &&
IsA(funcexpr->expr, FuncExpr))
and back then ExecEvalExpr() was allowed to return sets.  It also
depends on some default initializations (e.g. rsinfo.returnMode =
SFRM_ValuePerCall).

This yields plenty weird behaviour in < v10. E.g. the following is
disallowed:
  SELECT * FROM int4mul(generate_series(1,3), 1);
  ERROR:  0A000: set-valued function called in context that cannot accept a set
as is
  SELECT * FROM CAST(int4mul(generate_series(1,3), 1) AS bigint);
  ERROR:  0A000: set-valued function called in context that cannot accept a set
because the cast is implemented as int8(expr) which avoids the fallback
path as it's a FuncExpr, but
  SELECT * FROM CAST(int4mul(generate_series(1,3), 1) AS text);
works because the cast is implemented as a io coercion, which is not a
funcexpr. Therefore it uses the fallback ExecEvalExpr().

The mismatch between ExecEvalExpr() and nodeFunctionscan.c SRF behaviour
back then also yields odd behaviour, e.g.:
  SELECT * FROM generate_series(1,0);
returns zero rows, but
  SELECT * FROM CAST(generate_series(1,0) * 5 AS INT);
returns one NULL row.


In v10, as it stands, these all error out, because the SRFs are now only
to be evaluated via either nodeFunctionscan.c (FROM) or via
nodeProjectSet.c (targetlist), not ExecEvalExpr() anymore.


I've basically pondered three different methods of implementing
something akin to the old behaviour:

1) Move the non-SRF part into nodeFunctionscan.c's targetlist, and let
   it be evaluated there.  E.g. if the expression is
   CAST(generate_series(1,5) AS text), evaluate the generate_series(1,5)
   using nodeFunctionscan's FunctionScanPerFuncState machinery, but
   implement the CAST as CAST(Var(whatever, 1) AS Text).

   That doesn't work well for composite/record returning rows, because
   RangeTblFunction's returning composites are expanded into multiple
   columns. E.g.
   SELECT * FROM CAST(CAST(twocol_noset_outline(generate_series(1,3), 'text') 
AS text) AS twocol);
   returns all the columns of twocol, not a single wholerow datum.

   There's also some issues around what to do with cases involving
   volatile functions when the output is not referenced, or referenced
   multiple times e.g.
 SELECT f, f FROM CAST(generate_series(1,3) * nextval(...)) AS f;
   would evaluate nextval() twice with such an approach...

2) During planning, split of the SRF bit from the !SRF bit and have
   nodeFunctionscan.c evaluate both separately, like 1).  That allows to
   avoid the volatility issue, because the targetlist is still projected
   separately.

   I've prototyped this to a reasonable point, by having
   create_functionscan_plan() process each RangeTblFunction and split
   the expression into SRF and non-SRF parts, and then have
   FunctionNext() evaluate the non-SRF part.

   At the current state of my prototype this happens to allow simple SRF
   in arguments cases like SELECT * FROM int4mul(generate_series(1,3), 1)
   which are 

Re: [pgsql-www] [HACKERS] Small issue in online devel documentation build

[Peter Eisentraut]

On 4/12/17 15:43, Peter Eisentraut wrote:
> On 4/12/17 07:31, Magnus Hagander wrote:
>> Once difference I notice is that for example the "note boxes" are no
>> longer centered, but they do now get the new formatting.
> 
> I have committed something for that.  The issue was that the generated
> HTML contained hard-coded style attributes.

This appears to have been successful.

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


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

Re: [HACKERS] SUBSCRIPTIONS and pg_upgrade

[Peter Eisentraut]

On 4/11/17 23:41, Noah Misch wrote:
> On Tue, Apr 11, 2017 at 11:21:24PM -0400, Peter Eisentraut wrote:
>> On 4/9/17 22:16, Noah Misch wrote:
>>> [Action required within three days.  This is a generic notification.]
>>
>> Patches have been posted.  Discussion is still going on a bit.
> 
> By what day should the community look for your next update?

tomorrow

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


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

Re: [HACKERS] pg_upgrade vs extension upgrades

[Peter Eisentraut]

On 4/10/17 11:30, Magnus Hagander wrote:
> After you've run pg_upgrade, you have to loop through all your databases
> and do an "ALTER EXTENSION abc UPDATE" once for each extension.
> 
> Is there a reason we shouldn't have pg_upgrade emit a script that does
> this, similar to how it emits a script to run ANALYZE?

Shouldn't pg_dump do this, and perhaps by default?

If I restore a dump into another instance, I need to upgrade all my
extensions to that installations's versions, no?  That's not particular
to pg_upgrade.

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


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

Re: [HACKERS] Letting the client choose the protocol to use during a SASL exchange

[Álvaro Hernández Tortosa]

On 12/04/17 19:34, Heikki Linnakangas wrote:

On 04/11/2017 02:32 PM, Álvaro Hernández Tortosa wrote:

 So I still see your proposal more awkward and less clear, mixing
things that are separate. But again, your choice  :)


So, here's my more full-fledged proposal.

The first patch refactors libpq code, by moving the responsibility of 
reading the GSS/SSPI/SASL/MD5 specific data from the authentication 
request packet, from the enormous switch-case construct in 
PQConnectPoll(), into pg_fe_sendauth(). This isn't strictly necessary, 
but I think it's useful cleanup anyway, and now that there's a bit 
more structure in the AuthenticationSASL message, the old way was 
getting awkward.


The second patch contains the protocol changes, and adds the 
documentation for it.


- Heikki



Hi Heikki.

Thanks for the patches :)

By looking at the them, and unless I'm missing something, I don't 
see how the extra information for the future implementation of channel 
binding would be added (without changing the protocol). Relevant part is:


The message body is a list of SASL authentication mechanisms, in the
server's order of preference. A zero byte is required as terminator after
the last authentication mechanism name. For each mechanism, there is the
following:



String



Name of a SASL authentication mechanism.






How do you plan to implement it, in future versions, without 
modifying the AuthenticationSASL message? Or is it OK to add new fields 
to a message in future PostgreSQL versions, without considering that a 
protocol change?


On a side note, I'd mention that the list of SASL authentication 
mechanisms contains valid IANA Registry SCRAM names 
(https://www.iana.org/assignments/sasl-mechanisms/sasl-mechanisms.xhtml#scram)for 
SCRAM authentication messages (making it more clear what values would be 
expected there from the client).


I hope to start testing this from Java the coming weekend. I will 
keep you posted.



Álvaro


--

Álvaro Hernández Tortosa


---
<8K>data



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

Re: [HACKERS] pg_dump emits ALTER TABLE ONLY partitioned_table

[Robert Haas]

On Wed, Apr 12, 2017 at 3:29 PM, Stephen Frost  wrote:
> I'm not following what you're getting at here.
>
> There's already a constraint on the table, and ALTER TABLE ONLY doesn't
> say anything about what happens later on (certainly it doesn't make new
> tables created with 'LIKE' have bits omitted, if that's what you were
> thinking).  Lastly, the error being thrown certainly seems to imply that
> one needs to go fix all the child tables to have the constraint first
> and then the constraint can be added to the parent (presumably using the
> same ALTER TABLE ONLY command).  If there aren't any child tables, then
> it should work, if there *are* child tables and they've got the
> necessary constraint, then this should be allowed, so that future child
> tables create will have the constraint.

So I think I was indeed confused before, and I think you're basically
right here, but on one point I think you are not right -- ALTER TABLE
ONLY .. CHECK () doesn't work on a table with inheritance children
regardless of whether the children already have the matching
constraint:

rhaas=# create table foo (a int, b text);
CREATE TABLE
rhaas=# create table bar () inherits (foo);
CREATE TABLE
rhaas=# alter table only foo add check (a = 1);
ERROR:  constraint must be added to child tables too
rhaas=# alter table only bar add check (a = 1);
ALTER TABLE
rhaas=# alter table only foo add check (a = 1);
ERROR:  constraint must be added to child tables too

It looks like ALTER TABLE ONLY works find on a table with no children,
but once it's got children it no longer works, period.  However,
you're right that you can add the constraint to the as-yet-childless
table and then future children will inherit the constraint properly.
Continuing the previous example:

rhaas=# drop table bar;
DROP TABLE
rhaas=# alter table only foo add check (a = 1);
ALTER TABLE
rhaas=# create table bar () inherits (foo);
CREATE TABLE

So, regarding Amit's 0001:

- I think we should update the relevant hunk of the documentation
rather than just removing it.

- Should we similarly allow TRUNCATE ONLY foo and ALTER TABLE ONLY foo
.. to work on a partitioned table without partitions, or is that just
pointless tinkering?  That seems to be the only case where, after this
patch, an ONLY operation will fail on a childless partitioned table.

Do you want to be responsible for committing these or should I do it?

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


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

Re: [HACKERS] pg_statistic_ext.staenabled might not be the best column name

[Robert Haas]

On Wed, Apr 12, 2017 at 9:36 AM, David Rowley
 wrote:
> I'd been thinking that staenabled is not the most suitable column name
> for storing the types of statistics that are defined for the extended
> statistics.

+1.

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


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

Re: [HACKERS] Patch: Write Amplification Reduction Method (WARM)

[Peter Geoghegan]

On Wed, Apr 12, 2017 at 10:12 AM, Robert Haas  wrote:
>> I may have missed something, but there is no intention to ignore known
>> regressions/reviews. Of course, I don't think that every regression will be
>> solvable, like if you run a CPU-bound workload, setting it up in a way such
>> that you repeatedly exercise the area where WARM is doing additional work,
>> without providing any benefit, may be you can still find regression. I am
>> willing to fix them as long as they are fixable and we are comfortable with
>> the additional code complexity. IMHO certain trade-offs are good, but I
>> understand that not everybody will agree with my views and that's ok.
>
> The point here is that we can't make intelligent decisions about
> whether to commit this feature unless we know which situations get
> better and which get worse and by how much.  I don't accept as a
> general principle the idea that CPU-bound workloads don't matter.
> Obviously, I/O-bound workloads matter too, but we can't throw
> CPU-bound workloads under the bus.  Now, avoiding index bloat does
> also save CPU, so it is easy to imagine that WARM could come out ahead
> even if each update consumes slightly more CPU when actually updating,
> so we might not actually regress.  If we do, I guess I'd want to know
> why.

I myself wonder if this CPU overhead is at all related to LP_DEAD
recycling during page splits. I have my suspicions that the recyling
has some relationship to locality, which leads me to want to
investigate how Claudio Freire's patch to consistently treat heap TID
as part of the B-Tree sort order could help, both in general, and for
WARM.

Bear in mind that the recycling has to happen with an exclusive buffer
lock held on a leaf page, which could hold up rather a lot of scans
that need to visit the same value even if it's on some other,
relatively removed leaf page.

This is just a theory.

-- 
Peter Geoghegan

VMware vCenter Server
https://www.vmware.com/


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

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

[Peter Eisentraut]

On 4/12/17 09:55, Fujii Masao wrote:
> To fix this issue, we should terminate walsender for logical replication
> before shutdown checkpoint starts. Of course walsender for physical
> replication still needs to keep running until shutdown checkpoint ends,
> though.

Can we turn it into a kind of read-only or no-new-commands mode instead,
so it can keep streaming but not accept any new actions?

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


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

Re: [HACKERS] pg_dump emits ALTER TABLE ONLY partitioned_table

[Robert Haas]

On Wed, Apr 12, 2017 at 3:29 PM, Stephen Frost  wrote:
> * Robert Haas (robertmh...@gmail.com) wrote:
>> On Wed, Apr 12, 2017 at 6:29 AM, Amit Langote
>>  wrote:
>> > Actually, p1 is a partitioned table, so the error.  And I realize that
>> > that's a wrong behavior.  Currently the check is performed using only the
>> > relkind, which is bogus.  Specifying ONLY should cause an error only when
>> > the table has partitions.
>>
>> That sounds like a REALLY bad idea, because now you're going to end up
>> with a constraint that can never be enforced against any actual data
>> rows ... or else you're going to later pretend that ONLY wasn't
>> specified.  I think the rule that partitioned tables can't have
>> non-inherited constraints is absolutely right, and relaxing it is
>> quite wrong.
>
> I'm not following what you're getting at here.

Urk, I might be confusing ONLY with NO INHERIT.  Let me think about
this again...

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


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

Re: [pgsql-www] [HACKERS] Small issue in online devel documentation build

[Peter Eisentraut]

On 4/12/17 07:31, Magnus Hagander wrote:
> Once difference I notice is that for example the "note boxes" are no
> longer centered, but they do now get the new formatting.

I have committed something for that.  The issue was that the generated
HTML contained hard-coded style attributes.

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


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

Re: [HACKERS] TAP tests take a long time

[Stephen Frost]

Andrew,

* Andrew Dunstan (andrew.duns...@2ndquadrant.com) wrote:
> On 04/12/2017 08:40 AM, Andrew Dunstan wrote:
> > On 04/12/2017 01:27 AM, Craig Ringer wrote:
> >> BTW, I suggest adding --timer to our default PROVE_FLAGS, so we can
> >> collect more data from the buildfarm on what takes a while. Sample
> >> output:
> >
> > I'll add that to the commandline the buildfarm uses in the upcoming release.
> 
> See
> 
> 
> One thing I noticed was this
> 
> [10:02:40] t/001_basic.pl . ok  320 ms
> [10:02:59] t/002_pg_dump.pl ... ok19441 ms
> [10:03:47] t/010_dump_connstr.pl .. ok47430 ms
> 
> Why does the last test set take 47s?

It's doing a bunch of dropdb/createdb stuff, and also creates multiple
clusters.  Part of this is because it's generating random names for
databases to make sure they are handled correctly.  I'd certainly love
to see it take less time but I've not really done very much with it.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Vacuum: allow usage of more than 1GB of work mem

[Robert Haas]

On Tue, Apr 11, 2017 at 4:38 PM, Claudio Freire  wrote:
> In essence, the patch as it is proposed, doesn't *need* a binary
> search, because the segment list can only grow up to 15 segments at
> its biggest, and that's a size small enough that linear search will
> outperform (or at least perform as well as) binary search. Reducing
> the initial segment size wouldn't change that. If the 12GB limit is
> lifted, or the maximum segment size reduced (from 1GB to 128MB for
> example), however, that would change.
>
> I'd be more in favor of lifting the 12GB limit than of reducing the
> maximum segment size, for the reasons above. Raising the 12GB limit
> has concrete and readily apparent benefits, whereas using bigger (or
> smaller) segments is far more debatable. Yes, that will need a binary
> search. But, I was hoping that could be a second (or third) patch, to
> keep things simple, and benefits measurable.

To me, it seems a bit short-sighted to say, OK, let's use a linear
search because there's this 12GB limit so we can limit ourselves to 15
segments.  Because somebody will want to remove that 12GB limit, and
then we'll have to revisit the whole thing anyway.  I think, anyway.

What's not clear to me is how sensitive the performance of vacuum is
to the number of cycles used here.  For a large index, the number of
searches will presumably be quite large, so it does seem worth
worrying about performance.  But if we just always used a binary
search, would that lose enough performance with small numbers of
segments that anyone would care?  If so, maybe we need to use linear
search for small numbers of segments and switch to binary search with
larger numbers of segments.

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


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

Re: [HACKERS] Inadequate parallel-safety check for SubPlans

[Tom Lane]

Robert Haas  writes:
> On Wed, Apr 12, 2017 at 2:41 PM, Tom Lane  wrote:
>> This is 100% wrong.  It's failing to recurse into the subexpressions of
>> the SubPlan, which could very easily include parallel-unsafe function
>> calls.

> My understanding (apparently flawed?) is that the parallel_safe flag
> on the SubPlan is supposed to encapsulate whether than SubPlan is
> entirely parallel safe, so that no recursion is needed.  If the
> parallel_safe flag on the SubPlan is being set wrong, doesn't that
> imply that the Path from which the subplan was created had the wrong
> value of this flag, too?

> ...

> Oh, I see.  The testexpr is separate from the Path.  Oops.

Right.  Although you're nudging up against an alternate idea that
I just had: we could define the parallel_safe flag on the SubPlan as
encapsulating not only whether the child plan is safe, but whether
the contained testexpr (and args) are safe.  If we were to make
an is_parallel_safe() check on the testexpr before injecting
PARAM_EXEC Params into it, and record the results in the SubPlan,
maybe that would lead to a simpler answer.

OTOH, that might not work out nicely; and I have a feeling that
we'd end up needing a whitelist anyway later, to handle the
case of correlated subplans.

> Sounds reasonable.  Do you want to have a go at that?

Yeah.  I'm knocking off for the day a bit early today, but I'll have
a look at it tomorrow, unless anyone in the Far East beats me to it.

regards, tom lane


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

Re: [HACKERS] TAP tests take a long time

[Andrew Dunstan]

On 04/12/2017 08:40 AM, Andrew Dunstan wrote:
>
> On 04/12/2017 01:27 AM, Craig Ringer wrote:
>> BTW, I suggest adding --timer to our default PROVE_FLAGS, so we can
>> collect more data from the buildfarm on what takes a while. Sample
>> output:
>>
>
> I'll add that to the commandline the buildfarm uses in the upcoming release.
>


See


One thing I noticed was this

[10:02:40] t/001_basic.pl . ok  320 ms
[10:02:59] t/002_pg_dump.pl ... ok19441 ms
[10:03:47] t/010_dump_connstr.pl .. ok47430 ms

Why does the last test set take 47s?

cheers

andrew

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



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

Re: [HACKERS] Inadequate parallel-safety check for SubPlans

[Robert Haas]

On Wed, Apr 12, 2017 at 2:41 PM, Tom Lane  wrote:
> While poking at the question of parallel_safe marking for Plans,
> I noticed that max_parallel_hazard_walker() does this:
>
> /* We can push the subplans only if they are parallel-safe. */
> else if (IsA(node, SubPlan))
> return !((SubPlan *) node)->parallel_safe;
>
> This is 100% wrong.  It's failing to recurse into the subexpressions of
> the SubPlan, which could very easily include parallel-unsafe function
> calls.

My understanding (apparently flawed?) is that the parallel_safe flag
on the SubPlan is supposed to encapsulate whether than SubPlan is
entirely parallel safe, so that no recursion is needed.  If the
parallel_safe flag on the SubPlan is being set wrong, doesn't that
imply that the Path from which the subplan was created had the wrong
value of this flag, too?

...

Oh, I see.  The testexpr is separate from the Path.  Oops.

> Basically, therefore, this logic is totally inadequate.  I think what
> we need to do is improve matters so that while looking at the testexpr
> of a SubPlan, we pass down a whitelist saying that the Params having
> the numbers indicated by the SubLink's paramIds list are OK.

Sounds reasonable.  Do you want to have a go at that?

> I'm also suspicious that the existing dumb treatment of Params here
> may be blocking recognition that correlated subplans are parallel-safe.
> But that would only be a failure to apply a possible optimization,
> not a failure to generate a valid plan.

Smarter treatment of Params would be very nice, but possibly hard.

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


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

Re: [HACKERS] pg_dump emits ALTER TABLE ONLY partitioned_table

[Stephen Frost]

Robert,

* Robert Haas (robertmh...@gmail.com) wrote:
> On Wed, Apr 12, 2017 at 6:29 AM, Amit Langote
>  wrote:
> > Actually, p1 is a partitioned table, so the error.  And I realize that
> > that's a wrong behavior.  Currently the check is performed using only the
> > relkind, which is bogus.  Specifying ONLY should cause an error only when
> > the table has partitions.
> 
> That sounds like a REALLY bad idea, because now you're going to end up
> with a constraint that can never be enforced against any actual data
> rows ... or else you're going to later pretend that ONLY wasn't
> specified.  I think the rule that partitioned tables can't have
> non-inherited constraints is absolutely right, and relaxing it is
> quite wrong.

I'm not following what you're getting at here.

There's already a constraint on the table, and ALTER TABLE ONLY doesn't
say anything about what happens later on (certainly it doesn't make new
tables created with 'LIKE' have bits omitted, if that's what you were
thinking).  Lastly, the error being thrown certainly seems to imply that
one needs to go fix all the child tables to have the constraint first
and then the constraint can be added to the parent (presumably using the
same ALTER TABLE ONLY command).  If there aren't any child tables, then
it should work, if there *are* child tables and they've got the
necessary constraint, then this should be allowed, so that future child
tables create will have the constraint.

> I think you had the right idea upthread when you suggested dumping it this 
> way:
> 
> CREATE TABLE p1 PARTITION OF p (
> b NOT NULL
> )
> FOR VALUES IN (1)
> PARTITION BY RANGE (b);
> 
> That looks absolutely right to me, and very much principled.

It's not principled at all to claim that CREATE TABLE + NOT NULL is
somehow different from CREATE TABLE + ALTER TABLE SET NOT NULL and that
one should work and the other shouldn't.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Adding support for Default partition in partitioning

[Robert Haas]

On Thu, Apr 6, 2017 at 7:30 AM, Rahila Syed  wrote:
> Thanks a lot for testing and reporting this. Please find attached an updated
> patch with the fix. The patch also contains a fix
> regarding operator used at the time of creating expression as default
> partition constraint. This was notified offlist by Amit Langote.

I think that the syntax for this patch should probably be revised.
Right now the proposal is for:

CREATE TABLE .. PARTITION OF ... FOR VALUES IN (DEFAULT);

But that's not a good idea for several reasons.  For one thing, you
can also write FOR VALUES IN (DEFAULT, 5) or which isn't sensible.
For another thing, this kind of syntax won't generalize to range
partitioning, which we've talked about making this feature support.
Maybe something like:

CREATE TABLE .. PARTITION OF .. DEFAULT;

This patch makes the assumption throughout that any DefElem represents
the word DEFAULT, which is true in the patch as written but doesn't
seem very future-proof.  I think the "def" in "DefElem" stands for
"definition" or "define" or something like that, so this is actually
pretty confusing.  Maybe we should introduce a dedicated node type to
represent a default-specification in the parser grammar.  If not, then
let's at least encapsulate the test a little better, e.g. by adding
isDefaultPartitionBound() which tests not only IsA(..., DefElem) but
also whether the name is DEFAULT as expected.  BTW, we typically use
lower-case internally, so if we stick with this representation it
should really be "default" not "DEFAULT".

Useless hunk:

+boolhas_def;/* Is there a default partition?
Currently false
+ * for a range partitioned table */
+intdef_index;/* Index of the default list
partition. -1 for
+ * range partitioned tables */

Why abbreviate "default" to def here?  Seems pointless.

+if (found_def)
+{
+if (mapping[def_index] == -1)
+mapping[def_index] = next_index++;
+}

Consider &&

@@ -717,7 +754,6 @@ check_new_partition_bound(char *relname, Relation
parent, Node *bound)
 }
 }
 }
-
 break;
 }

+ * default partiton for rows satisfying the new partition

Spelling.

+ * constraint. If found dont allow addition of a new partition.

Missing apostrophe.

+defrel = heap_open(defid, AccessShareLock);
+tupdesc = CreateTupleDescCopy(RelationGetDescr(defrel));
+
+/* Build expression execution states for partition check quals */
+partqualstate = ExecPrepareCheck(partConstraint,
+estate);
+
+econtext = GetPerTupleExprContext(estate);
+snapshot = RegisterSnapshot(GetLatestSnapshot());

Definitely not safe against concurrency, since AccessShareLock won't
exclude somebody else's update.  In fact, it won't even cover somebody
else's already-in-flight transaction.

+errmsg("new default partition constraint is violated
by some row")));

Normally in such cases we try to give more detail using
ExecBuildSlotValueDescription.

+boolis_def = true;

This variable starts out true and is never set to any value other than
true.  Just get rid of it and, in the one place where it is currently
used, write "true".  That's shorter and clearer.

+inhoids = find_inheritance_children(RelationGetRelid(parent), NoLock);

If it's actually safe to do this with no lock, there ought to be a
comment with a very compelling explanation of why it's safe.

+boundspec = (Node *) stringToNode(TextDatumGetCString(datum));
+bspec = (PartitionBoundSpec *)boundspec;

There's not really a reason to cast the result of stringToNode() to
Node * and then turn around and cast it to PartitionBoundSpec *.  Just
cast it directly to whatever it needs to be.  And use the new castNode
macro.

+foreach(cell1, bspec->listdatums)
+{
+Node *value = lfirst(cell1);
+if (IsA(value, DefElem))
+{
+def_elem = true;
+*defid = inhrelid;
+}
+}
+if (def_elem)
+{
+ReleaseSysCache(tuple);
+continue;
+}
+foreach(cell3, bspec->listdatums)
+{
+Node *value = lfirst(cell3);
+boundspecs = lappend(boundspecs, value);
+}
+ReleaseSysCache(tuple);
+}
+foreach(cell4, spec->listdatums)
+{
+Node *value = lfirst(cell4);
+boundspecs = lappend(boundspecs, value);
+}

cell1, cell2, cell3, and cell4 are not very clear variable names.
Between that and the lack of comments, this is not easy to understand.
It's sort of spaghetti logic, too.  The if (def_elem) test continues
early, but if the point is that the loop using 

Re: [HACKERS] Cutting initdb's runtime (Perl question embedded)

[Andreas Karlsson]

On 04/12/2017 05:00 PM, Andreas Karlsson wrote:

Looked at this an option 1 seems simple enough if I am not missing
something. I might hack something up later tonight. Either way I think
this improvement can be done separately from the proposed replacement of
the catalog header files. Trying to fix everything at once often leads
to nothing being fixed at all.


Here is my proof of concept patch. It does basically the same thing as 
Andres's patch except that it handles quoted values a bit better and 
does not try to support anything other than the regproc type.


The patch speeds up initdb without fsync from 0.80 seconds to 0.55 
seconds, which is a nice speedup, while adding a negligible amount of 
extra work on compilation.


Two things which could be improved in this patch if people deem it 
important:


- Refactor the code to be more generic, like Andres patch, so it is 
easier to add lookups for other data types.


- Write something closer to a real .bki parser to actually understand 
quoted values and _null_ when parsing the data. Right now this patch 
only splits each row into its fields (while being aware of quotes) but 
does not attempt to remove quotes. The PoC patch treats "foo" and foo as 
different.


Andreas
commit bc8ede661b12407c0b6a9e4c0932d21028f87fc1
Author: Andreas Karlsson 
Date:   Wed Apr 12 21:00:49 2017 +0200

WIP: Resolve regproc entires when generating .bki

diff --git a/src/backend/catalog/genbki.pl b/src/backend/catalog/genbki.pl
index 6e9d57aa8d..f918c9ef8a 100644
--- a/src/backend/catalog/genbki.pl
+++ b/src/backend/catalog/genbki.pl
@@ -102,6 +102,7 @@ print $bki "# PostgreSQL $major_version\n";
 # vars to hold data needed for schemapg.h
 my %schemapg_entries;
 my @tables_needing_macros;
+my %procs;
 our @types;
 
 # produce output, one catalog at a time
@@ -164,20 +165,41 @@ foreach my $catname (@{ $catalogs->{names} })
 			$row->{bki_values} =~ s/\bPGUID\b/$BOOTSTRAP_SUPERUSERID/g;
 			$row->{bki_values} =~ s/\bPGNSP\b/$PG_CATALOG_NAMESPACE/g;
 
+			# Split values into tokens without interpreting their meaning.
+			my %bki_values;
+			@bki_values{@attnames} = $row->{bki_values} =~ /"[^"]*"|\S+/g;
+
+			# Substitute regproc entires with oids
+			foreach my $att (keys %bki_values)
+			{
+next if $bki_attr{$att}->{type} ne 'regproc';
+next if $bki_values{$att} =~ /\A(\d+|-|_null_)\z/;
+
+$bki_values{$att} = $procs{$bki_values{$att}};
+			}
+
+			# Save pg_proc oids for use by later catalogs. This relies on the
+			# order we process the files, but the bootstrap code also relies on
+			# pg_proc being loaded first.
+			if ($catname eq 'pg_proc')
+			{
+$procs{$bki_values{proname}} = $row->{oid};
+			}
+
 			# Save pg_type info for pg_attribute processing below
 			if ($catname eq 'pg_type')
 			{
-my %type;
+my %type = %bki_values;
 $type{oid} = $row->{oid};
-@type{@attnames} = split /\s+/, $row->{bki_values};
 push @types, \%type;
 			}
 
 			# Write to postgres.bki
 			my $oid = $row->{oid} ? "OID = $row->{oid} " : '';
-			printf $bki "insert %s( %s)\n", $oid, $row->{bki_values};
+			printf $bki "insert %s( %s)\n", $oid,
+			  join(' ', @bki_values{@attnames});
 
-		   # Write comments to postgres.description and postgres.shdescription
+			# Write comments to postgres.description and postgres.shdescription
 			if (defined $row->{descr})
 			{
 printf $descr "%s\t%s\t0\t%s\n", $row->{oid}, $catname,
diff --git a/src/backend/utils/Gen_fmgrtab.pl b/src/backend/utils/Gen_fmgrtab.pl
index 2af9b355e7..10d9c6abc7 100644
--- a/src/backend/utils/Gen_fmgrtab.pl
+++ b/src/backend/utils/Gen_fmgrtab.pl
@@ -58,16 +58,9 @@ foreach my $column (@{ $catalogs->{pg_proc}->{columns} })
 my $data = $catalogs->{pg_proc}->{data};
 foreach my $row (@$data)
 {
-
-	# To construct fmgroids.h and fmgrtab.c, we need to inspect some
-	# of the individual data fields.  Just splitting on whitespace
-	# won't work, because some quoted fields might contain internal
-	# whitespace.  We handle this by folding them all to a simple
-	# "xxx". Fortunately, this script doesn't need to look at any
-	# fields that might need quoting, so this simple hack is
-	# sufficient.
-	$row->{bki_values} =~ s/"[^"]*"/"xxx"/g;
-	@{$row}{@attnames} = split /\s+/, $row->{bki_values};
+	# Split values into tokens without interpreting their meaning.
+	# This is safe becease we are going to use the values as-is.
+	@{$row}{@attnames} = $row->{bki_values} =~ /"[^"]*"|\S+/g;
 
 	# Select out just the rows for internal-language procedures.
 	# Note assumption here that INTERNALlanguageId is 12.
diff --git a/src/backend/utils/adt/regproc.c b/src/backend/utils/adt/regproc.c
index 702924a958..a4237b0661 100644
--- a/src/backend/utils/adt/regproc.c
+++ b/src/backend/utils/adt/regproc.c
@@ -87,51 +87,11 @@ regprocin(PG_FUNCTION_ARGS)
 	/* Else it's a name, possibly schema-qualified */
 
 	/*
-	 * In bootstrap mode we assume the given name is not schema-qualified, and
-	 * just 

Re: [HACKERS] Query fails when SRFs are part of FROM clause (Commit id: 69f4b9c85f)

[Andres Freund]

On 2017-04-11 17:42:42 -0400, Tom Lane wrote:
> Now, that old behavior matches what you got in the RangeFunction case:
> 
> regression96=# select * from int4_tbl, cast(case when f1>0 then 
> generate_series(1,2) else null end as int);
>  f1  | int4 
> -+--
>0 | 
>   123456 |1
>   123456 |2
>  -123456 | 
>   2147483647 |1
>   2147483647 |2
>  -2147483647 | 
> (7 rows)
> 
> So it would make sense to me for our new behavior to still match the
> targetlist case.
> 
> Not sure if that's exactly the same as what you're saying or not.

The patch now indeed returns

regression[20994][1]=# select * from int4_tbl, cast(case when f1>0 then 
generate_series(1,2) else null end as int);
WARNING:  01000: replacing
LOCATION:  frobble_rtefunc, createplan.c:3102
(as you can see, this ain't quite ready)
┌─┬┐
│ f1  │  int4  │
├─┼┤
│   0 │ (null) │
│   0 │ (null) │
│  123456 │  1 │
│  123456 │  2 │
│ -123456 │ (null) │
│ -123456 │ (null) │
│  2147483647 │  1 │
│  2147483647 │  2 │
│ -2147483647 │ (null) │
│ -2147483647 │ (null) │
└─┴┘
(10 rows)

The basic approach seems quite workable.  It's not super extensible to
allow SRFs deeper inside generic ROWS FROM arguments however - I'm not
sure there's any need to work towards that however, I've not heard
demands so far.   Any arguments against that?

One other thing where it'd currently affect behaviour is something like:
SELECT * FROM CAST(generate_series(1,0) * 5 as int);

which, in < v10 would return 1 row, but with my patch returns no rows.
That makes a lot more sense in my opinion, given that a plain FROM
generate_series(1,0) doesn't return any rows in either version.

Right now I'm mopping up corner cases where it'd *expand* the set of
currently valid commands in an inconsistent manner. Namely FROM
int4mul(generate_series(..), 5) works, but FROM
composite_returning(somesrf()) wouldn't without additional work.  I plan
to continue to error out in either...

Greetings,

Andres Freund


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

Re: [HACKERS] Adding support for Default partition in partitioning

[Robert Haas]

On Thu, Apr 6, 2017 at 1:17 AM, Rushabh Lathia  wrote:
> I like the idea about having DEFAULT partition for the range partition. With
> the
> way partition is designed it can have holes into range partition. I think
> DEFAULT
> for the range partition is a good idea, generally when the range having
> holes. When
> range is serial then of course DEFAULT partition doen't much sense.

Yes, I like that idea, too.  I think the DEFAULT partition should be
allowed to be created for either range or list partitioning regardless
of whether we think there are any holes, but if you create a DEFAULT
partition when there are no holes, you just won't be able to put any
data into it.  It's silly, but it's not worth the code that it would
take to try to prevent it.

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


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

[HACKERS] Inadequate parallel-safety check for SubPlans

[Tom Lane]

While poking at the question of parallel_safe marking for Plans,
I noticed that max_parallel_hazard_walker() does this:

/* We can push the subplans only if they are parallel-safe. */
else if (IsA(node, SubPlan))
return !((SubPlan *) node)->parallel_safe;

This is 100% wrong.  It's failing to recurse into the subexpressions of
the SubPlan, which could very easily include parallel-unsafe function
calls.  Example:

regression=# explain select * from tenk1 where int4(unique1 + random()) not in 
(select ten from tenk2);
 QUERY PLAN  
-
 Gather  (cost=470.00..884.25 rows=5000 width=244)
   Workers Planned: 4
   ->  Parallel Seq Scan on tenk1  (cost=470.00..884.25 rows=1250 width=244)
 Filter: (NOT (hashed SubPlan 1))
 SubPlan 1
   ->  Seq Scan on tenk2  (cost=0.00..445.00 rows=1 width=4)

random() is parallel-restricted so it's not cool that the SubPlan was
allowed to be passed down to workers.

I tried to fix it like this:

else if (IsA(node, SubPlan))
{
if (!((SubPlan *) node)->parallel_safe &&
max_parallel_hazard_test(PROPARALLEL_RESTRICTED, context))
return true;
}

but got some failures in the regression tests due to things not getting
parallelized when expected.  Poking into that, I remembered that for some
SubPlans, the testexpr contains Params representing the output columns
of the sub-select.  So the recursive call of max_parallel_hazard_walker
visits those and deems the expression parallel-restricted.

Basically, therefore, this logic is totally inadequate.  I think what
we need to do is improve matters so that while looking at the testexpr
of a SubPlan, we pass down a whitelist saying that the Params having
the numbers indicated by the SubLink's paramIds list are OK.

I'm also suspicious that the existing dumb treatment of Params here
may be blocking recognition that correlated subplans are parallel-safe.
But that would only be a failure to apply a possible optimization,
not a failure to generate a valid plan.

regards, tom lane


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

Re: [HACKERS] Adding support for Default partition in partitioning

[Robert Haas]

On Tue, Apr 11, 2017 at 9:41 AM, Jeevan Ladhe
 wrote:
> I have checked for NULLs too, and the default partition can be created even
> when there are partitions for each TRUE, FALSE and NULL.
>
> Consider the example below:
>
> postgres=# CREATE TABLE list_partitioned (
> a bool
> ) PARTITION BY LIST (a);
> CREATE TABLE
> postgres=# CREATE TABLE part_1 PARTITION OF list_partitioned FOR VALUES IN
> ('false');
> CREATE TABLE
> postgres=# CREATE TABLE part_2 PARTITION OF list_partitioned FOR VALUES IN
> ('true');
> CREATE TABLE
> postgres=# CREATE TABLE part_3 PARTITION OF list_partitioned FOR VALUES IN
> (null);
> CREATE TABLE
> postgres=# CREATE TABLE part_default PARTITION OF list_partitioned FOR
> VALUES IN (DEFAULT);
> CREATE TABLE

In my opinion, that's absolutely fine, and it would be very strange to
try to prevent it.  The partitioning method shouldn't have specific
knowledge of the properties of individual data types.

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


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

Re: [HACKERS] Undefined psql variables

[Pavel Stehule]

2017-04-12 17:05 GMT+02:00 Robert Haas :

> On Sun, Apr 2, 2017 at 3:56 PM, Tom Lane  wrote:
> > So my view of this is that "send the expression to the server" ought
> > to be just one option for \if, not the only way to do it.
>
> I heartily agree.  There should be some kind of client-side expression
> language, and one thing it should allow is calling out to the server.
> Then people who only want to call out to the server can do that, but
> people who want to do something else have the option.  Insisting that
> this facility isn't allowed to do anything other than consult the
> server is (1) inconsistent with what we've already got in v10 and (2)
> boxing ourselves into a corner for no very good reason.
>
> Now, the optimal shape for that client-side expression language is not
> very clear to me.  Do we want to invent our own language, or maybe
> consider using something that already exists?  It's been previously
> suggested that we should somehow embed Lua, and this might not be a
> bad place to consider doing something like that.  That might be a way
> to add a lot of power without having to invent an entirely new
> programming language one bit at a time.  If we want to invent our own
> expression language, what kind of syntax should it use?  Upon what
> kind of design principles should it be based?  There's likely to be
> vigorous debate on these topics, and probably also complaints that the
> good designs are too much work and the easy-to-implement designs are
> too limiting.  (Regular readers of this mailing list will likely be
> able to guess which side of those debates I'll be on, but we need to
> have them all the same.)
>
>
Integration Lua engine can help lot of - and it can change the design
significantly. For this purpose it is maybe overkill, but it can be fresh
air in psql customisation and usage.

\setlua varname one line lua expression

or

\lua
  ...
  lua code

  psqlvar.set("", somevalue)

\endlua

I like this idea. We can use Math libraries, random generators, ...

If Lua engine and dependency are too strong cafe - very basic calculator
like
https://www.l2f.inesc-id.pt/~david/w/pt/The_YACC_Parser_Generator/Example:_Calculator_with_Variables
can
be good enough (Don't need a variables there)


> Regarding the ostensible topic of this thread, one thought I had while
> reading through these various responses is that the original need
> would be well-served by the (somewhat dubious) syntax that bash uses
> for variable substitution.  Obviously, we aren't going to change the
> interpolate-this-variable character from : to $, but bash has
> ${parameter:-word} to substitute a default for an unset parameter,
> ${parameter:=word} to substitute a default for an unset parameter and
> also set the parameter to that value, ${parameter:?word} to error out
> with word as the error mesage if parameter is not set, and so forth.
> If we decide to roll our own, we might consider taking inspiration
> from those constructs.
>
>
It is great and it can work

\set varname :{varname?some error message} ..
\set varname :{varname:-0} ..

Good ideas

Regards

Pavel




> I think that one of the general problems of language design is, as
> Larry Wall once said, that a good language should make simple things
> simple and complex things possible.  But simple is not an absolute; it
> depends on context.  The things which a language needs to make simple
> are those things which will be done frequently *in that language*.  So
> for example in this case, out-calls to SQL need to be very easy to
> write.  Maybe the empty-parameter thing needs to be easy; not sure.
> Coming up with a good solution here will involve understanding what
> people typically want to do with a language of this type and then
> making sure that stuff can be done succinctly - and ideally also
> making sure that other stuff is also possible if you're willing to put
> in more legwork.
>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>

Re: [HACKERS] Some thoughts about SCRAM implementation

[Robert Haas]

On Wed, Apr 12, 2017 at 2:09 PM, Heikki Linnakangas  wrote:
> That is very much appreciated! You writing a second implementation of the
> client-side support (libpq being the first) is very, very helpful, to
> validate that the protocol is sane, unambiguous, and adequately documented.

+1.

> Yes, we need to nail down the protocol and \password before beta. I am
> working on them now.

Good to hear.

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


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

Re: [HACKERS] pg_dump emits ALTER TABLE ONLY partitioned_table

[Robert Haas]

On Wed, Apr 12, 2017 at 6:29 AM, Amit Langote
 wrote:
> Actually, p1 is a partitioned table, so the error.  And I realize that
> that's a wrong behavior.  Currently the check is performed using only the
> relkind, which is bogus.  Specifying ONLY should cause an error only when
> the table has partitions.

That sounds like a REALLY bad idea, because now you're going to end up
with a constraint that can never be enforced against any actual data
rows ... or else you're going to later pretend that ONLY wasn't
specified.  I think the rule that partitioned tables can't have
non-inherited constraints is absolutely right, and relaxing it is
quite wrong.

I think you had the right idea upthread when you suggested dumping it this way:

CREATE TABLE p1 PARTITION OF p (
b NOT NULL
)
FOR VALUES IN (1)
PARTITION BY RANGE (b);

That looks absolutely right to me, and very much principled.

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


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

Re: [HACKERS] Some thoughts about SCRAM implementation

[Heikki Linnakangas]

On 04/12/2017 08:38 PM, Álvaro Hernández Tortosa wrote:

- Even though I don't really care about SCRAM, and without having any
prior knowledge about SCRAM, I volunteered some time ago to study SCRAM,
give a lightning talk about SCRAM and later write a client
implementation for the jdbc driver. And I have already devoted a very
fair amount of time in doing so, and will keep doing that until all code
is done. Code WIP is here FYI: https://github.com/ahachete/scram. So
it's not that I haven't already put my code behind my words.


That is very much appreciated! You writing a second implementation of 
the client-side support (libpq being the first) is very, very helpful, 
to validate that the protocol is sane, unambiguous, and adequately 
documented.



On 12/04/17 18:38, Robert Haas wrote:

Furthermore, I think that the state of this feature as it currently
exists in the tree is actually kind of concerning.  There are
currently four open items pertaining to SCRAM at least two of which
look to my mind an awful lot like stuff that should have ideally been
handled pre-feature-freeze: \password support, and protocol
negotiation.  I'm grateful for the hard work that has gone into this
feature, but these are pretty significant loose ends.  \password
support is a basic usability issue.  Protocol negotiation affects
anyone who may want to make their PG driver work with this feature,
and certainly can't be changed after final release, and ideally not
even after beta.  We really, really need to get that stuff nailed down
ASAP or we're going to have big problems.  So I think we should focus
on those things, not this.


Yes, we need to nail down the protocol and \password before beta. I am 
working on them now.


- Heikki



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

Re: [HACKERS] GSOC'17 project introduction: Parallel COPY execution with errors handling

[Stas Kelvich]

> On 12 Apr 2017, at 20:23, Robert Haas  wrote:
> 
> On Wed, Apr 12, 2017 at 1:18 PM, Nicolas Barbier
>  wrote:
>> 2017-04-11 Robert Haas :
>>> If the data quality is poor (say, 50% of lines have errors) it's
>>> almost impossible to avoid runaway XID consumption.
>> 
>> Yup, that seems difficult to work around with anything similar to the
>> proposed. So the docs might need to suggest not to insert a 300 GB
>> file with 50% erroneous lines :-).
> 
> Yep.  But it does seem reasonably likely that someone might shoot
> themselves in the foot anyway.  Maybe we just live with that.
> 

Moreover if that file consists of one-byte lines (plus one byte of newline char)
then during its import xid wraparound will happens 18 times =)

I think it’s reasonable at least to have something like max_errors parameter
to COPY, that will be set by default to 1000 for example. If user will hit that
limit then it is a good moment to put a warning about possible xid consumption
in case of bigger limit.

However I think it worth of quick research whether it is possible to create 
special
code path for COPY in which errors don’t cancel transaction. At least when
COPY called outside of transaction block.


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




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

Re: [HACKERS] Some thoughts about SCRAM implementation

[Álvaro Hernández Tortosa]

On 12/04/17 18:38, Robert Haas wrote:

On Tue, Apr 11, 2017 at 9:20 AM, Álvaro Hernández Tortosa
 wrote:

 LOL. Do you really want a half-baked Java programmer writing a patch in
C for PostgreSQL? I once tried that and Magnus said my code was Java code
that happened to compile with a C compiler ^_^

 Having said that, I take the bait, I like challenges and putting my
words behind my code :)

Excellent, because that's how stuff gets done around here.  Saying
that you want something and hoping other people will do it is fine,
but being will to put some effort into it is a lot more likely to get
it done.  Not to be harsh, but showing up 3 days after feature freeze
to complain that a feature pending commit for 14 months is missing
something that you really need isn't really the right way to make
something happen.  I'm pretty sure that the lack of channel binding
was discussed quite a bit earlier than now, so I think there was
adequate opportunity to protest, contribute a patch, etc.  It's not
that I don't have sympathy for the way you feel about this: seeing
features you care about fall out of a release sucks, and I've
experienced a lot of that suckage quite recently, so the pain is fresh
in my mind.  But it's something we all have to live with for the
overall good of the product.  We used to not have a firm feature
freeze, and that was way worse.


Hi Robert. Not harsh, no worries, but please note a couple of 
things, let me give you a bit of context about my recent comments:


- I haven't complained about not having channel binding support. I was 
just giving my own recommendations for the PostgreSQL community, in the 
belief that contributing this opinion could let -hackes to make a more 
informed decision about whether to include or not a given feature.


- I haven't said either that I need it. I don't use SCRAM, much less 
with channel binding. Rather than looking after myself here, I'm trying 
to sit on the foot of potential users and speak up for them. I might be 
wrong, of course, but this is the only role I'm trying to play here.


- I know how PostgreSQL release cycles work and not meaning to hack them 
anyway. I just thought raising the fact that something that I believe 
might be requested by enterprise users was on the other hand a minor 
addition to a feature, and thus a really high value-to-effort ratio. 
Indeed, I bet adding channel binding is an order of magnitude easier 
than adding saslprep (which is optional too, btw). Ultimately I'm happy 
with SCRAM in PG 10, whether it has cbinding or not, as it is a great 
addition and makes PG10 an even better software.


- Even though I don't really care about SCRAM, and without having any 
prior knowledge about SCRAM, I volunteered some time ago to study SCRAM, 
give a lightning talk about SCRAM and later write a client 
implementation for the jdbc driver. And I have already devoted a very 
fair amount of time in doing so, and will keep doing that until all code 
is done. Code WIP is here FYI: https://github.com/ahachete/scram. So 
it's not that I haven't already put my code behind my words.


- Given all that, I still want to volunteer to not only do the client 
jdbc part and consequently help debugging the server implementation (as 
I believe it is so far the only non-libpq implementation), but also try 
to also stand by my words by writing channel binding code for PostgreSQL 
server. This is a huge effort for me, I only did C programming on the 
year 2001. But still, I want to help from my limited capabilities as 
much as I can.


- Having thoroughly studied the RFC and companion documentation, I 
thought I was in a knowledge position as to give some recommendations 
that other hackers may not know about (unless a deep study of SCRAM 
would have been done). That's it, recommendation, ideas.





In this case, I think it is abundantly clear that SCRAM without
channel binding is still a good feature.


I agree. I may have exaggerated before, downplaying SCRAM without 
channel binding. I think it is great. Period. I also still think channel 
binding is very small code addition yet provides even better value. I 
apologize for not picking my previous words more carefully.



  One piece of evidence for
that is that the standard uses the suffix -PLUS to denote the
availability of channel binding.  That clearly conveys that channel
binding has value, but also that not having it does not make the whole
thing useless.  Otherwise, they would have required it to be part of
every implementation, or they would have made you add -CRAPPY if you
didn't have it.  The discussion elsewhere on this thread has
adequately underlined the value of what we've already got, so I won't
further belabor the point here.

Furthermore, I think that the state of this feature as it currently
exists in the tree is actually kind of concerning.  There are
currently four open items pertaining to SCRAM at least two of which
look to my mind an 

Re: [HACKERS] Letting the client choose the protocol to use during a SASL exchange

[Heikki Linnakangas]

On 04/11/2017 02:32 PM, Álvaro Hernández Tortosa wrote:

 So I still see your proposal more awkward and less clear, mixing
things that are separate. But again, your choice  :)


So, here's my more full-fledged proposal.

The first patch refactors libpq code, by moving the responsibility of 
reading the GSS/SSPI/SASL/MD5 specific data from the authentication 
request packet, from the enormous switch-case construct in 
PQConnectPoll(), into pg_fe_sendauth(). This isn't strictly necessary, 
but I think it's useful cleanup anyway, and now that there's a bit more 
structure in the AuthenticationSASL message, the old way was getting 
awkward.


The second patch contains the protocol changes, and adds the 
documentation for it.


- Heikki

>From 29c958dab9f8ece5d855b335c09cc9125606774a Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas 
Date: Wed, 12 Apr 2017 16:01:18 +0300
Subject: [PATCH 1/2] Refactor libpq authentication request processing.

Move the responsibility of reading the data from the authentication request
message from PQconnectPoll() to pg_fe_sendauth(). This way, PQconnectPoll()
doesn't need to know about the different authentication request types,
and we don't need the extra fields in the pg_conn struct to pass the data
from PQconnectPoll() to pg_fe_sendauth() anymore.

In the backend, free each SASL message after sending it. It's not a lot
of wasted memory, but a leak nevertheless. Adding the pfree() revealed
a little bug in build_server_first_message(): It attempts to keeps a copy
of the sent message, but it was missing a pstrdup(), so the pointer
started to dangle, after adding the pfree() into CheckSCRAMAuth().
---
 src/backend/libpq/auth-scram.c|  10 +-
 src/backend/libpq/auth.c  |  10 +-
 src/interfaces/libpq/fe-auth.c| 214 +-
 src/interfaces/libpq/fe-auth.h|   2 +-
 src/interfaces/libpq/fe-connect.c | 113 +++-
 src/interfaces/libpq/fe-misc.c|  13 +++
 src/interfaces/libpq/libpq-int.h  |  11 +-
 7 files changed, 204 insertions(+), 169 deletions(-)

diff --git a/src/backend/libpq/auth-scram.c b/src/backend/libpq/auth-scram.c
index 5077ff33b1..a47c48d980 100644
--- a/src/backend/libpq/auth-scram.c
+++ b/src/backend/libpq/auth-scram.c
@@ -161,10 +161,10 @@ static char *scram_MockSalt(const char *username);
  * needs to be called before doing any exchange.  It will be filled later
  * after the beginning of the exchange with verifier data.
  *
- * 'username' is the provided by the client.  'shadow_pass' is the role's
- * password verifier, from pg_authid.rolpassword.  If 'shadow_pass' is NULL, we
- * still perform an authentication exchange, but it will fail, as if an
- * incorrect password was given.
+ * 'username' is the username provided by the client in the startup message.
+ * 'shadow_pass' is the role's password verifier, from pg_authid.rolpassword.
+ * If 'shadow_pass' is NULL, we still perform an authentication exchange, but
+ * it will fail, as if an incorrect password was given.
  */
 void *
 pg_be_scram_init(const char *username, const char *shadow_pass)
@@ -984,7 +984,7 @@ build_server_first_message(scram_state *state)
  state->client_nonce, state->server_nonce,
  state->salt, state->iterations);
 
-	return state->server_first_message;
+	return pstrdup(state->server_first_message);
 }
 
 
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index 66ead9381d..681b93855f 100644
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -872,6 +872,8 @@ CheckSCRAMAuth(Port *port, char *shadow_pass, char **logdetail)
 	strlen(SCRAM_SHA256_NAME) + 1);
 
 	/*
+	 * Initialize the status tracker for message exchanges.
+	 *
 	 * If the user doesn't exist, or doesn't have a valid password, or it's
 	 * expired, we still go through the motions of SASL authentication, but
 	 * tell the authentication method that the authentication is "doomed".
@@ -880,8 +882,6 @@ CheckSCRAMAuth(Port *port, char *shadow_pass, char **logdetail)
 	 * This is because we don't want to reveal to an attacker what usernames
 	 * are valid, nor which users have a valid password.
 	 */
-
-	/* Initialize the status tracker for message exchanges */
 	scram_opaq = pg_be_scram_init(port->user_name, shadow_pass);
 
 	/*
@@ -918,7 +918,7 @@ CheckSCRAMAuth(Port *port, char *shadow_pass, char **logdetail)
 			return STATUS_ERROR;
 		}
 
-		elog(DEBUG4, "Processing received SASL token of length %d", buf.len);
+		elog(DEBUG4, "Processing received SASL response of length %d", buf.len);
 
 		/*
 		 * we pass 'logdetail' as NULL when doing a mock authentication,
@@ -936,9 +936,11 @@ CheckSCRAMAuth(Port *port, char *shadow_pass, char **logdetail)
 			/*
 			 * Negotiation generated data to be sent to the client.
 			 */
-			elog(DEBUG4, "sending SASL response token of length %u", outputlen);
+			elog(DEBUG4, "sending SASL challenge of length %u", outputlen);
 
 			sendAuthRequest(port, 

Re: [HACKERS] Cutting initdb's runtime (Perl question embedded)

[Andres Freund]

On 2017-04-12 10:12:47 -0400, Tom Lane wrote:
> Andres mentioned, and I've confirmed locally, that a large chunk of
> initdb's runtime goes into regprocin's brute-force lookups of function
> OIDs from function names.  The recent discussion about cutting TAP test
> time prompted me to look into that question again.  We had had some
> grand plans for getting genbki.pl to perform the name-to-OID conversion
> as part of a big rewrite, but since that project is showing few signs
> of life, I'm thinking that a more localized performance fix would be
> a good thing to look into.  There seem to be a couple of plausible
> routes to a fix:
> 
> 1. The best thing would still be to make genbki.pl do the conversion,
> and write numeric OIDs into postgres.bki.  The core stumbling block
> here seems to be that for most catalogs, Catalog.pm and genbki.pl
> never really break down a DATA line into fields --- and we certainly
> have got to do that, if we're going to replace the values of regproc
> fields.  The places that do need to do that approximate it like this:
> 
>   # To construct fmgroids.h and fmgrtab.c, we need to inspect some
>   # of the individual data fields.  Just splitting on whitespace
>   # won't work, because some quoted fields might contain internal
>   # whitespace.  We handle this by folding them all to a simple
>   # "xxx". Fortunately, this script doesn't need to look at any
>   # fields that might need quoting, so this simple hack is
>   # sufficient.
>   $row->{bki_values} =~ s/"[^"]*"/"xxx"/g;
>   @{$row}{@attnames} = split /\s+/, $row->{bki_values};
> 
> We would need a bullet-proof, non-hack, preferably not too slow way to
> split DATA lines into fields properly.  I'm one of the world's worst
> Perl programmers, but surely there's a way?

I've done something like 1) before:
http://archives.postgresql.org/message-id/20150221230839.GE2037%40awork2.anarazel.de

I don't think the speeds matters all that much, because we'll only do it
when generating the .bki file - a couple ms more or less won't matter
much.

I IIRC spent some more time to also load the data files from a different
format:
https://git.postgresql.org/gitweb/?p=users/andresfreund/postgres.git;a=shortlog;h=refs/heads/sane-catalog
although that's presumably heavily outdated now.

- Andres


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

Re: [HACKERS] index-only count(*) for indexes supporting bitmap scans

[Alexander Kuzmenkov]

On 12.04.2017 12:29, Alexander Korotkov wrote:

That's a cool feature for FTS users! Please, register this patch to 
the next commitfest.
I've added this to the 2017-07 commitfest: 
https://commitfest.postgresql.org/14/1117/



Also, what is planning overhead of this patch? That's worth
testing too.

The planning overhead is about 0.1 - 0.07 ms on the samples from my 
first letter.


Another thing catch my eye.  The estimated number of rows in Bitmap 
Count node is the same as in Bitmap Index Scan node.  Should it be 1 
because it always returns single row?

You're right, I'll fix this in the next version of the patch.


Does this limitation cause a performance drawback?  When bitmap
index scan returns all rechecks, alternative to Bitmap Count is
still Aggregate + Bitmap Heap Scan.  Thus, I think Bitmap Count
would have the same performance or even slightly faster.  That's
worth testing.

Bitmap heap scan can indeed be faster, because it prefetches heap pages, 
and can be run in parallel. When the table data is not cached, the 
difference is not big on my machine. It could probably be significant if 
I used a storage that supported parallel reads. When the data is cached 
in memory, the parallel bitmap heap scan can be significantly faster.
We could use the standard bitmap heap scan node with some tweaks, as 
discussed in the other subthread, to avoid this regression.


Here are some test queries:

--- not cached 
---
test1=# explain analyze select count(*) from pglist where fts @@ 
to_tsquery( 'tom & lane' );

  QUERY PLAN
--
 Bitmap Count on pglist  (cost=542.65..1087.68 rows=54503 width=8) 
(actual time=30264.174..30264.177 rows=1 loops=1)

   Recheck Cond: (fts @@ to_tsquery('tom & lane'::text))
   Rows Removed by Index Recheck: 270853
   Heap Fetches: 66138
   Heap Blocks: exact=39854 lossy=66138
   ->  Bitmap Index Scan on idx_pglist_fts  (cost=0.00..529.02 
rows=54503 width=0) (actual time=525.341..525.341 rows=222813 loops=1)

 Index Cond: (fts @@ to_tsquery('tom & lane'::text))
 Planning time: 128.238 ms
 Execution time: 30264.299 ms
(9 rows)

test1=# set enable_bitmapcount to off;
SET
test1=# explain analyze select count(*) from pglist where fts @@ 
to_tsquery( 'tom & lane' );

QUERY PLAN

 Finalize Aggregate  (cost=119989.73..119989.74 rows=1 width=8) (actual 
time=31699.829..31699.830 rows=1 loops=1)
   ->  Gather  (cost=119989.52..119989.73 rows=2 width=8) (actual 
time=31698.699..31699.819 rows=3 loops=1)

 Workers Planned: 2
 Workers Launched: 2
 ->  Partial Aggregate  (cost=118989.52..118989.53 rows=1 
width=8) (actual time=31689.289..31689.290 rows=1 loops=3)
   ->  Parallel Bitmap Heap Scan on pglist 
(cost=542.65..118932.74 rows=22710 width=0) (actual 
time=608.532..31634.692 rows=74271 loops=3)

 Recheck Cond: (fts @@ to_tsquery('tom & lane'::text))
 Rows Removed by Index Recheck: 90284
 Heap Blocks: exact=13242 lossy=21960
 ->  Bitmap Index Scan on idx_pglist_fts 
(cost=0.00..529.02 rows=54503 width=0) (actual time=552.136..552.136 
rows=222813 loops=1)
   Index Cond: (fts @@ to_tsquery('tom & 
lane'::text))

 Planning time: 160.055 ms
 Execution time: 31724.468 ms
(13 rows)


- cached 
-
test1=# explain analyze select count(*) from pglist where fts @@ 
to_tsquery( 'tom & lane' );

QUERY PLAN
--
 Finalize Aggregate  (cost=119989.73..119989.74 rows=1 width=8) (actual 
time=1250.973..1250.973 rows=1 loops=1)
   ->  Gather  (cost=119989.52..119989.73 rows=2 width=8) (actual 
time=1250.588..1250.964 rows=3 loops=1)

 Workers Planned: 2
 Workers Launched: 2
 ->  Partial Aggregate  (cost=118989.52..118989.53 rows=1 
width=8) (actual time=1246.144..1246.144 rows=1 loops=3)
   ->  Parallel Bitmap Heap Scan on pglist 
(cost=542.65..118932.74 rows=22710 width=0) (actual 
time=82.781..1237.585 rows=74271 loops=3)

 Recheck Cond: (fts @@ to_tsquery('tom & lane'::text))
 Rows Removed by Index Recheck: 90284
 Heap Blocks: exact=13221 lossy=22217
 ->  Bitmap Index Scan on 

Re: [HACKERS] GSOC'17 project introduction: Parallel COPY execution with errors handling

[Robert Haas]

On Wed, Apr 12, 2017 at 1:18 PM, Nicolas Barbier
 wrote:
> 2017-04-11 Robert Haas :
>> There's a nasty trade-off here between XID consumption (and the
>> aggressive vacuums it eventually causes) and preserving performance in
>> the face of errors - e.g. if you make k = 100,000 you consume 100x
>> fewer XIDs than if you make k = 1000, but you also have 100x the work
>> to redo (on average) every time you hit an error.
>
> You could make it dynamic: Commit the subtransaction even when not
> encountering any error after N lines (N starts out at 1), then double
> N and continue. When encountering an error, roll back the current
> subtransaction back and re-insert all the known good rows that have
> been rolled back (plus maybe the erroneous row into a separate table
> or whatever) in one new subtransaction and commit; then reset N to 1
> and continue processing the rest of the file.
>
> That would work reasonable well whenever the ratio of erroneous rows
> is not extremely high: whether the erroneous rows are all clumped
> together, entirely randomly spread out over the file, or a combination
> of both.

Right.  I wouldn't suggest the exact algorithm you proposed; I think
you ought to vary between some lower limit >1, maybe 10, and some
upper limit, maybe 1,000,000, ratcheting up and down based on how
often you hit errors in some way that might not be as simple as
doubling.  But something along those lines.

>> If the data quality is poor (say, 50% of lines have errors) it's
>> almost impossible to avoid runaway XID consumption.
>
> Yup, that seems difficult to work around with anything similar to the
> proposed. So the docs might need to suggest not to insert a 300 GB
> file with 50% erroneous lines :-).

Yep.  But it does seem reasonably likely that someone might shoot
themselves in the foot anyway.  Maybe we just live with that.

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


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

Re: [HACKERS] snapbuild woes

[Andres Freund]

On 2017-04-12 11:03:57 -0400, Peter Eisentraut wrote:
> On 4/12/17 02:31, Noah Misch wrote:
> >>> But I hope you mean to commit these snapbuild patches before the postgres 
> >>> 10
> >>> release?  As far as I know, logical replication is still very broken 
> >>> without
> >>> them (or at least some of that set of 5 patches - I don't know which ones
> >>> are essential and which may not be).
> >>
> >> Yes, these should go into 10 *and* earlier releases, and I don't plan to
> >> wait for 2017-07.
> > 
> > [Action required within three days.  This is a generic notification.]
> 
> I'm hoping for a word from Andres on this.

Feel free to reassign to me.


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

Re: [HACKERS] GSOC'17 project introduction: Parallel COPY execution with errors handling

[Nicolas Barbier]

2017-04-11 Robert Haas :

> There's a nasty trade-off here between XID consumption (and the
> aggressive vacuums it eventually causes) and preserving performance in
> the face of errors - e.g. if you make k = 100,000 you consume 100x
> fewer XIDs than if you make k = 1000, but you also have 100x the work
> to redo (on average) every time you hit an error.

You could make it dynamic: Commit the subtransaction even when not
encountering any error after N lines (N starts out at 1), then double
N and continue. When encountering an error, roll back the current
subtransaction back and re-insert all the known good rows that have
been rolled back (plus maybe the erroneous row into a separate table
or whatever) in one new subtransaction and commit; then reset N to 1
and continue processing the rest of the file.

That would work reasonable well whenever the ratio of erroneous rows
is not extremely high: whether the erroneous rows are all clumped
together, entirely randomly spread out over the file, or a combination
of both.

> If the data quality is poor (say, 50% of lines have errors) it's
> almost impossible to avoid runaway XID consumption.

Yup, that seems difficult to work around with anything similar to the
proposed. So the docs might need to suggest not to insert a 300 GB
file with 50% erroneous lines :-).

Greetings,

Nicolas


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

Re: [HACKERS] Some thoughts about SCRAM implementation

[Álvaro Hernández Tortosa]

On 12/04/17 18:09, Tom Lane wrote:

Heikki Linnakangas  writes:

On 04/12/2017 06:26 PM, Bruce Momjian wrote:

How does it do that?

Good question, crypto magic? I don't know the details, but the basic
idea is that you extract a blob of data that uniquely identifies the TLS
connection. Using some OpenSSL functions, in this case. I think it's a
hash of some of the TLS handshake messages that were used when the TLS
connection was established (that's what "tls-unique" means). That data
is then incorporated in the hash calculations of the SCRAM
authentication. If the client and the server are not speaking over the
same TLS connection, they will use different values for the TLS data,
and the SCRAM computations will not match, and you get an authentication
failure.


I believe the above is not correct. Channel binding data is *not* 
used for any hash computations. It is simply a byte array that is 
received as an extra user parameter, and the server then gets it by its 
own way (its own TLS data) and do a byte comparison. That's what the 
RFCs say about it.

... which the user can't tell apart from having fat-fingered the password,
I suppose?  Doesn't sound terribly friendly.  A report of a certificate
mismatch is far more likely to lead people to realize there's a MITM.


So given what I said before, that won't happen. Indeed, SCRAM RFC 
contains a specific error code for this: "channel-bindings-dont-match".



Álvaro


--

Álvaro Hernández Tortosa


---
<8K>data



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

Re: [HACKERS] Patch: Write Amplification Reduction Method (WARM)

[Robert Haas]

On Tue, Apr 11, 2017 at 1:20 PM, Pavan Deolasee
 wrote:
> I don't know why you say that regressions are not addressed. Here are a few
> things I did to address the regressions/reviews/concerns, apart from fixing
> all the bugs discovered, but please let me know if there are things I've not
> addressed.

I'm making statements based on my perception of the discussion on the
thread.  Perhaps you did some work which you either didn't mention or
I missed you mentioning it, but it sure didn't feel like all of the
things reported got addressed.

> 1. Improved the interesting attrs patch that Alvaro wrote to address the
> regression discovered in fetching more heap attributes. The patch that got
> committed in fact improved certain synthetic workloads over then master.

Yep, though it was not clear that all of the regressing cases were
actually addressed, at least not to me.

> 2. Based on Petr and your feedback, disabled WARM on toasted attributes to
> reduce overhead of fetching/decompressing the attributes.

But that's not necessarily the right fix, as per
http://postgr.es/m/ca+tgmoyufxy1lsedzsw8uuulujhh0r8ncd-up-hzmc1fydp...@mail.gmail.com
and subsequent discussion.  It's not clear to me from that discussion
that we've got to a place where the method used to identify whether a
WARM update happened during a scan is exactly identical to the method
used to decide whether to perform one in the first place.

> 3. Added code to avoid doing second index scan when the index does not
> contain any WARM pointers. This should address the situation Amit brought up
> where only one of the indexes receive WARM inserts
> 4. Added code to kill wrong index pointers to do online cleanup.

Good changes.

> 5. Added code to set a CLEAR pointer to a WARM pointer when we know that the
> entire chain is WARM. This should address the workload Dilip ran and found
> regression (I don't think he got chance to confirm that)

Which is clearly a thing that should happen before commit, and really,
you ought to be leading the effort to confirm that, not him.  It's
good for him to verify that your fix worked, but you should test it
first.

> 6. Enhanced stats collector to collect information about candidate WARM
> chains and added mechanism to control WARM cleanup at the heap as well as
> index level, based on configurable parameters. This gives user better
> control over the additional work that is required for WARM cleanup.

I haven't seen previous discussion of this; therefore I doubt whether
we have agreement on these parameters.

> 7. Added table level option to disable WARM if nothing else works.

-1 from me.

> 8. Added mechanism to disable WARM when more than 50% indexes are being
> updated. I ran some benchmarks with different percentage of indexes getting
> updated and thought this is a good threshold.

+1 from me.

> I may have missed something, but there is no intention to ignore known
> regressions/reviews. Of course, I don't think that every regression will be
> solvable, like if you run a CPU-bound workload, setting it up in a way such
> that you repeatedly exercise the area where WARM is doing additional work,
> without providing any benefit, may be you can still find regression. I am
> willing to fix them as long as they are fixable and we are comfortable with
> the additional code complexity. IMHO certain trade-offs are good, but I
> understand that not everybody will agree with my views and that's ok.

The point here is that we can't make intelligent decisions about
whether to commit this feature unless we know which situations get
better and which get worse and by how much.  I don't accept as a
general principle the idea that CPU-bound workloads don't matter.
Obviously, I/O-bound workloads matter too, but we can't throw
CPU-bound workloads under the bus.  Now, avoiding index bloat does
also save CPU, so it is easy to imagine that WARM could come out ahead
even if each update consumes slightly more CPU when actually updating,
so we might not actually regress.  If we do, I guess I'd want to know
why.

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


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

Re: [HACKERS] Some thoughts about SCRAM implementation

[Tom Lane]

Stephen Frost  writes:
> * Tom Lane (t...@sss.pgh.pa.us) wrote:
>> ... which the user can't tell apart from having fat-fingered the password,
>> I suppose?  Doesn't sound terribly friendly.  A report of a certificate
>> mismatch is far more likely to lead people to realize there's a MITM.

> We might be able to improve on that.

I'd sure be happier about this being a useful feature if we can.  But
unless someone has a design for that ready-to-go, that's another good
reason to put it off.

regards, tom lane


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

Re: [HACKERS] index-only count(*) for indexes supporting bitmap scans

[Alexander Kuzmenkov]

On 12.04.2017 17:24, Tom Lane wrote:

TBH, I'm not sure you need to do any of that work.  Have you got evidence
that the planner will fail to choose the right plan regardless? I'm
particularly unconvinced that choose_bitmap_and is a critical problem,
because once you're into having to AND multiple indexes, you're talking
about an expensive query anyhow.
The most expensive part would probably be accessing the heap in the 
bitmap heap scan. It may be worth trying to choose an index path that 
checks all the restriction and therefore allows us to skip this heap 
access. This path might not be the cheapest one, though. The standard 
AND selection procedure would have discarded it based on cost.
I've seen this happen on the regression database. Somehow I can't seem 
to reproduce it on my earlier full-text search example.


An example:

regression=# explain select count(*) from tenk1 where hundred < 90 and 
thousand < 31;

QUERY PLAN
---
 Bitmap Count on tenk1  (cost=182.69..185.56 rows=1 width=8)
   Recheck Cond: ((thousand < 31) AND (hundred < 90))
   ->  BitmapAnd  (cost=182.69..182.69 rows=287 width=0)
 ->  Bitmap Index Scan on tenk1_thous_tenthous (cost=0.00..6.68 
rows=319 width=0)

   Index Cond: (thousand < 31)
 ->  Bitmap Index Scan on tenk1_hundred (cost=0.00..175.62 
rows=8978 width=0)

   Index Cond: (hundred < 90)
(7 rows)

regression=# set enable_bitmapcount to off;
SET
regression=# explain select count(*) from tenk1 where hundred < 90 and 
thousand < 31;

QUERY PLAN
---
 Aggregate  (cost=375.34..375.35 rows=1 width=8)
   ->  Bitmap Heap Scan on tenk1  (cost=6.75..374.62 rows=287 width=0)
 Recheck Cond: (thousand < 31)
 Filter: (hundred < 90)
 ->  Bitmap Index Scan on tenk1_thous_tenthous (cost=0.00..6.68 
rows=319 width=0)

   Index Cond: (thousand < 31)
(6 rows)

--

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



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

Re: [HACKERS] [sqlsmith] ERROR: badly formatted node string "RESTRICTINFO...

[Tom Lane]

Amit Kapila  writes:
> On Wed, Apr 12, 2017 at 12:40 AM, Tom Lane  wrote:
>> Anyone want to draft a patch for this?

> Please find patch attached based on above discussion.

This patch seems fairly incomplete: you can't just whack around major data
structures like PlannedStmt and PlannerGlobal without doing the janitorial
work of cleaning up support logic such as outfuncs/readfuncs.

Also, when you think about what will happen when doing a copyObject()
on a completed plan, it seems like a pretty bad idea to link subplans
into two independent lists.  We'll end up with two separate copies of
those subtrees in places like the plan cache.

I'm inclined to think the other approach of adding a parallel_safe
flag to struct Plan is a better answer in the long run.  It's basically
free to have it there because of alignment considerations, and it's
hard to believe that we're not going to need that labeling at some
point in the future anyway.

I had been a bit concerned about having to have some magic in outfuncs
and/or readfuncs to avoid transferring the unsafe subplan(s), but I see
from your patch there's a better way: we can have ExecSerializePlan modify
the subplan list as it transfers it into its private PlannedStmt struct.
But I think iterating over the list and examining each subplan's
parallel_safe marking is a better way to do that.

Will work on this approach.

regards, tom lane


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

Re: [HACKERS] the need to finish

[Robert Haas]

On Wed, Apr 12, 2017 at 11:41 AM, Simon Riggs  wrote:
> On 12 April 2017 at 16:26, Tom Lane  wrote:
>> Erik Rijkers  writes:
>>> Logical replication emits logmessages like these:
>>> DETAIL:  90 transactions need to finish.
>>> I would prefer the line to be more terse:
>>> DETAIL:  90 transactions to finish.
>> Our style guidelines say that detail messages should be complete
>> sentences, so I don't like your proposal too much.
>> Maybe "N transactions remain to finish." ?
> waiting for N transactions to complete

+1 for something along those lines, but perhaps it needs a capital
letter and a period.

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


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

Re: [HACKERS] Some thoughts about SCRAM implementation

[Stephen Frost]

Tom, all,

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> ... which the user can't tell apart from having fat-fingered the password,
> I suppose?  Doesn't sound terribly friendly.  A report of a certificate
> mismatch is far more likely to lead people to realize there's a MITM.

We might be able to improve on that.

> So this seems more like a hack than like a feature we need so desperately
> as to push it into v10 post-freeze.

Channel binding certainly isn't a 'hack' and is something we should
support, but I agree that it doesn't need to go into v10.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Possible problem in Custom Scan API

[Dmitry Ivanov]

Tom Lane wrote:

I'm coming around to the idea that it'd be better to disable physical
tlists for custom scans.


I've been thinking about this all along, and it seems that this is a decent 
decision. However, I've made a tiny bugfix patch which allows CustomScans 
to notify the core code that they can handle physical targetlists. 
Extension authors won't have to change anything provided that CustomPath is 
allocated using palloc0().



However, I'm hesitant to make such a change in the back branches; if
there's anyone using custom scans who's negatively affected, they'd be
rightfully unhappy.  Are you satisfied if we change this in HEAD?


It's kind of bittersweet. I'm really glad that you've changed your mind and 
this is going to be fixed in HEAD, but this change is crucial for our 
extension (if we use it with vanilla postgres).


Maybe you'll like my patch more.


--
Dmitry Ivanov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Companydiff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c
index 71678d08dc..4b0322530b 100644
--- a/src/backend/optimizer/plan/createplan.c
+++ b/src/backend/optimizer/plan/createplan.c
@@ -761,6 +761,9 @@ use_physical_tlist(PlannerInfo *root, Path *path, int flags)
 	if (rel->reloptkind != RELOPT_BASEREL)
 		return false;
 
+	if (IsA(path, CustomPath) && ((CustomPath *) path)->forbid_physical_tlist)
+		return false;
+
 	/*
 	 * Can't do it if any system columns or whole-row Vars are requested.
 	 * (This could possibly be fixed but would take some fragile assumptions
diff --git a/src/include/nodes/relation.h b/src/include/nodes/relation.h
index 48862d93ae..5a1f6cee47 100644
--- a/src/include/nodes/relation.h
+++ b/src/include/nodes/relation.h
@@ -1089,6 +1089,7 @@ typedef struct CustomPath
  * nodes/extensible.h */
 	List	   *custom_paths;	/* list of child Path nodes, if any */
 	List	   *custom_private;
+	bool		forbid_physical_tlist;
 	const struct CustomPathMethods *methods;
 } CustomPath;
 

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

Re: [HACKERS] Some thoughts about SCRAM implementation

[Robert Haas]

On Tue, Apr 11, 2017 at 9:20 AM, Álvaro Hernández Tortosa
 wrote:
> LOL. Do you really want a half-baked Java programmer writing a patch in
> C for PostgreSQL? I once tried that and Magnus said my code was Java code
> that happened to compile with a C compiler ^_^
>
> Having said that, I take the bait, I like challenges and putting my
> words behind my code :)

Excellent, because that's how stuff gets done around here.  Saying
that you want something and hoping other people will do it is fine,
but being will to put some effort into it is a lot more likely to get
it done.  Not to be harsh, but showing up 3 days after feature freeze
to complain that a feature pending commit for 14 months is missing
something that you really need isn't really the right way to make
something happen.  I'm pretty sure that the lack of channel binding
was discussed quite a bit earlier than now, so I think there was
adequate opportunity to protest, contribute a patch, etc.  It's not
that I don't have sympathy for the way you feel about this: seeing
features you care about fall out of a release sucks, and I've
experienced a lot of that suckage quite recently, so the pain is fresh
in my mind.  But it's something we all have to live with for the
overall good of the product.  We used to not have a firm feature
freeze, and that was way worse.

In this case, I think it is abundantly clear that SCRAM without
channel binding is still a good feature.  One piece of evidence for
that is that the standard uses the suffix -PLUS to denote the
availability of channel binding.  That clearly conveys that channel
binding has value, but also that not having it does not make the whole
thing useless.  Otherwise, they would have required it to be part of
every implementation, or they would have made you add -CRAPPY if you
didn't have it.  The discussion elsewhere on this thread has
adequately underlined the value of what we've already got, so I won't
further belabor the point here.

Furthermore, I think that the state of this feature as it currently
exists in the tree is actually kind of concerning.  There are
currently four open items pertaining to SCRAM at least two of which
look to my mind an awful lot like stuff that should have ideally been
handled pre-feature-freeze: \password support, and protocol
negotiation.  I'm grateful for the hard work that has gone into this
feature, but these are pretty significant loose ends.  \password
support is a basic usability issue.  Protocol negotiation affects
anyone who may want to make their PG driver work with this feature,
and certainly can't be changed after final release, and ideally not
even after beta.  We really, really need to get that stuff nailed down
ASAP or we're going to have big problems.  So I think we should focus
on those things, not this.

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


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

Re: [HACKERS] Add pgstathashindex() to get hash index table statistics.

[Robert Haas]

On Wed, Apr 12, 2017 at 2:28 AM, Noah Misch  wrote:
> [Action required within three days.  This is a generic notification.]
>
> The above-described topic is currently a PostgreSQL 10 open item.  Robert,
> since you committed the patch believed to have created it, you own this open
> item.  If some other commit is more relevant or if this does not belong as a
> v10 open item, please let us know.  Otherwise, please observe the policy on
> open item ownership[1] and send a status update within three calendar days of
> this message.  Include a date for your subsequent status update.  Testers may
> discover new open items at any time, and I want to plan to get them all fixed
> well in advance of shipping v10.  Consequently, I will appreciate your efforts
> toward speedy resolution.  Thanks.

Oops.  I had forgotten about this one; committed now.

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


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

Re: [HACKERS] Some thoughts about SCRAM implementation

[Tom Lane]

Heikki Linnakangas  writes:
> On 04/12/2017 06:26 PM, Bruce Momjian wrote:
>> How does it do that?

> Good question, crypto magic? I don't know the details, but the basic 
> idea is that you extract a blob of data that uniquely identifies the TLS 
> connection. Using some OpenSSL functions, in this case. I think it's a 
> hash of some of the TLS handshake messages that were used when the TLS 
> connection was established (that's what "tls-unique" means). That data 
> is then incorporated in the hash calculations of the SCRAM 
> authentication. If the client and the server are not speaking over the 
> same TLS connection, they will use different values for the TLS data, 
> and the SCRAM computations will not match, and you get an authentication 
> failure.

... which the user can't tell apart from having fat-fingered the password,
I suppose?  Doesn't sound terribly friendly.  A report of a certificate
mismatch is far more likely to lead people to realize there's a MITM.

So this seems more like a hack than like a feature we need so desperately
as to push it into v10 post-freeze.

regards, tom lane


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

Re: [HACKERS] Possible problem in Custom Scan API

[Tom Lane]

Dmitry Ivanov  writes:
>> Uh, no, construction of a custom plan node is entirely driven by the
>> PlanCustomPath method as far as I can see.  You're free to ignore what
>> create_scan_plan did and insert your own tlist.

> Are you sure? Even if it's true, this targetlist should still contain each 
> and every Var that's been requested. If I'm correct, the only way to ensure 
> that is to call build_path_tlist(), which is static (oops!). Perhaps I 
> could make my own, but it uses replace_nestloop_params() (again, static), 
> and the problem goes further and further.

True.  We could expose build_path_tlist(), perhaps, but then you soon
reach the conclusion that PlanCustomPath should also be given access to
the tlist-related "flags" that are being passed around in createplan.c,
and that's a conclusion I don't want to reach.  That whole business is
a bit of a hack that I hope to redesign someday, but if we embed it in
the custom-scan API it will get very hard to change.

I'm coming around to the idea that it'd be better to disable physical
tlists for custom scans.  That optimization is built on the assumption
that providing all the columns is free (or at least, cheaper than doing
ExecProject), but it's easy to think of custom-scan applications where
that's not true.  It's a disastrous choice for a custom-scan provider
that's trying to interface to a column store, for instance.

However, I'm hesitant to make such a change in the back branches; if
there's anyone using custom scans who's negatively affected, they'd be
rightfully unhappy.  Are you satisfied if we change this in HEAD?

regards, tom lane


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

Re: [HACKERS] Some thoughts about SCRAM implementation

[Heikki Linnakangas]

On 04/12/2017 06:26 PM, Bruce Momjian wrote:

On Wed, Apr 12, 2017 at 12:13:03PM +0300, Heikki Linnakangas wrote:

That said, I stand by my comment that I don't think it's the enterprises
that need or want the channel binding. If they care about it, they have
already put certificate validation in place, and it won't buy them anything.

Because channel binding also only secures the authentication (SCRAM), not
the actual contents and commands that are then sent across the channel,
AFAIK?


TLS protects the contents and the commands. The point of channel binding is
to defeat a MITM attack, where the client connects to a malicious server,
using TLS, which then connects to the real server, using another TLS
connection. Channel binding will detect that the client and the real server
are not communicating over the same TLS connection, but two different TLS
connections, and make the authentication fail.

SSL certificates, with validation, achieves the same, but channel binding
achieves it without the hassle of certificates.


How does it do that?


Good question, crypto magic? I don't know the details, but the basic 
idea is that you extract a blob of data that uniquely identifies the TLS 
connection. Using some OpenSSL functions, in this case. I think it's a 
hash of some of the TLS handshake messages that were used when the TLS 
connection was established (that's what "tls-unique" means). That data 
is then incorporated in the hash calculations of the SCRAM 
authentication. If the client and the server are not speaking over the 
same TLS connection, they will use different values for the TLS data, 
and the SCRAM computations will not match, and you get an authentication 
failure.


- Heikki



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

Re: [HACKERS] error handling in RegisterBackgroundWorker

[Robert Haas]

On Tue, Apr 11, 2017 at 10:13 PM, Noah Misch  wrote:
> On Tue, Apr 11, 2017 at 11:33:34AM -0400, Tom Lane wrote:
>> Peter Eisentraut  writes:
>> > I think there is no clear agreement here, and no historically consistent
>> > behavior.  I'm prepared to let it go and cross it off the list of open
>> > items.  I believe we should keep thinking about it, but it's not
>> > something that has to hold up beta.
>>
>> Agreed, this doesn't seem like a must-fix-for-beta consideration.
>
> Definitely not a beta blocker, agreed.  Would it be okay to release v10 final
> with the logical replication launcher soft-failing this way?

I'm not really in favor of making a behavior change here, so it would
be fine with me.  Obviously people who think the behavior should be
changed are more likely to disagree with that idea.  Maybe in the long
run we should have a command-line option (not a GUC) that's like...

postgres --soldier-on-valiently

...and then when that's not supplied we can die but when it is
supplied we persist in spite of all failures that are in any way
recoverable.  However, I think that's really a new development effort,
not a cleanup item for v10.  I share Tom's concern to the effect that
single-user mode is a really awful way to try to recover from
failures, so we should avoid decisions that force people into that
recovery mode more often.

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


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

Re: [HACKERS] the need to finish

[Simon Riggs]

On 12 April 2017 at 16:26, Tom Lane  wrote:
> Erik Rijkers  writes:
>> Logical replication emits logmessages like these:
>> DETAIL:  90 transactions need to finish.
>> DETAIL:  87 transactions need to finish.
>> DETAIL:  70 transactions need to finish.
>
>> Could we get rid of that 'need'?   It strikes me as a bit off; something
>> that people would say but not a mechanical message by a computer. I
>> dislike it strongly.
>
>> I would prefer the line to be more terse:
>
>> DETAIL:  90 transactions to finish.
>
>> Am I the only one who is annoyed by this phrase?
>
> Our style guidelines say that detail messages should be complete
> sentences, so I don't like your proposal too much.
>
> Maybe "N transactions remain to finish." ?

waiting for N transactions to complete

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


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

Re: [HACKERS] Partitioned tables and relfilenode

[Robert Haas]

On Tue, Apr 11, 2017 at 10:15 PM, Amit Langote
 wrote:
> Alright.  So I made it into two patches instead: 0001 fixes the bug that
> validateCheckConstraint() tries to scan partitioned tables and 0002 makes
> trying to convert a partitioned table to a view a user error.

Committed together, after updating the regression test outputs to make
the tests pass.  You forgot to update the expected output file in
0002.

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


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

Re: [HACKERS] snapbuild woes

[Simon Riggs]

On 3 March 2017 at 00:30, Petr Jelinek  wrote:

>> 0004 - Changes handling of the xl_running_xacts in initial snapshot
>> build to what I wrote above and removes the extra locking from
>> LogStandbySnapshot introduced by logical decoding.

This seems OK and unlikely to have wider impact.

The "race condition" we're speaking about is by design, not a bug.

I think the initial comment could be slightly better worded; if I
didn't already know what is being discussed, I wouldnt be too much
further forwards from those comments.

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


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

Re: [HACKERS] GCC 7 warnings

[Tom Lane]

Peter Eisentraut  writes:
> On 4/12/17 00:12, Tom Lane wrote:
>> Now a human can see that saved_timeval.tv_usec must be 0..99, so
>> that the %d format item must always emit exactly 3 characters, which
>> means that really 5 bytes would be enough.  I wouldn't expect a
>> compiler to know that, but if it's making a generic assumption about
>> the worst-case width of %d, shouldn't it conclude that we might need
>> as many as 13 bytes for the buffer?  Why does msbuf[10] satisfy it
>> if msbuf[8] doesn't?

> Because the /1000 takes off three digits?

Huh ... I would not really have expected it to figure that out, but
this makes it pretty clear that it did:

> test.c:11:15: note: directive argument in the range [-2147483, 2147483]


> (This is with -O2.  With -O0 it only asks for 5 bytes.)

But that reinforces my suspicion that the intelligence brought to bear
here will be variable.  I'd still go for msbuf[16]; it's not like
anybody's going to notice the extra stack consumption.

regards, tom lane


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

Re: [HACKERS] the need to finish

[Tom Lane]

Erik Rijkers  writes:
> Logical replication emits logmessages like these:
> DETAIL:  90 transactions need to finish.
> DETAIL:  87 transactions need to finish.
> DETAIL:  70 transactions need to finish.

> Could we get rid of that 'need'?   It strikes me as a bit off; something 
> that people would say but not a mechanical message by a computer. I 
> dislike it strongly.

> I would prefer the line to be more terse:

> DETAIL:  90 transactions to finish.

> Am I the only one who is annoyed by this phrase?

Our style guidelines say that detail messages should be complete
sentences, so I don't like your proposal too much.

Maybe "N transactions remain to finish." ?

regards, tom lane


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

Re: [HACKERS] Some thoughts about SCRAM implementation

[Bruce Momjian]

On Wed, Apr 12, 2017 at 12:13:03PM +0300, Heikki Linnakangas wrote:
> >That said, I stand by my comment that I don't think it's the enterprises
> >that need or want the channel binding. If they care about it, they have
> >already put certificate validation in place, and it won't buy them anything.
> >
> >Because channel binding also only secures the authentication (SCRAM), not
> >the actual contents and commands that are then sent across the channel,
> >AFAIK?
> 
> TLS protects the contents and the commands. The point of channel binding is
> to defeat a MITM attack, where the client connects to a malicious server,
> using TLS, which then connects to the real server, using another TLS
> connection. Channel binding will detect that the client and the real server
> are not communicating over the same TLS connection, but two different TLS
> connections, and make the authentication fail.
> 
> SSL certificates, with validation, achieves the same, but channel binding
> achieves it without the hassle of certificates.

How does it do that?

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +


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

Re: [HACKERS] TAP tests take a long time

[Tom Lane]

Mithun Cy  writes:
> I have tried to optimize the tests maintaining the same coverage we were
> able to get with it.

This patch looks good to me: on my workstation, it reduces the total
runtime of the parallel regression tests from ~20.5 to ~16.5 seconds.
I concur that it doesn't look like it would reduce test coverage
significantly.  It gets rid of a VACUUM FULL and a REINDEX, both of
which should be equivalent to an ordinary index build so far as hash is
concerned, and it trims the volume of data being run through the test.

If no objections, I'll push this soon.

regards, tom lane


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

Re: [HACKERS] GCC 7 warnings

[Peter Eisentraut]

On 4/12/17 00:12, Tom Lane wrote:
> The change in setup_formatted_log_time seems a bit weird:
> 
> - charmsbuf[8];
> + charmsbuf[10];
> 
> The associated call is
> 
>   sprintf(msbuf, ".%03d", (int) (saved_timeval.tv_usec / 1000));
> 
> Now a human can see that saved_timeval.tv_usec must be 0..99, so
> that the %d format item must always emit exactly 3 characters, which
> means that really 5 bytes would be enough.  I wouldn't expect a
> compiler to know that, but if it's making a generic assumption about
> the worst-case width of %d, shouldn't it conclude that we might need
> as many as 13 bytes for the buffer?  Why does msbuf[10] satisfy it
> if msbuf[8] doesn't?

Because the /1000 takes off three digits?

The full message from an isolated test case is

test.c: In function 'f':
test.c:11:15: warning: '%03d' directive writing between 3 and 8 bytes
into a region of size 7 [-Wformat-overflow=]
  sprintf(buf, ".%03d", (int) (tv.tv_usec / 1000));
   ^
test.c:11:15: note: directive argument in the range [-2147483, 2147483]
test.c:11:2: note: '__builtin___sprintf_chk' output between 5 and 10
bytes into a destination of size 8
  sprintf(buf, ".%03d", (int) (tv.tv_usec / 1000));
  ^

(This is with -O2.  With -O0 it only asks for 5 bytes.)

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


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

Re: [HACKERS] Logical replication and inheritance

[Robert Haas]

On Wed, Apr 5, 2017 at 8:25 AM, Peter Eisentraut
 wrote:
> After thinking about it some more, I think the behavior we want would be
> that changes to inheritance would reflect in the publication membership.
>  So if you have a partitioned table, adding more partitions over time
> would automatically add them to the publication.

I think the threshold question here is: is the partitioned table a
member of the publication, or are the partitions members of the
publication?  Probably both should be allowed.  If you add a partition
to the publication by name, then we should publish exactly that
partition, full stop.  If you add the partitioned table, then there is
a choice of behaviors, including:

(1) That's an error; the user should publish the partitions instead.
(2) That's a shorthand for all partitions, resolved at the time the
table is added to the publication.  Later changes affect nothing.
(3) All partitions are published, and changes in the set of partitions
change what is published.

In my view, (3) is more desirable than (1) which is more desirable than (2).

> We could implement this either by updating pg_publication_rel as
> inheritance changes, or perhaps more easily by checking and expanding
> inheritance in the output plugin/walsender.  There might be subtle
> behavioral differences between those two approaches that we should think
> through.  But I think one of these two should be done.

The latter approach sounds better to me.

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


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

[HACKERS] the need to finish

[Erik Rijkers]

Logical replication emits logmessages like these:

DETAIL:  90 transactions need to finish.
DETAIL:  87 transactions need to finish.
DETAIL:  70 transactions need to finish.

Could we get rid of that 'need'?   It strikes me as a bit off; something 
that people would say but not a mechanical message by a computer. I 
dislike it strongly.


I would prefer the line to be more terse:

DETAIL:  90 transactions to finish.

Am I the only one who is annoyed by this phrase?


Thanks,


Erik Rijkers















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

Re: [HACKERS] Undefined psql variables

[Robert Haas]

On Sun, Apr 2, 2017 at 3:56 PM, Tom Lane  wrote:
> So my view of this is that "send the expression to the server" ought
> to be just one option for \if, not the only way to do it.

I heartily agree.  There should be some kind of client-side expression
language, and one thing it should allow is calling out to the server.
Then people who only want to call out to the server can do that, but
people who want to do something else have the option.  Insisting that
this facility isn't allowed to do anything other than consult the
server is (1) inconsistent with what we've already got in v10 and (2)
boxing ourselves into a corner for no very good reason.

Now, the optimal shape for that client-side expression language is not
very clear to me.  Do we want to invent our own language, or maybe
consider using something that already exists?  It's been previously
suggested that we should somehow embed Lua, and this might not be a
bad place to consider doing something like that.  That might be a way
to add a lot of power without having to invent an entirely new
programming language one bit at a time.  If we want to invent our own
expression language, what kind of syntax should it use?  Upon what
kind of design principles should it be based?  There's likely to be
vigorous debate on these topics, and probably also complaints that the
good designs are too much work and the easy-to-implement designs are
too limiting.  (Regular readers of this mailing list will likely be
able to guess which side of those debates I'll be on, but we need to
have them all the same.)

Regarding the ostensible topic of this thread, one thought I had while
reading through these various responses is that the original need
would be well-served by the (somewhat dubious) syntax that bash uses
for variable substitution.  Obviously, we aren't going to change the
interpolate-this-variable character from : to $, but bash has
${parameter:-word} to substitute a default for an unset parameter,
${parameter:=word} to substitute a default for an unset parameter and
also set the parameter to that value, ${parameter:?word} to error out
with word as the error mesage if parameter is not set, and so forth.
If we decide to roll our own, we might consider taking inspiration
from those constructs.

I think that one of the general problems of language design is, as
Larry Wall once said, that a good language should make simple things
simple and complex things possible.  But simple is not an absolute; it
depends on context.  The things which a language needs to make simple
are those things which will be done frequently *in that language*.  So
for example in this case, out-calls to SQL need to be very easy to
write.  Maybe the empty-parameter thing needs to be easy; not sure.
Coming up with a good solution here will involve understanding what
people typically want to do with a language of this type and then
making sure that stuff can be done succinctly - and ideally also
making sure that other stuff is also possible if you're willing to put
in more legwork.

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


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

Re: [HACKERS] snapbuild woes

[Peter Eisentraut]

On 4/12/17 02:31, Noah Misch wrote:
>>> But I hope you mean to commit these snapbuild patches before the postgres 10
>>> release?  As far as I know, logical replication is still very broken without
>>> them (or at least some of that set of 5 patches - I don't know which ones
>>> are essential and which may not be).
>>
>> Yes, these should go into 10 *and* earlier releases, and I don't plan to
>> wait for 2017-07.
> 
> [Action required within three days.  This is a generic notification.]

I'm hoping for a word from Andres on this.

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


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

Re: [HACKERS] Logical replication and inheritance

[Peter Eisentraut]

On 4/9/17 22:16, Noah Misch wrote:
> On Wed, Apr 05, 2017 at 08:25:56AM -0400, Peter Eisentraut wrote:
>> After thinking about it some more, I think the behavior we want would be
>> that changes to inheritance would reflect in the publication membership.
>>  So if you have a partitioned table, adding more partitions over time
>> would automatically add them to the publication.
>>
>> We could implement this either by updating pg_publication_rel as
>> inheritance changes, or perhaps more easily by checking and expanding
>> inheritance in the output plugin/walsender.  There might be subtle
>> behavioral differences between those two approaches that we should think
>> through.  But I think one of these two should be done.
> 
> [Action required within three days.  This is a generic notification.]

Relative to some of the other open items I'm involved in, I consider
this a low priority, so I'm not working on it right now.  I would also
appreciate some guidance from those who are more involved in inheritance
and partitioning to determine the best behavior.  It could be argued
that the current behavior is correct, and also that there are several
other correct behaviors.

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


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

Re: [HACKERS] Cutting initdb's runtime (Perl question embedded)

[Andreas Karlsson]

On 04/12/2017 04:12 PM, Tom Lane wrote:

1. The best thing would still be to make genbki.pl do the conversion,
and write numeric OIDs into postgres.bki.  The core stumbling block
here seems to be that for most catalogs, Catalog.pm and genbki.pl
never really break down a DATA line into fields --- and we certainly
have got to do that, if we're going to replace the values of regproc
fields.  The places that do need to do that approximate it like this:

# To construct fmgroids.h and fmgrtab.c, we need to inspect some
# of the individual data fields.  Just splitting on whitespace
# won't work, because some quoted fields might contain internal
# whitespace.  We handle this by folding them all to a simple
# "xxx". Fortunately, this script doesn't need to look at any
# fields that might need quoting, so this simple hack is
# sufficient.
$row->{bki_values} =~ s/"[^"]*"/"xxx"/g;
@{$row}{@attnames} = split /\s+/, $row->{bki_values};

We would need a bullet-proof, non-hack, preferably not too slow way to
split DATA lines into fields properly.  I'm one of the world's worst
Perl programmers, but surely there's a way?

2. Alternatively, we could teach bootstrap mode to build a hashtable
mapping proname to OID while it reads pg_proc data from postgres.bki,
and then make regprocin's bootstrap path consult the hashtable instead
of looking directly at the pg_proc file.  That I'm quite sure is do-able,
but it seems like it's leaving money on the table compared to doing
the transformation earlier.

Thoughts?


Looked at this an option 1 seems simple enough if I am not missing 
something. I might hack something up later tonight. Either way I think 
this improvement can be done separately from the proposed replacement of 
the catalog header files. Trying to fix everything at once often leads 
to nothing being fixed at all.


Andreas


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