Re: [HACKERS] More stable query plans via more predictable column statistics

[Alex Shulgin]

On Mon, Apr 4, 2016 at 1:06 AM, Tom Lane <t...@sss.pgh.pa.us> wrote:

> Alex Shulgin <alex.shul...@gmail.com> writes:
> > On Sun, Apr 3, 2016 at 10:53 PM, Tom Lane <t...@sss.pgh.pa.us> wrote:
> >> The reason for checking toowide_cnt is that if it's greater than zero,
> >> then in fact the track list does NOT include all values seen, and it's
> >> flat-out wrong to claim that it is an exhaustive set of values.
>
> > But do we really state that with the short path?
>
> Well, yes: the point of the short path is that we're hypothesizing that
> the track list contains all values in the table, and that they should all
> be considered MCVs.  Another way to think about it is that if we didn't
> have the width-limit implementation restriction, those values would appear
> in the track list, almost certainly with count 1, and so we would not
> have taken the short path anyway.
>
> Now you can argue that the long path would have accepted all the real
> track-list entries as MCVs, and have rejected all these hypothetical
> count-1 entries for too-wide values, and so the end result would be the
> same.  But that gets back to the fact that that's not necessarily how
> the long path behaves, either today or with the proposed patch.
>

 Agreed.

The design intention was that the short path would handle columns
> with a finite, small set of values (think booleans or enums) where the
> ideal thing is that the table population is completely represented by
> the MCV list.  As soon as we have good reason to think that the MCV
> list won't represent the table contents fully, we should switch over
> to a different approach where we're trying to identify which sample
> values are common enough to justify putting in the MCV list.


This is a precious detail that I unfortunately couldn't find in any of the
sources of information available to me online. :-)

I don't have a habit of hanging on IRC channels, but now I wonder how
likely is it that I could learn this by just asking around on #postgresql
(or mailing you directly as the committer of this early implementation--is
that OK at all?)

Again, having this type of design decisions documented in the code might
save some time and confusion for the sociopath^W introvert-type of folks
like myself. ;-)

In that
> situation there are good reasons to not blindly fill the MCV list all
> the way to the stats-target limit, but to try to cut it off at the
> point of diminishing returns, so that the planner isn't saddled with
> a huge MCV list that doesn't really contain a lot of useful information.
>

This came to be my understanding also at some point.

So that's the logic behind there being two code paths with discontinuous
> behavior.  I'm not sure whether we need to try to narrow the discontinuity
> or whether it's OK to act that way and we just need to refine the decision
> rule about which path to take.  But anyway, comparisons of frequencies
> of candidate MCVs seem to me to make sense in a large-ndistinct scenario
> (where we have to be selective) but not a small-ndistinct scenario
> (where we should just take 'em all).
>

Yeah, this seems to be an open question.  And a totally new one to me in
the light of recent revelations.

>> The point of the original logic was to try to decide whether the
> >> values in the sample are significantly more common than typical values
> >> in the whole table population.  I think we may have broken that with
> >> 3d3bf62f3: you can't make any such determination if you consider only
> >> what's in the sample without trying to estimate what is not in the
> >> sample.
>
> > Speaking of rabbit holes...
> > I'm out of ideas, unfortunately.  We badly need more eyes/brainpower on
> > this, which is why I have submitted a talk proposal on this topic to
> > PGDay.ru this summer in St. Petersburg, fearing that it might be too late
> > to commit a satisfactory version during the current dev cycle for 9.6,
> and
> > in hope to draw at least some attention to it.
>
> If you're thinking it's too late to get more done for 9.6,


Not necessarily, but given the time constraints and some personal issues
that just keep popping up I'm not that optimistic as I was just 24h ago
anymore.


> I'm inclined to
> revert the aspect of 3d3bf62f3 that made us work from "d" (the observed
> number of distinct values in the sample) rather than stadistinct (the
> extrapolated estimate for the table).  On reflection I think that that's
> inconsistent with the theory behind the old MCV-cutoff rule.  It wouldn't
> matter if we were going to replace the cutoff rule with something else,
> but it's beginning to sound like that won't happen for 9.6.
>

Please feel free to do what you think is in the best interest of the people
preparing 9.6 for the freeze.  I'm not all that familiar with the process,
but I guess reverting this early might save some head-scratching if some
interesting interactions of this change combined with some others are going
to show up.

Cheers!
--
Alex

Re: [HACKERS] More stable query plans via more predictable column statistics

[Alex Shulgin]

On Sun, Apr 3, 2016 at 10:53 PM, Tom Lane <t...@sss.pgh.pa.us> wrote:

> Alex Shulgin <alex.shul...@gmail.com> writes:
> > This recalled observation can now also explain to me why in the
> regression
> > you've seen, the short path was not followed: my bet is that stadistinct
> > appeared negative.
>
> Yes, I think that's right.  The table under consideration had just a few
> live rows (I think 3), so that even though there was only one value in
> the sample, the "if (stats->stadistinct > 0.1 * totalrows)" condition
> succeeded.
>

Yeah, this part of the logic can be really surprising at times.

> Given that we change the logic in the complex path substantially, the
> > assumptions that lead to the "Take all MVCs" condition above might no
> > longer hold, and I see it as a pretty compelling argument to remove the
> > extra checks, thus keeping the only one: track_cnt == ndistinct.  This
> > should also bring the patch's effect more close to the thread's topic,
> > which is "More stable query plans".
>
> The reason for checking toowide_cnt is that if it's greater than zero,
> then in fact the track list does NOT include all values seen, and it's
> flat-out wrong to claim that it is an exhaustive set of values.
>

But do we really state that with the short path?

If there would be only one too wide value, it might be the only thing left
for the histogram in the end and will be discarded anyway, so from the end
result perspective there is no difference.

If there are multiple too wide values, they will be effectively discarded
by the histogram calculation part also, so again no difference from the
perspective of the end result.

The reason for the track_cnt <= num_mcv condition is that if that's not
> true, the track list has to be trimmed to meet the statistics target.
> Again, that's not optional.
>

Yes, but this check we only need in compute_distinct_stats() and we are
talking about compute_scalar_stats() now where track_cnt is always less
than or equals to num_mcv (again, please see at the bottom of the
thread-starting email), or is my analysis broken on this part?

I think the reasoning for having the stats->stadistinct > 0 test in there
> was that if we'd set it negative, then we think that the set of distinct
> values will grow --- which again implies that the set of values actually
> seen should not be considered exhaustive.


This is actually very neat.  So the idea here as I get it is that if we
have enough distinct values to suspect that more unique ones will be added
later as the table grows (which is a natural tendency with most of the
tables anyway), then at the moment the statistics we produce are going to
be actually used by the planner, it is likely that we no longer cover all
the distinct values by the MCV list, right?

I would *love* to see that documented in code comments at the least.

Of course, with a table as
> small as that regression-test example, we have little evidence to support
> either that conclusion or its opposite.
>

I think it might be possible to record historical ndistinct values between
the ANALYZE runs and use that as better evidence that the number of
distincts is actually growing rather than basing that decision on that
hard-coded 10% limit rule.  What do you think?

We do not support migration of pg_statistic system table during major
version upgrades (yet), so if we somehow achieve what I've just described,
it might be not a compatibility-breaking change anyway.

It's possible that what we should do to eliminate the sudden change
> of behaviors is to drop the "track list includes all values seen, and all
> will fit" code path entirely, and always go through the track list
> one-at-a-time.
>

That could also be an option, that I have considered initially.  Now that I
read your explanation of each check, I'm not that sure anymore.

If we do, though, the currently-proposed filter rules aren't going to
> be too satisfactory: if we have a relatively small group of roughly
> equally common MCVs, this logic would reject all of them, which is
> surely not what we want.
>

Indeed. :-(


> The point of the original logic was to try to decide whether the
> values in the sample are significantly more common than typical values
> in the whole table population.  I think we may have broken that with
> 3d3bf62f3: you can't make any such determination if you consider only
> what's in the sample without trying to estimate what is not in the
> sample.
>

Speaking of rabbit holes...

I'm out of ideas, unfortunately.  We badly need more eyes/brainpower on
this, which is why I have submitted a talk proposal on this topic to
PGDay.ru this summer in St. Petersburg, fearing that it might be too late
to commit a satisfactory version during the current dev cycle for 9.6, and
in hope to draw at least some attention to it.

Regards,
--
Alex

Re: [HACKERS] More stable query plans via more predictable column statistics

[Alex Shulgin]

On Sun, Apr 3, 2016 at 8:24 AM, Alex Shulgin <alex.shul...@gmail.com> wrote:
>
> On Sun, Apr 3, 2016 at 7:49 AM, Tom Lane <t...@sss.pgh.pa.us> wrote:
>>
>> Alex Shulgin <alex.shul...@gmail.com> writes:
>> > On Sun, Apr 3, 2016 at 7:18 AM, Tom Lane <t...@sss.pgh.pa.us> wrote:
>> >> Well, we have to do *something* with the last (possibly only) value.
>> >> Neither "include always" nor "omit always" seem sane to me.  What
other
>> >> decision rule do you want there?
>>
>> > Well, what implies that the last value is somehow special?  I would
think
>> > we should just do with it whatever we do with the rest of the candidate
>> > MCVs.
>>
>> Sure, but both of the proposed decision rules break down when there are
no
>> values after the one under consideration.  We need to do something sane
>> there.
>
>
> Hm... There are indeed the case where it would beneficial to have at
least 2 values in the histogram (to have at least the low/high bounds for
inequality comparison selectivity) instead of taking both to the MCV list
or taking one to the MCVs and having to discard the other.

I was thinking about this in the background...

Popularity of the last sample value (which is not the only) one can be:

a) As high as 50%, in case we have an even division between the only two
values in the sample.  Quite obviously, we should take this one into the
MCV list (well, unless the user has specified statistics_target of 1 for
some bizarre reason, but that should not be our problem).

b) As low as 2/(statistics_target*300), which is with the target set to a
maximum allowed value of 10,000 amounts to 2/(10,000*300) = 1 in
1,500,000.  This seems like a really tiny number, but if your table has
some tens of billions of rows, for example, seeing such a value at least
twice means that it might correspond to some thousands of rows in the
table, whereas seeing a value only once might mean just that: it's a unique
value.

In this case, putting such a duplicate value in the MCV list will allow a
much better selectivity estimate for equality comparison, as I've mentioned
earlier.  It also allows for better estimate with inequality comparison,
since MCVs are also consulted in this case.  I see no good reason to
discard such a value.

c) Or anything in between the above figures.

In my opinion that amounts to "include always" being the sane option.  Do
you see anything else as a problem here?

> Obviously, we need a fresh idea on how to handle this.

On reflection, the case where we have a duplicate value in the track list
which is not followed by any other sample should be covered by the short
path where we put all the tracked values in the MCV list, so there should
be no point to even consider all of the above!

But the exact short path condition is formulated like this:

if (track_cnt == ndistinct && toowide_cnt == 0 &&
stats->stadistinct > 0 &&
track_cnt <= num_mcv)
{
/* Track list includes all values seen, and all will fit */

So the execution path here is additionally put in dependence of two
factors: whether we've seen at least one too wide sample or the distinct
estimation produced a number higher than 10% of the estimated total table
size (which is yet another arbitrary limit, but that's not in scope of this
patch).

I've been puzzled by these conditions a lot, as I have mentioned in the
last section of this thread's starting email and I could not find anything
that would hint why they exist there, in the documentation, code comments
or emails on hackers leading to the introduction of analyze.c in the form
we know it today.  Probably we will never know, unless Tom still has some
notes on this topic from 15 years ago. ;-)

This recalled observation can now also explain to me why in the regression
you've seen, the short path was not followed: my bet is that stadistinct
appeared negative.

Given that we change the logic in the complex path substantially, the
assumptions that lead to the "Take all MVCs" condition above might no
longer hold, and I see it as a pretty compelling argument to remove the
extra checks, thus keeping the only one: track_cnt == ndistinct.  This
should also bring the patch's effect more close to the thread's topic,
which is "More stable query plans".

Regards,
--
Alex

Re: [HACKERS] More stable query plans via more predictable column statistics

[Alex Shulgin]

On Sun, Apr 3, 2016, 18:40 Tom Lane <t...@sss.pgh.pa.us> wrote:

> Alex Shulgin <alex.shul...@gmail.com> writes:
>
> > Well, if it's the only value it will be accepted simply because we are
> > checking that special case already and don't even bother to loop through
> > the track list.
>
> That was demonstrably not the case in the failing regression test.
> I forget what aspect of the test case allowed it to get past the short
> circuit, but it definitely got into the scan-the-track-list code.
>

Hm, I'll have to see that for myself, probably there was something more to
it.

--
Alex

Re: [HACKERS] Add schema-qualified relnames in constraint error messages.

[Alex Shulgin]

On Sun, Apr 3, 2016, 18:32 Tom Lane  wrote:

> Peter Geoghegan  writes:
> > On Thu, Feb 11, 2016 at 9:50 AM, Robert Haas 
> wrote:
> >> Wow, that's a fabulous idea.  I see Oleksandr has tried to implement
> >> it, although I haven't looked at the patch.  But I think this would be
> >> REALLY helpful.
>
> > +1
>
> Pushed.
>

Yay!

Re: [HACKERS] More stable query plans via more predictable column statistics

[Alex Shulgin]

On Sun, Apr 3, 2016 at 7:49 AM, Tom Lane <t...@sss.pgh.pa.us> wrote:

> Alex Shulgin <alex.shul...@gmail.com> writes:
> > On Sun, Apr 3, 2016 at 7:18 AM, Tom Lane <t...@sss.pgh.pa.us> wrote:
> >> Well, we have to do *something* with the last (possibly only) value.
> >> Neither "include always" nor "omit always" seem sane to me.  What other
> >> decision rule do you want there?
>
> > Well, what implies that the last value is somehow special?  I would think
> > we should just do with it whatever we do with the rest of the candidate
> > MCVs.
>
> Sure, but both of the proposed decision rules break down when there are no
> values after the one under consideration.  We need to do something sane
> there.
>

Hm... There are indeed the case where it would beneficial to have at least
2 values in the histogram (to have at least the low/high bounds for
inequality comparison selectivity) instead of taking both to the MCV list
or taking one to the MCVs and having to discard the other.

Obviously, we need a fresh idea on how to handle this.

> For "the only value" case: we cannot build a histogram out of a single
> > value, so omitting it from MCVs is not a good strategy, ISTM.
> > From my point of view that amounts to "include always".
>
> If there is only one value, it will have 100% of the samples, so it would
> get included under just about any decision rule (other than "more than
> 100% of this value plus following values").  I don't think making sure
> this case works is sufficient to get us to a reasonable rule --- it's
> a necessary case, but not a sufficient case.
>

Well, if it's the only value it will be accepted simply because we are
checking that special case already and don't even bother to loop through
the track list.

--
Alex

Re: [HACKERS] More stable query plans via more predictable column statistics

[Alex Shulgin]

On Sun, Apr 3, 2016 at 7:18 AM, Tom Lane <t...@sss.pgh.pa.us> wrote:

> Alex Shulgin <alex.shul...@gmail.com> writes:
> > On Sun, Apr 3, 2016 at 3:43 AM, Alex Shulgin <alex.shul...@gmail.com>
> wrote:
> >> I'm not sure yet about the 1% rule for the last value, but would also
> love
> >> to see if we can avoid the arbitrary limit here.  What happens with a
> last
> >> value which is less than 1% popular in the current code anyway?
>
> > Now that I think about it, I don't really believe this arbitrary
> heuristic
> > is any good either, sorry.
>
> Yeah, it was just a placeholder to produce a working patch.
>
> Maybe we could base this cutoff on the stats target for the column?
> That is, "1%" would be the right number if stats target is 100,
> otherwise scale appropriately.
>
> > What was your motivation to introduce some limit at the bottom anyway?
>
> Well, we have to do *something* with the last (possibly only) value.
> Neither "include always" nor "omit always" seem sane to me.  What other
> decision rule do you want there?
>

Well, what implies that the last value is somehow special?  I would think
we should just do with it whatever we do with the rest of the candidate
MCVs.

For "the only value" case: we cannot build a histogram out of a single
value, so omitting it from MCVs is not a good strategy, ISTM.

>From my point of view that amounts to "include always".  What problems do
you see with this approach exactly?

--
Alex

Re: [HACKERS] More stable query plans via more predictable column statistics

[Alex Shulgin]

On Sun, Apr 3, 2016 at 3:43 AM, Alex Shulgin <alex.shul...@gmail.com> wrote:

>
> I'm not sure yet about the 1% rule for the last value, but would also love
> to see if we can avoid the arbitrary limit here.  What happens with a last
> value which is less than 1% popular in the current code anyway?
>

Tom,

Now that I think about it, I don't really believe this arbitrary heuristic
is any good either, sorry.  What if you have a value that is just a bit
under 1% popular, but is being used in 50% of your queries in WHERE clause
with equality comparison?  Without this value in the MCV list the planner
will likely use SeqScan instead of an IndexScan that might be more
appropriate here.  I think we are much better off if we don't touch this
aspect of the current code.

What was your motivation to introduce some limit at the bottom anyway?  If
that was to prevent accidental division by zero, then an explicit check on
denominator not being 0 seems to me like a better safeguard than this.

Regards.
--
Alex

Re: [HACKERS] Add schema-qualified relnames in constraint error messages.

[Alex Shulgin]

On Sat, Apr 2, 2016 at 11:41 PM, Tom Lane  wrote:

> "Shulgin, Oleksandr"  writes:
> > On Mon, Mar 14, 2016 at 7:55 PM, Tom Lane  wrote:
> >> Yeah, I don't much like that either.  But I don't think we can avoid
> >> some refactoring there; as designed, conversion of an error message into
> >> user-visible form is too tightly tied to receipt of the message.
>
> > True.  Attached is a v2 which addresses all of the points raised earlier
> I
> > believe.
>
> I took a closer look at what's going on here and realized that actually
> it's not that hard to decouple the message-building routine from the
> PGconn state, because mostly it works with fields it's extracting out
> of the PGresult anyway.  The only piece of information that's lacking
> is conn->last_query.  I propose therefore that instead of doing it like
> this, we copy last_query into error PGresults.  This is strictly less
> added storage requirement than storing the whole verbose message would be,
> and it saves time compared to the v2 patch in the typical case where
> the application never does ask for an alternately-formatted error message.
> Plus we can actually support requests for any variant format, not only
> VERBOSE.
>
> Attached is a libpq-portion-only version of a patch doing it this way.
> I've not yet looked at the psql part of your patch.
>
> Comments?
>

Ah, neat, that's even better. :-)

What about regression tests?  My assumption was that we won't be able to
add them with the usual expected file approach, but that we also don't need
it that hard.  Everyone's in favor?

--
Alex

Re: [HACKERS] More stable query plans via more predictable column statistics

[Alex Shulgin]

On Sat, Apr 2, 2016 at 8:57 PM, Shulgin, Oleksandr <
oleksandr.shul...@zalando.de> wrote:
> On Apr 2, 2016 18:38, "Tom Lane"  wrote:
>
>> I did not like the fact that the compute_scalar_stats logic
>> would allow absolutely anything into the MCV list once num_hist falls
>> below 2. I think it's important that we continue to reject values that
>> are only seen once in the sample, because there's no very good reason to
>> think that they are MCVs and not just infrequent values that by luck
>> appeared in the sample.
>
> In my understanding we only put a value in the track list if we've seen it
> at least twice, no?

This is actually the case for compute_scalar_stats, but not for
compute_distinct_stats.  In the latter case we can still have
track[i].count == 1, but we can also break out of the loop if we see the
first tracked item like that.

>> Before I noticed the regression failure, I'd been thinking that maybe
it'd
>> be better if the decision rule were not "at least 100+x% of the average
>> frequency of this value and later ones", but "at least 100+x% of the
>> average frequency of values after this one".
>
> Hm, sounds pretty similar to what I wanted to achieve, but better
> formalized.
>
>> With that formulation, we're
>> not constrained as to the range of x.  Now, if there are *no* values
after
>> this one, then this way needs an explicit special case in order not to
>> compute 0/0; but the preceding para shows that we need a special case for
>> the last value anyway.
>>
>> So, attached is a patch rewritten along these lines.  I used 50% rather
>> than 25% as the new cutoff percentage --- obviously it should be higher
>> in this formulation than before, but I have no idea if that particular
>> number is good or we should use something else.  Also, the rule for the
>> last value is "at least 1% of the non-null samples".  That's a pure guess
>> as well.
>>
>> I do not have any good corpuses of data to try this on.  Can folks who
>> have been following this thread try it on their data and see how it
>> does?  Also please try some other multipliers besides 1.5, so we can
>> get a feeling for where that cutoff should be placed.
>
> Expect me to run it on my pet db early next week. :-)

I was trying to come up with some examples where 50% could be a good or a
bad choice and then I noticed that we might be able to turn it it the other
way round: instead of inventing an arbitrary limit at the MCVs frequency we
could use the histogram as the criteria for a candidate MCV to be
considered "common enough".  If we can prove that the value would produce
duplicates in the histogram, we should rather put it in the MCV list
(unless it's already fully occupied, then we can't do anything).

A value is guaranteed to produce a duplicate if it has appeared at least
2*hfreq+1 times in the sample (hfreq from your version of the patch, which
is recalculated on every loop iteration).  I could produce an updated patch
on Monday or anyone else following this discussion should be able to do
that.

This approach would be a huge win in my opinion, because this way we can
avoid all the arbitrariness of that .25 / .50 multiplier.  Otherwise there
might be (valid) complaints that for some data .40 (or .60) is a better
fit, but we have already hard-coded something and there would be no easy
way to improve situation for some users while avoiding to break it for the
rest (unless we introduce a per-attribute configurable parameter like
statistics_target for this multiplier, which I'd like to avoid even
thinking about ;-)

While we don't (well, can't) build a histogram in the
compute_distinct_stats variant, we could also apply the above mind trick
there for the same reason and to make the output of both functions more
consistent (and to have less maintenance burden between the variants).  And
anyway it would be rather surprising to see that depending on the presence
of an order operator for the type, the resulting MCV lists after the
ANALYZE would be different (I mean not only due to the random nature of the
sample).

I'm not sure yet about the 1% rule for the last value, but would also love
to see if we can avoid the arbitrary limit here.  What happens with a last
value which is less than 1% popular in the current code anyway?

Cheers!
--
Alex

Re: [HACKERS] Sanity checking for ./configure options?

[Alex Shulgin]

The following review has been posted through the commitfest application:
make installcheck-world:  not tested
Implements feature:   tested, passed
Spec compliant:   not tested
Documentation:not tested

Looks good to me.  It only allows valid number between 1 and 65535, disallows 
leading zero, empty string, or non-digit chars.  Error messages looks good.

Marking this Ready for Committer.

--
Alex


The new status of this patch is: Ready for Committer

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

[HACKERS] POC PATCH: copy from ... exceptions to: (was Re: VLDB Features)

[Alex Shulgin]

Trent Shipley trent_ship...@qwest.net writes:

 On Friday 2007-12-14 16:22, Tom Lane wrote:
 Neil Conway ne...@samurai.com writes:
  By modifying COPY: COPY IGNORE ERRORS or some such would instruct COPY
  to drop (and log) rows that contain malformed data. That is, rows with
  too many or too few columns, rows that result in constraint violations,
  and rows containing columns where the data type's input function raises
  an error. The last case is the only thing that would be a bit tricky to
  implement, I think: you could use PG_TRY() around the InputFunctionCall,
  but I guess you'd need a subtransaction to ensure that you reset your
  state correctly after catching an error.

 Yeah.  It's the subtransaction per row that's daunting --- not only the
 cycles spent for that, but the ensuing limitation to 4G rows imported
 per COPY.

 You could extend the COPY FROM syntax with a COMMIT EVERY n clause.  This 
 would help with the 4G subtransaction limit.  The cost to the ETL process is 
 that a simple rollback would not be guaranteed send the process back to it's 
 initial state.  There are easy ways to deal with the rollback issue though.  

 A {NO} RETRY {USING algorithm} clause might be useful.   If the NO RETRY 
 option is selected then the COPY FROM can run without subtransactions and in 
 excess of the 4G per transaction limit.  NO RETRY should be the default since 
 it preserves the legacy behavior of COPY FROM.

 You could have an EXCEPTIONS TO {filename|STDERR} clause. I would not give 
 the 
 option of sending exceptions to a table since they are presumably malformed, 
 otherwise they would not be exceptions.  (Users should re-process exception 
 files if they want an if good then table a else exception to table b ...)

 EXCEPTIONS TO and NO RETRY would be mutually exclusive.


 If we could somehow only do a subtransaction per failure, things would
 be much better, but I don't see how.

Hello,

Attached is a proof of concept patch for this TODO item.  There is no
docs yet, I just wanted to know if approach is sane.

The added syntax is like the following:

  COPY [table] FROM [file/program/stdin] EXCEPTIONS TO [file or stdout]

The way it's done it is abusing Copy Both mode and from my limited
testing, that seems to just work.  The error trapping itself is done
using PG_TRY/PG_CATCH and can only catch formatting or before-insert
trigger errors, no attempt is made to recover from a failed unique
constraint, etc.

Example in action:

postgres=# \d test_copy2
  Table public.test_copy2
 Column |  Type   | Modifiers 
+-+---
 id | integer | 
 val| integer | 

postgres=# copy test_copy2 from program 'seq 3' exceptions to stdout;
1
NOTICE:  missing data for column val
CONTEXT:  COPY test_copy2, line 1: 1
2
NOTICE:  missing data for column val
CONTEXT:  COPY test_copy2, line 2: 2
3
NOTICE:  missing data for column val
CONTEXT:  COPY test_copy2, line 3: 3
NOTICE:  total exceptions ignored: 3

postgres=# \d test_copy1
  Table public.test_copy1
 Column |  Type   | Modifiers 
+-+---
 id | integer | not null

postgres=# set client_min_messages to warning;
SET
postgres=# copy test_copy1 from program 'ls /proc' exceptions to stdout;
...
vmstat
zoneinfo
postgres=# 

Limited performance testing shows no significant difference between
error-catching and plain code path.  For example, timing

  copy test_copy1 from program 'seq 100' [exceptions to stdout]

shows similar numbers with or without the added exceptions to clause.

Now that I'm sending this I wonder if the original comment about the
need for subtransaction around every loaded line still holds.  Any
example of what would be not properly rolled back by just PG_TRY?

Happy hacking!
--
Alex

From 50f7ab0a503a0d61776add8a138abf2622fc6c35 Mon Sep 17 00:00:00 2001
From: Alex Shulgin a...@commandprompt.com
Date: Fri, 19 Dec 2014 18:21:31 +0300
Subject: [PATCH] POC: COPY FROM ... EXCEPTIONS TO

---
 contrib/file_fdw/file_fdw.c |   4 +-
 src/backend/commands/copy.c | 251 +---
 src/backend/parser/gram.y   |  26 +++-
 src/bin/psql/common.c   |  14 +-
 src/bin/psql/copy.c | 119 ++-
 src/bin/psql/settings.h |   1 +
 src/bin/psql/startup.c  |   1 +
 src/bin/psql/tab-complete.c |  12 +-
 src/include/commands/copy.h |   3 +-
 src/include/nodes/parsenodes.h  |   1 +
 src/include/parser/kwlist.h |   1 +
 src/interfaces/ecpg/preproc/ecpg.addons |   2 +-
 12 files changed, 396 insertions(+), 39 deletions(-)

diff --git a/contrib/file_fdw/file_fdw.c b/contrib/file_fdw/file_fdw.c
new file mode 100644
index 5a4d5aa..0df02f7
*** a/contrib/file_fdw/file_fdw.c
--- b/contrib/file_fdw/file_fdw.c
*** fileBeginForeignScan(ForeignScanState *n
*** 624,629 
--- 624,630 
  	cstate = BeginCopyFrom(node-ss.ss_currentRelation

Re: [HACKERS] Turning recovery.conf into GUCs

[Alex Shulgin]

Alex Shulgin a...@commandprompt.com writes:

 Petr Jelinek p...@2ndquadrant.com writes:

 I had a quick look, the patch does not apply cleanly anymore but it's
 just release notes so nothing too bad.

 Yes, there were some ongoing changes that touched some parts of this and
 I must have missed the latest one (or maybe I was waiting for it to
 settle down).

The rebased version is attached.

 - the StandbyModeRequested and StandbyMode should be lowercased like
 the rest of the GUCs

 Yes, except that standby_mode is linked to StandbyModeRequested, not the
 other one.  I can try see if there's a sane way out of this.

As I see it, renaming these will only add noise to this patch, and there
is also ArchiveRecoveryRequested / InArchiveRecovery.  This is going to
be tricky and I'm not sure it's really worth the effort.

 - The whole CopyErrorData and memory context logic is not needed in
 check_recovery_target_time() as the FlushErrorState() is not called
 there

 You must be right.  I recall having some trouble with strings being
 free'd prematurely, but if it's ERROR, then there should be no need for
 that.  I'll check again.

Hrm, after going through this again I'm pretty sure that was correct:
the only way to obtain the current error message is to use
CopyErrorData(), but that requires you to switch back to non-error
memory context (via Assert).

The FlushErrorState() call is not there because it's handled by the hook
caller (or it can exit via ereport() with elevel==ERROR).

 - The new code in StartupXLOG() like
 +if (recovery_target_xid_string != NULL)
 +InitRecoveryTarget(RECOVERY_TARGET_XID);
 +
 +if (recovery_target_time_string != NULL)
 +InitRecoveryTarget(RECOVERY_TARGET_TIME);
 +
 +if (recovery_target_name != NULL)
 +InitRecoveryTarget(RECOVERY_TARGET_NAME);
 +
 +if (recovery_target_string != NULL)
 +InitRecoveryTarget(RECOVERY_TARGET_IMMEDIATE);

 would probably be better in separate function that you then call
 StartupXLOG() as StartupXLOG() is crazy long already so I think it's
 preferable to not make it worse.

 We can move it at top of CheckStartingAsStandby() obviously.

Moved.

--
Alex



recovery_guc_v5.6.patch.gz
Description: application/gzip

-- 
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] HINT: pg_hba.conf changed since last config reload

[Alex Shulgin]

Steve Singer st...@ssinger.info writes:

 On 12/15/2014 11:38 AM, Alex Shulgin wrote:

 These are all valid concerns IMHO. Attached is the modified version
 of the original patch by Craig, addressing the handling of the new
 hint_log error data field and removing the client-side HINT. I'm
 also moving this to the current CF. -- Alex




 The updated patch removes the client message. I feel this addresses
 Peter's concern.  The updated patch also addresses the missing free I
 mentioned in my original review.

 The patch applies cleanly to head,

 One thing I'm noticed while testing is that if you do the following

 1. Edit pg_hba.conf  to allow a connection from somewhere
 2. Attempt to connect, you get an error + the hind in the server log
 3. Make another change to pg_hba.conf and introduce an error in the file
 4. Attempt to reload the config files, pg_hba.conf the reload fails
 because of the above error
 5. Attempt to connect you still can't connect since we have the
 original pg_hba.conf loaded

 You don't get the warning in step 5 since we update PgReloadTime even
 if the reload didn't work.

 Is this enough of a concern to justify the extra complexity in
 tracking the reload time of the pg_hba and pg_ident times directly?

I don't think so.  The scenario this patch relies on assumes that the
DBA will remember to look in the log if something goes wrong, and in
your case there would be a message like the following:

WARNING:  pg_hba.conf not reloaded

So an extra hint about file timestamp is unneeded.

--
Alex


-- 
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] HINT: pg_hba.conf changed since last config reload

[Alex Shulgin]

Craig Ringer cr...@2ndquadrant.com writes:

 On 12/19/2014 11:41 PM, Alex Shulgin wrote:
 I don't think so.  The scenario this patch relies on assumes that the
 DBA will remember to look in the log if something goes wrong

 Well, actually, the whole point was that the user who's connecting
 (likely also the DBA) will see a HINT telling them that there's more
 in the logs.

 But that got removed.

While it sounds useful at first glance, I believe Peter's arguments
upthread provide enough justification to avoid sending the hint to the
client.

--
Alex


-- 
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] REVIEW: Track TRUNCATE via pgstat

[Alex Shulgin]

Alvaro Herrera alvhe...@2ndquadrant.com writes:

 Even with the current approach of checking the stats after every
 isolated case it's sometimes takes quite a little more than a glance
 to verify correctness due to side-effects of rollback (ins/upd/del
 counters are still updated), and the way stats are affecting the dead
 tuples counter.

 Honestly I think pg_regress is not the right tool to test stat counter
 updates.  It kind-of works today, but only because we don't stress it
 too much.  If you want to create a new test framework for pgstats, and
 include some tests for truncate, be my guest.

OK, I think I have now all bases covered, though the updated patch is
not that pretty.

The problem is that we don't know in advance if the (sub)transaction is
going to succeed or abort, and in case of aborted truncate we need to
use the stats gathered prior to truncate.  Thus the need to track
insert/update/deletes that happened before first truncate separately.

To the point of making a dedicated pgstat testing tool: let's have
another TODO item?

--
Alex

From 0b3161191a3ddb999cd9d0da08e1b6088ce07a84 Mon Sep 17 00:00:00 2001
From: Alex Shulgin a...@commandprompt.com
Date: Tue, 9 Dec 2014 16:35:14 +0300
Subject: [PATCH] WIP: track TRUNCATEs in pgstat transaction stats.

The n_live_tup and n_dead_tup counters need to be set to 0 after a
TRUNCATE on the relation.  We can't issue a special message to the stats
collector because the xact might be later aborted, so we track the fact
that the relation was truncated during the xact (and reset this xact's
insert/update/delete counters).  When xact is committed, we use the
`truncated' flag to reset the n_live_tup and n_dead_tup counters.
---
 src/backend/commands/tablecmds.c |  3 +
 src/backend/postmaster/pgstat.c  | 98 ++--
 src/include/pgstat.h | 14 ++--
 src/test/regress/expected/prepared_xacts.out | 50 ++
 src/test/regress/expected/stats.out  | 63 ++
 src/test/regress/sql/prepared_xacts.sql  | 27 
 src/test/regress/sql/stats.sql   | 68 +++
 7 files changed, 315 insertions(+), 8 deletions(-)

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
new file mode 100644
index 1e737a0..4f0e3d8
*** a/src/backend/commands/tablecmds.c
--- b/src/backend/commands/tablecmds.c
***
*** 71,76 
--- 71,77 
  #include parser/parse_type.h
  #include parser/parse_utilcmd.h
  #include parser/parser.h
+ #include pgstat.h
  #include rewrite/rewriteDefine.h
  #include rewrite/rewriteHandler.h
  #include rewrite/rewriteManip.h
*** ExecuteTruncate(TruncateStmt *stmt)
*** 1224,1229 
--- 1225,1232 
  			 */
  			reindex_relation(heap_relid, REINDEX_REL_PROCESS_TOAST);
  		}
+ 
+ 		pgstat_count_heap_truncate(rel);
  	}
  
  	/*
diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
new file mode 100644
index f71fdeb..9d19cf9
*** a/src/backend/postmaster/pgstat.c
--- b/src/backend/postmaster/pgstat.c
*** typedef struct TwoPhasePgStatRecord
*** 199,206 
--- 199,210 
  	PgStat_Counter tuples_inserted;		/* tuples inserted in xact */
  	PgStat_Counter tuples_updated;		/* tuples updated in xact */
  	PgStat_Counter tuples_deleted;		/* tuples deleted in xact */
+ 	PgStat_Counter inserted_pre_trunc;	/* tuples inserted prior to truncate */
+ 	PgStat_Counter updated_pre_trunc;	/* tuples updated prior to truncate */
+ 	PgStat_Counter deleted_pre_trunc;	/* tuples deleted prior to truncate */
  	Oid			t_id;			/* table's OID */
  	bool		t_shared;		/* is it a shared catalog? */
+ 	bool		t_truncated;	/* was the relation truncated? */
  } TwoPhasePgStatRecord;
  
  /*
*** pgstat_count_heap_delete(Relation rel)
*** 1863,1868 
--- 1867,1919 
  	}
  }
  
+ static void
+ record_xact_truncate_stats(PgStat_TableXactStatus *trans)
+ {
+ 	if (!trans-truncated)
+ 	{
+ 		trans-inserted_pre_trunc = trans-tuples_inserted;
+ 		trans-updated_pre_trunc = trans-tuples_updated;
+ 		trans-deleted_pre_trunc = trans-tuples_deleted;
+ 	}
+ }
+ 
+ static void
+ restore_xact_truncate_stats(PgStat_TableXactStatus *trans)
+ {
+ 	if (trans-truncated)
+ 	{
+ 		trans-tuples_inserted = trans-inserted_pre_trunc;
+ 		trans-tuples_updated = trans-updated_pre_trunc;
+ 		trans-tuples_deleted = trans-deleted_pre_trunc;
+ 	}
+ }
+ 
+ /*
+  * pgstat_count_heap_truncate - update tuple counters due to truncate
+  */
+ void
+ pgstat_count_heap_truncate(Relation rel)
+ {
+ 	PgStat_TableStatus *pgstat_info = rel-pgstat_info;
+ 
+ 	if (pgstat_info != NULL)
+ 	{
+ 		/* We have to log the effect at the proper transactional level */
+ 		int			nest_level = GetCurrentTransactionNestLevel();
+ 
+ 		if (pgstat_info-trans == NULL ||
+ 			pgstat_info-trans-nest_level != nest_level)
+ 			add_tabstat_xact_level(pgstat_info, nest_level);
+ 
+ 		record_xact_truncate_stats(pgstat_info-trans

Re: [HACKERS] REVIEW: Track TRUNCATE via pgstat

[Alex Shulgin]

Alvaro Herrera alvhe...@2ndquadrant.com writes:

 Alex Shulgin wrote:

 OK, I think I have now all bases covered, though the updated patch is
 not that pretty.
 
 The problem is that we don't know in advance if the (sub)transaction is
 going to succeed or abort, and in case of aborted truncate we need to
 use the stats gathered prior to truncate.  Thus the need to track
 insert/update/deletes that happened before first truncate separately.

 Ugh, this is messy indeed.  I grant that TRUNCATE is a tricky case to
 handle correctly, so some complexity is expected.  Can you please
 explain in detail how this works?

The main idea is that aborted transaction can leave dead tuples behind
(that is every insert and update), but when TRUNCATE is issued we need
to reset insert/update/delete counters to 0: otherwise we won't get
accurate live and dead counts at commit time.

If the transaction that issued TRUNCATE is instead aborted, the
insert/update counters that we were incrementing *after* truncate are
not relevant to accurate calculation of dead tuples in the original
relfilenode we are now back to due to abort.  We need the insert/updates
counts that happened *before* the first TRUNCATE, hence the need for
separate counters.

 To the point of making a dedicated pgstat testing tool: let's have
 another TODO item?

 Sure.

Added one.

--
Alex


-- 
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] REVIEW: Track TRUNCATE via pgstat

[Alex Shulgin]

Jim Nasby jim.na...@bluetreble.com writes:

 https://commitfest.postgresql.org/action/patch_view?id=1661 (apologies for 
 not replying to the thread; I can't find it in my inbox)

 Patch applies and passes make check. Code formatting looks good.

Jim,

 The regression test partially tests this. It does not cover 2PC, nor
 does it test rolling back a subtransaction that contains a
 truncate. The latter actually doesn't work correctly.

Thanks for pointing out the missing 2PC test, I've added one.

The test you've added for rolling back a subxact actually works
correctly, if you consider the fact that aborted (sub)xacts still
account for insert/update/delete in pgstat.  I've added this test with
the corrected expected results.

 The test also adds 2.5 seconds of forced pg_sleep. I think that's both
 bad and unnecessary. When I removed the sleeps I still saw times of
 less than 0.1 seconds.

Well, I never liked that part, but the stats don't get updated if we
don't put the session to sleep for at least PGSTAT_STAT_INTERVAL (which
is 500ms).

Removing these extra sleep calls would theoretically not make a
difference as wait_for_trunc_test_stats() seems to have enough sleep
calls itself, but due to the pgstat_report_stat() being called from the
main loop only, there's no way short of placing the explicit pg_sleep at
top level, if we want to be able to check the effects reproducibly.

Another idea would be exposing pgstat_report_stat(true) at SQL level.
That would eleminate the need for explicit pg_sleep(=0.5), but we'll
still need the wait_for_... call to make sure the collector has picked
it up.

 Also, wait_for_trunc_test_stats() should display something if it times
 out; otherwise you'll have a test failure and won't have any
 indication why.

Oh, good catch.  Since I've copied this function from stats.sql, we
might want to update that one too in a separate commit.

 I've attached a patch that adds logging on timeout and contains a test
 case that demonstrates the rollback to savepoint bug.

I'm attaching the updated patch version.

Thank you for the review!
--
Alex

PS: re: your CF comment: I'm producing the patches using

  git format-patch --ext-diff

where diff.external is set to '/bin/bash src/tools/git-external-diff'.

Now that I try to apply it using git, looks like git doesn't like the
copied context diff very much...

From cc51823a01a194ef6fcd90bc763fa26498837322 Mon Sep 17 00:00:00 2001
From: Alex Shulgin a...@commandprompt.com
Date: Tue, 9 Dec 2014 16:35:14 +0300
Subject: [PATCH] WIP: track TRUNCATEs in pgstat transaction stats.

The n_live_tup and n_dead_tup counters need to be set to 0 after a
TRUNCATE on the relation.  We can't issue a special message to the stats
collector because the xact might be later aborted, so we track the fact
that the relation was truncated during the xact (and reset this xact's
insert/update/delete counters).  When xact is committed, we use the
`truncated' flag to reset the n_live_tup and n_dead_tup counters.
---
 src/backend/commands/tablecmds.c |   3 +
 src/backend/postmaster/pgstat.c  |  52 -
 src/include/pgstat.h |   3 +
 src/test/regress/expected/prepared_xacts.out |  50 
 src/test/regress/expected/truncate.out   | 164 +++
 src/test/regress/sql/prepared_xacts.sql  |  27 +
 src/test/regress/sql/truncate.sql| 118 +++
 7 files changed, 414 insertions(+), 3 deletions(-)

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
new file mode 100644
index 1e737a0..4f0e3d8
*** a/src/backend/commands/tablecmds.c
--- b/src/backend/commands/tablecmds.c
***
*** 71,76 
--- 71,77 
  #include parser/parse_type.h
  #include parser/parse_utilcmd.h
  #include parser/parser.h
+ #include pgstat.h
  #include rewrite/rewriteDefine.h
  #include rewrite/rewriteHandler.h
  #include rewrite/rewriteManip.h
*** ExecuteTruncate(TruncateStmt *stmt)
*** 1224,1229 
--- 1225,1232 
  			 */
  			reindex_relation(heap_relid, REINDEX_REL_PROCESS_TOAST);
  		}
+ 
+ 		pgstat_count_heap_truncate(rel);
  	}
  
  	/*
diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
new file mode 100644
index f71fdeb..b02e4a1
*** a/src/backend/postmaster/pgstat.c
--- b/src/backend/postmaster/pgstat.c
*** typedef struct TwoPhasePgStatRecord
*** 201,206 
--- 201,207 
  	PgStat_Counter tuples_deleted;		/* tuples deleted in xact */
  	Oid			t_id;			/* table's OID */
  	bool		t_shared;		/* is it a shared catalog? */
+ 	bool		t_truncated;	/* was the relation truncated? */
  } TwoPhasePgStatRecord;
  
  /*
*** pgstat_count_heap_delete(Relation rel)
*** 1864,1869 
--- 1865,1894 
  }
  
  /*
+  * pgstat_count_heap_truncate - update tuple counters due to truncate
+  */
+ void
+ pgstat_count_heap_truncate(Relation rel)
+ {
+ 	PgStat_TableStatus *pgstat_info = rel

Re: [HACKERS] REVIEW: Track TRUNCATE via pgstat

[Alex Shulgin]

Alvaro Herrera alvhe...@2ndquadrant.com writes:
 
 Another idea would be exposing pgstat_report_stat(true) at SQL level.
 That would eleminate the need for explicit pg_sleep(=0.5), but we'll
 still need the wait_for_... call to make sure the collector has picked
 it up.

 We already have a stats test that sleeps.  Why not add this stuff there,
 to avoid making another test slow?

Well, if we could come up with a set of statements to test that would
produce the end result unambigously, so that we can be certain the stats
we check at the end cannot be a result of neat interaction of buggy
behavior...

I'm not sure this is at all possible, but I know for sure it will make
debugging the possible fails harder.  Even with the current approach of
checking the stats after every isolated case it's sometimes takes quite
a little more than a glance to verify correctness due to side-effects of
rollback (ins/upd/del counters are still updated), and the way stats are
affecting the dead tuples counter.

I'll try to see if the checks can be converged though.

--
Alex


-- 
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] REVIEW: Track TRUNCATE via pgstat

[Alex Shulgin]

Alvaro Herrera alvhe...@2ndquadrant.com writes:

 Alex Shulgin wrote:

 Alvaro Herrera alvhe...@2ndquadrant.com writes:
  
  Another idea would be exposing pgstat_report_stat(true) at SQL level.
  That would eleminate the need for explicit pg_sleep(=0.5), but we'll
  still need the wait_for_... call to make sure the collector has picked
  it up.
 
  We already have a stats test that sleeps.  Why not add this stuff there,
  to avoid making another test slow?
 
 Well, if we could come up with a set of statements to test that would
 produce the end result unambigously, so that we can be certain the stats
 we check at the end cannot be a result of neat interaction of buggy
 behavior...

 It is always possible that things work just right because two bugs
 cancel each other.

 I'm not sure this is at all possible, but I know for sure it will make
 debugging the possible fails harder.

 Surely if some other patch introduces a failure, nobody will try to
 debug it by looking only at the failures of this test.

 Even with the current approach of checking the stats after every
 isolated case it's sometimes takes quite a little more than a glance
 to verify correctness due to side-effects of rollback (ins/upd/del
 counters are still updated), and the way stats are affecting the dead
 tuples counter.

 Honestly I think pg_regress is not the right tool to test stat counter
 updates.  It kind-of works today, but only because we don't stress it
 too much.  If you want to create a new test framework for pgstats, and
 include some tests for truncate, be my guest.

Yes, these are all good points.  Actually, moving the test to stats.sql
helped me realize the current patch behavior is not strictly correct:
there's a corner case when you insert/update before truncate in
transaction, which is later aborted.  I need to take a closer look.

--
Alex
 


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

Re: [HACKERS] [PATCH] add ssl_protocols configuration option

[Alex Shulgin]

Michael Paquier michael.paqu...@gmail.com writes:

 Perhaps ssloptions.[ch], unless you plan to add non-option-related code
 there later?

 I don't think anything else than common options-related code fits in
 there, so ssloptions.c makes sense to me.

 BTW, there is no Regent code in your openssl.c, so the copyright
 statement is incorrect.

 Good catch, I just blindly copied that from some other file.
 There have been arguments in favor and against this patch... But
 marking it as returned with feedback because of a lack of activity in
 the last couple of weeks. Feel free to update if you think that's not
 correct, and please move this patch to commit fest 2014-12.

Attached is a new version that addresses the earlier feedback: renamed
the added *.[ch] files and removed incorrect copyright line.

I'm moving this to the current CF.

--
Alex

From 18388300f9ed34fa47c66b8a2da098aeb19a7f71 Mon Sep 17 00:00:00 2001
From: Alex Shulgin a...@commandprompt.com
Date: Wed, 19 Nov 2014 19:49:29 +0300
Subject: [PATCH] DRAFT: ssl_protocols GUC

---
 doc/src/sgml/config.sgml  |  29 ++
 doc/src/sgml/libpq.sgml   |  25 ++
 src/backend/libpq/Makefile|   2 +-
 src/backend/libpq/be-secure-openssl.c |  29 --
 src/backend/libpq/be-secure.c |   3 +-
 src/backend/libpq/ssloptions.c| 123 ++
 src/backend/utils/misc/guc.c  |  15 
 src/backend/utils/misc/postgresql.conf.sample |   1 +
 src/include/libpq/libpq.h |   1 +
 src/include/libpq/ssloptions.h|  48 ++
 src/interfaces/libpq/Makefile |   8 +-
 src/interfaces/libpq/fe-connect.c |   4 +
 src/interfaces/libpq/fe-secure-openssl.c  |  18 +++-
 src/interfaces/libpq/libpq-int.h  |   1 +
 14 files changed, 297 insertions(+), 10 deletions(-)
 create mode 100644 src/backend/libpq/ssloptions.c
 create mode 100644 src/include/libpq/ssloptions.h

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
new file mode 100644
index 48ae3e4..699f0f9
*** a/doc/src/sgml/config.sgml
--- b/doc/src/sgml/config.sgml
*** include_dir 'conf.d'
*** 1055,1060 
--- 1055,1089 
/listitem
   /varlistentry
  
+  varlistentry id=guc-ssl-protocols xreflabel=ssl_protocols
+   termvarnamessl_protocols/varname (typestring/type)
+   indexterm
+primaryvarnamessl_protocols/ configuration parameter/primary
+   /indexterm
+   /term
+   listitem
+para
+ Specifies a colon-separated list of acronymSSL/ protocols that are
+ allowed to be used on secure connections. Each entry in the list can
+ be prefixed by a literal+/ (add to the current list)
+ or literal-/ (remove from the current list). If no prefix is used,
+ the list is replaced with the specified protocol.
+/para
+para
+ The full list of supported protocols can be found in the
+ the applicationopenssl/ manual page.  In addition to the names of
+ individual protocols, the following keywords can be
+ used: literalALL/ (all protocols supported by the underlying
+ crypto library), literalSSL/ (all supported versions
+ of acronymSSL/) and literalTLS/ (all supported versions
+ of acronymTLS/).
+/para
+para
+ The default is literalALL:-SSL/literal.
+/para
+   /listitem
+  /varlistentry
+ 
   varlistentry id=guc-ssl-ciphers xreflabel=ssl_ciphers
termvarnamessl_ciphers/varname (typestring/type)
indexterm
diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
new file mode 100644
index d829a4b..62ee0b4
*** a/doc/src/sgml/libpq.sgml
--- b/doc/src/sgml/libpq.sgml
*** postgresql://%2Fvar%2Flib%2Fpostgresql/d
*** 1230,1235 
--- 1230,1250 
/listitem
   /varlistentry
  
+  varlistentry id=libpq-connect-sslprotocols xreflabel=sslprotocols
+   termliteralsslprotocols/literal/term
+   listitem
+para
+ Specifies a colon-separated list of acronymSSL/ protocols that are
+ allowed to be used on secure connections.
+ See xref linkend=guc-ssl-protocols server configuration parameter
+ for format.
+/para
+para
+ Defaults to literalALL:-SSL/.
+/para
+   /listitem
+  /varlistentry
+ 
   varlistentry id=libpq-connect-sslcompression xreflabel=sslcompression
termliteralsslcompression/literal/term
listitem
*** myEventProc(PGEventId evtId, void *evtIn
*** 6693,6698 
--- 6708,6723 
   /para
  /listitem
  
+ listitem
+  para
+   indexterm
+primaryenvarPGSSLPROTOCOLS/envar/primary
+   /indexterm
+   envarPGSSLPROTOCOLS/envar behaves the same as the xref
+   linkend=libpq-connect-sslprotocols connection

Re: [HACKERS] [PATCH] HINT: pg_hba.conf changed since last config reload

[Alex Shulgin]

Peter Eisentraut pete...@gmx.net writes:

 On 10/16/14 11:34 PM, Craig Ringer wrote:
 psql: FATAL:  Peer authentication failed for user fred
 HINT:  See the server error log for additional information.

 I think this is wrong for many reasons.

 I have never seen an authentication system that responds with, hey, what
 you just did didn't get you in, but the administrators are currently in
 the process of making a configuration change, so why don't you check
 that out.

 We don't know whether the user has access to the server log.  They
 probably don't.  Also, it is vastly more likely that the user really
 doesn't have access in the way they chose, so throwing in irrelevant
 hints will be distracting.

 Moreover, it will be confusing to regular users if this message
 sometimes shows up and sometimes doesn't, independent of their own state
 and actions.

 Finally, the fact that a configuration change is in progress is
 privileged information.  Unprivileged users can deduct from the presence
 of this message that administrators are doing something, and possibly
 that they have done something wrong.

 I think it's fine to log a message in the server log if the pg_hba.conf
 file needs reloading.  But the client shouldn't know about this at all.

These are all valid concerns IMHO.

Attached is the modified version of the original patch by Craig,
addressing the handling of the new hint_log error data field and
removing the client-side HINT.

I'm also moving this to the current CF.

--
Alex

From 702e0ac31f3d8023ad8c064d90bdf5a8441fddea Mon Sep 17 00:00:00 2001
From: Craig Ringer cr...@2ndquadrant.com
Date: Fri, 17 Oct 2014 11:18:18 +0800
Subject: [PATCH 1/2] Add an errhint_log, akin to errdetail_log

This allows a different HINT to be sent to the server error log
and to the client, which will be useful where there's security
sensitive information that's more appropriate for a HINT than
a DETAIL message.
---
 src/backend/utils/error/elog.c | 59 --
 src/include/utils/elog.h   |  7 +
 2 files changed, 53 insertions(+), 13 deletions(-)

diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
new file mode 100644
index 2316464..da55207
*** a/src/backend/utils/error/elog.c
--- b/src/backend/utils/error/elog.c
*** errfinish(int dummy,...)
*** 503,508 
--- 503,510 
  		pfree(edata-detail_log);
  	if (edata-hint)
  		pfree(edata-hint);
+ 	if (edata-hint_log)
+ 		pfree(edata-hint_log);
  	if (edata-context)
  		pfree(edata-context);
  	if (edata-schema_name)
*** errhint(const char *fmt,...)
*** 1015,1020 
--- 1017,1042 
  	return 0;	/* return value does not matter */
  }
  
+ /*
+  * errhint_log --- add a hint_log error message text to the current error
+  */
+ int
+ errhint_log(const char *fmt,...)
+ {
+ 	ErrorData  *edata = errordata[errordata_stack_depth];
+ 	MemoryContext oldcontext;
+ 
+ 	recursion_depth++;
+ 	CHECK_STACK_DEPTH();
+ 	oldcontext = MemoryContextSwitchTo(edata-assoc_context);
+ 
+ 	EVALUATE_MESSAGE(edata-domain, hint_log, false, true);
+ 
+ 	MemoryContextSwitchTo(oldcontext);
+ 	recursion_depth--;
+ 	return 0;	/* return value does not matter */
+ }
+ 
  
  /*
   * errcontext_msg --- add a context error message text to the current error
*** CopyErrorData(void)
*** 1498,1503 
--- 1520,1527 
  		newedata-detail_log = pstrdup(newedata-detail_log);
  	if (newedata-hint)
  		newedata-hint = pstrdup(newedata-hint);
+ 	if (newedata-hint_log)
+ 		newedata-hint_log = pstrdup(newedata-hint_log);
  	if (newedata-context)
  		newedata-context = pstrdup(newedata-context);
  	if (newedata-schema_name)
*** FreeErrorData(ErrorData *edata)
*** 1536,1541 
--- 1560,1567 
  		pfree(edata-detail_log);
  	if (edata-hint)
  		pfree(edata-hint);
+ 	if (edata-hint_log)
+ 		pfree(edata-hint_log);
  	if (edata-context)
  		pfree(edata-context);
  	if (edata-schema_name)
*** ThrowErrorData(ErrorData *edata)
*** 1607,1612 
--- 1633,1640 
  		newedata-detail_log = pstrdup(edata-detail_log);
  	if (edata-hint)
  		newedata-hint = pstrdup(edata-hint);
+ 	if (edata-hint_log)
+ 		newedata-hint_log = pstrdup(edata-hint_log);
  	if (edata-context)
  		newedata-context = pstrdup(edata-context);
  	if (edata-schema_name)
*** ReThrowError(ErrorData *edata)
*** 1669,1674 
--- 1697,1704 
  		newedata-detail_log = pstrdup(newedata-detail_log);
  	if (newedata-hint)
  		newedata-hint = pstrdup(newedata-hint);
+ 	if (newedata-hint_log)
+ 		newedata-hint_log = pstrdup(newedata-hint_log);
  	if (newedata-context)
  		newedata-context = pstrdup(newedata-context);
  	if (newedata-schema_name)
*** write_csvlog(ErrorData *edata)
*** 2710,2717 
  		appendCSVLiteral(buf, edata-detail);
  	appendStringInfoChar(buf, ',');
  
! 	/* errhint */
! 	appendCSVLiteral(buf, edata-hint);
  	appendStringInfoChar(buf, ',');
  
  	/* internal query */
--- 

Re: [HACKERS] Turning recovery.conf into GUCs

[Alex Shulgin]

Alex Shulgin a...@commandprompt.com writes:

 Alex Shulgin a...@commandprompt.com writes:

 Here's an attempt to revive this patch.

 Here's the patch rebased against current HEAD, that is including the
 recently committed action_at_recovery_target option.

 The default for the new GUC is 'pause', as in HEAD, and
 pause_at_recovery_target is removed completely in favor of it.

 I've also taken the liberty to remove that part that errors out when
 finding $PGDATA/recovery.conf.  Now get your rotten tomatoes ready. ;-)

This was rather short-sighted, so I've restored that part.

Also, rebased on current HEAD, following the rename of
action_at_recovery_target to recovery_target_action.

--
Alex



recovery_guc_v5.5.patch.gz
Description: application/gzip

-- 
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] SSL information view

[Alex Shulgin]

Magnus Hagander mag...@hagander.net writes:

 You should add it to the next CF for proper tracking, there are already many
 patches in the queue waiting for reviews :)

 Absolutely - I just wanted those that were already involved in the
 thread to get a chance to look at it early :) didn't want to submit it
 until it was complete.

 Which it is now - attached is a new version that includes docs.

Here's my review of the patch:

* Applies to the current HEAD, no failed hunks.
* Compiles and works as advertised.
* Docs included.


The following catches my eye:

psql (9.5devel)
SSL connection (protocol: TLSv1.2, cipher: ECDHE-RSA-AES256-GCM-SHA384, bits: 
256, compression: off)
Type help for help.

postgres=# select * from pg_stat_ssl;
 pid  | ssl | bits | compression | version |   cipher| 
clientdn 
--+-+--+-+-+-+--
 1343 | t   |  256 | f   | TLSv1.2 | ECDHE-RSA-AES256-GCM-SHA384 | 
(1 row)

I think the order of details in the psql prompt makes more sense,
because it puts more important details first.

I suggest we reorder the view columns, while also renaming 'version' to
'protocol' (especially since we have another patch in the works that
adds a GUC named 'ssl_protocols'):

  pid, ssl, protocol, cipher, bits, compression, clientdn.


Next, this one:

+ be_tls_get_cipher(Port *port, char *ptr, size_t len)
+ {
+   if (port-ssl)
+   strlcpy(ptr, SSL_get_cipher(port-ssl), NAMEDATALEN);

should be this:

+   strlcpy(ptr, SSL_get_cipher(port-ssl), len);

The same goes for this one:

+ be_tls_get_peerdn_name(Port *port, char *ptr, size_t len)
+ {
+   if (port-peer)
+   strlcpy(ptr, 
X509_NAME_to_cstring(X509_get_subject_name(port-peer)), NAMEDATALEN);

The NAMEDATALEN constant is passed in the calling code in
pgstat_bestart().


Other than that, looks good to me.

--
Alex


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

Re: [HACKERS] Small TRUNCATE glitch

[Alex Shulgin]

Bruce Momjian br...@momjian.us writes:

 On Wed, Dec 10, 2014 at 10:32:42AM +0200, Heikki Linnakangas wrote:
 I don't think we need to have 2PC files survive a pg_upgrade.  It seems
 perfectly okay to remove them from the new cluster at some appropriate
 time, *if* they are copied from the old cluster at all (I don't think
 they should be.)
 
 I think pg_upgrade should check if there are any prepared
 transactions pending, and refuse to upgrade if there are. It could
 be made to work, but it's really not worth the trouble. If there are
 any pending prepared transactions in the system when you run
 pg_upgrade, it's more likely to be a mistake or oversight in the
 first place, than on purpose.

 pg_upgrade already checks for prepared transactions and errors out if
 they exist;  see check_for_prepared_transactions().

Alright, that's good to know.  So I'm just adding a new bool field to
the 2PC pgstat record instead of the bit magic.

Attached is v0.2, now with a regression test included.

--
Alex

From 4c8fae27ecd9c94e7c3da0997f03099045a152d9 Mon Sep 17 00:00:00 2001
From: Alex Shulgin a...@commandprompt.com
Date: Tue, 9 Dec 2014 16:35:14 +0300
Subject: [PATCH] WIP: track TRUNCATEs in pgstat transaction stats.

The n_live_tup and n_dead_tup counters need to be set to 0 after a
TRUNCATE on the relation.  We can't issue a special message to the stats
collector because the xact might be later aborted, so we track the fact
that the relation was truncated during the xact (and reset this xact's
insert/update/delete counters).  When xact is committed, we use the
`truncated' flag to reset the n_live_tup and n_dead_tup counters.
---
 src/backend/commands/tablecmds.c   |   2 +
 src/backend/postmaster/pgstat.c|  52 -
 src/include/pgstat.h   |   3 +
 src/test/regress/expected/truncate.out | 136 +
 src/test/regress/sql/truncate.sql  |  98 
 5 files changed, 288 insertions(+), 3 deletions(-)

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
new file mode 100644
index 1e737a0..192d033
*** a/src/backend/commands/tablecmds.c
--- b/src/backend/commands/tablecmds.c
*** ExecuteTruncate(TruncateStmt *stmt)
*** 1224,1229 
--- 1224,1231 
  			 */
  			reindex_relation(heap_relid, REINDEX_REL_PROCESS_TOAST);
  		}
+ 
+ 		pgstat_count_heap_truncate(rel);
  	}
  
  	/*
diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
new file mode 100644
index c7f41a5..88c83d2
*** a/src/backend/postmaster/pgstat.c
--- b/src/backend/postmaster/pgstat.c
*** typedef struct TwoPhasePgStatRecord
*** 201,206 
--- 201,207 
  	PgStat_Counter tuples_deleted;		/* tuples deleted in xact */
  	Oid			t_id;			/* table's OID */
  	bool		t_shared;		/* is it a shared catalog? */
+ 	bool		t_truncated;	/* was the relation truncated? */
  } TwoPhasePgStatRecord;
  
  /*
*** pgstat_count_heap_delete(Relation rel)
*** 1864,1869 
--- 1865,1894 
  }
  
  /*
+  * pgstat_count_heap_truncate - update tuple counters due to truncate
+  */
+ void
+ pgstat_count_heap_truncate(Relation rel)
+ {
+ 	PgStat_TableStatus *pgstat_info = rel-pgstat_info;
+ 
+ 	if (pgstat_info != NULL)
+ 	{
+ 		/* We have to log the effect at the proper transactional level */
+ 		int			nest_level = GetCurrentTransactionNestLevel();
+ 
+ 		if (pgstat_info-trans == NULL ||
+ 			pgstat_info-trans-nest_level != nest_level)
+ 			add_tabstat_xact_level(pgstat_info, nest_level);
+ 
+ 		pgstat_info-trans-tuples_inserted = 0;
+ 		pgstat_info-trans-tuples_updated = 0;
+ 		pgstat_info-trans-tuples_deleted = 0;
+ 		pgstat_info-trans-truncated = true;
+ 	}
+ }
+ 
+ /*
   * pgstat_update_heap_dead_tuples - update dead-tuples count
   *
   * The semantics of this are that we are reporting the nontransactional
*** AtEOXact_PgStat(bool isCommit)
*** 1927,1932 
--- 1952,1959 
  			tabstat-t_counts.t_tuples_deleted += trans-tuples_deleted;
  			if (isCommit)
  			{
+ tabstat-t_counts.t_truncated = trans-truncated;
+ 
  /* insert adds a live tuple, delete removes one */
  tabstat-t_counts.t_delta_live_tuples +=
  	trans-tuples_inserted - trans-tuples_deleted;
*** AtEOSubXact_PgStat(bool isCommit, int ne
*** 1991,1999 
  			{
  if (trans-upper  trans-upper-nest_level == nestDepth - 1)
  {
! 	trans-upper-tuples_inserted += trans-tuples_inserted;
! 	trans-upper-tuples_updated += trans-tuples_updated;
! 	trans-upper-tuples_deleted += trans-tuples_deleted;
  	tabstat-trans = trans-upper;
  	pfree(trans);
  }
--- 2018,2037 
  			{
  if (trans-upper  trans-upper-nest_level == nestDepth - 1)
  {
! 	if (trans-truncated)
! 	{
! 		trans-upper-truncated = true;
! 		/* replace upper xact stats with ours */
! 		trans-upper-tuples_inserted = trans-tuples_inserted;
! 		trans-upper-tuples_updated = trans

Re: [HACKERS] Small TRUNCATE glitch

[Alex Shulgin]

Bruce Momjian br...@momjian.us writes:

 Added to TODO:

 o Clear table counters on TRUNCATE

   http://archives.postgresql.org/pgsql-hackers/2008-04/msg00169.php

Hello,

Attached is a WIP patch for this TODO.

From 97665ef1ca7d1847e90d4dfab38562135f01fb2b Mon Sep 17 00:00:00 2001
From: Alex Shulgin a...@commandprompt.com
Date: Tue, 9 Dec 2014 16:35:14 +0300
Subject: [PATCH] WIP: track TRUNCATEs in pgstat transaction stats.

The n_live_tup and n_dead_tup counters need to be set to 0 after a
TRUNCATE on the relation.  We can't issue a special message to the stats
collector because the xact might be later aborted, so we track the fact
that the relation was truncated during the xact (and reset this xact's
insert/update/delete counters).  When xact is committed, we use the
`truncated' flag to reset the n_live_tup and n_dead_tup counters.
---
 src/backend/commands/tablecmds.c |  2 ++
 src/backend/postmaster/pgstat.c  | 70 
 src/include/pgstat.h |  3 ++
 3 files changed, 68 insertions(+), 7 deletions(-)

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
new file mode 100644
index 1e737a0..192d033
*** a/src/backend/commands/tablecmds.c
--- b/src/backend/commands/tablecmds.c
*** ExecuteTruncate(TruncateStmt *stmt)
*** 1224,1229 
--- 1224,1231 
  			 */
  			reindex_relation(heap_relid, REINDEX_REL_PROCESS_TOAST);
  		}
+ 
+ 		pgstat_count_heap_truncate(rel);
  	}
  
  	/*
diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
new file mode 100644
index c7f41a5..7ff66b5
*** a/src/backend/postmaster/pgstat.c
--- b/src/backend/postmaster/pgstat.c
*** typedef struct TwoPhasePgStatRecord
*** 200,208 
  	PgStat_Counter tuples_updated;		/* tuples updated in xact */
  	PgStat_Counter tuples_deleted;		/* tuples deleted in xact */
  	Oid			t_id;			/* table's OID */
! 	bool		t_shared;		/* is it a shared catalog? */
  } TwoPhasePgStatRecord;
  
  /*
   * Info about current snapshot of stats file
   */
--- 200,211 
  	PgStat_Counter tuples_updated;		/* tuples updated in xact */
  	PgStat_Counter tuples_deleted;		/* tuples deleted in xact */
  	Oid			t_id;			/* table's OID */
! 	char		t_flags;		/* see TWOPHASE_PGSTAT_RECORD_*_FLAGs */
  } TwoPhasePgStatRecord;
  
+ #define TWOPHASE_PGSTAT_RECORD_SHARED_FLAG	0x01	/* is it a shared catalog? */
+ #define TWOPHASE_PGSTAT_RECORD_TRUNC_FLAG	0x02	/* was the relation truncated? */
+ 
  /*
   * Info about current snapshot of stats file
   */
*** pgstat_count_heap_delete(Relation rel)
*** 1864,1869 
--- 1867,1896 
  }
  
  /*
+  * pgstat_count_heap_truncate - update tuple counters due to truncate
+  */
+ void
+ pgstat_count_heap_truncate(Relation rel)
+ {
+ 	PgStat_TableStatus *pgstat_info = rel-pgstat_info;
+ 
+ 	if (pgstat_info != NULL)
+ 	{
+ 		/* We have to log the effect at the proper transactional level */
+ 		int			nest_level = GetCurrentTransactionNestLevel();
+ 
+ 		if (pgstat_info-trans == NULL ||
+ 			pgstat_info-trans-nest_level != nest_level)
+ 			add_tabstat_xact_level(pgstat_info, nest_level);
+ 
+ 		pgstat_info-trans-tuples_inserted = 0;
+ 		pgstat_info-trans-tuples_updated = 0;
+ 		pgstat_info-trans-tuples_deleted = 0;
+ 		pgstat_info-trans-truncated = true;
+ 	}
+ }
+ 
+ /*
   * pgstat_update_heap_dead_tuples - update dead-tuples count
   *
   * The semantics of this are that we are reporting the nontransactional
*** AtEOXact_PgStat(bool isCommit)
*** 1927,1932 
--- 1954,1961 
  			tabstat-t_counts.t_tuples_deleted += trans-tuples_deleted;
  			if (isCommit)
  			{
+ tabstat-t_counts.t_truncated = trans-truncated;
+ 
  /* insert adds a live tuple, delete removes one */
  tabstat-t_counts.t_delta_live_tuples +=
  	trans-tuples_inserted - trans-tuples_deleted;
*** AtEOSubXact_PgStat(bool isCommit, int ne
*** 1991,1999 
  			{
  if (trans-upper  trans-upper-nest_level == nestDepth - 1)
  {
! 	trans-upper-tuples_inserted += trans-tuples_inserted;
! 	trans-upper-tuples_updated += trans-tuples_updated;
! 	trans-upper-tuples_deleted += trans-tuples_deleted;
  	tabstat-trans = trans-upper;
  	pfree(trans);
  }
--- 2020,2039 
  			{
  if (trans-upper  trans-upper-nest_level == nestDepth - 1)
  {
! 	if (trans-truncated)
! 	{
! 		trans-upper-truncated = true;
! 		/* replace upper xact stats with ours */
! 		trans-upper-tuples_inserted = trans-tuples_inserted;
! 		trans-upper-tuples_updated = trans-tuples_updated;
! 		trans-upper-tuples_deleted = trans-tuples_deleted;
! 	}
! 	else
! 	{
! 		trans-upper-tuples_inserted += trans-tuples_inserted;
! 		trans-upper-tuples_updated += trans-tuples_updated;
! 		trans-upper-tuples_deleted += trans-tuples_deleted;
! 	}
  	tabstat-trans = trans-upper;
  	pfree(trans

Re: [HACKERS] Small TRUNCATE glitch

[Alex Shulgin]

Alex Shulgin a...@commandprompt.com writes:

 Bruce Momjian br...@momjian.us writes:

 Added to TODO:

 o Clear table counters on TRUNCATE

   http://archives.postgresql.org/pgsql-hackers/2008-04/msg00169.php

 Hello,

 Attached is a WIP patch for this TODO.

This part went as an attachment, which wasn't my intent:


It does the trick by tracking if a TRUNCATE command was issued under a
(sub)transaction and uses this knowledge to reset the live/dead tuple
counters later if the transaction was committed.  Testing in simple
cases shows that this clears the counters correctly, including use of
savepoints.

The 2PC part requires extending bool flag to fit the trunc flag, is this
approach sane?  Given that 2PC transaction should survive server
restart, it's reasonable to expect it to also survive the upgrade, so I
see no clean way of adding another bool field to the
TwoPhasePgStatRecord struct (unless some would like to add checks on
record length, etc.).

I'm going to add some regression tests, but not sure what would be the
best location for this.  The truncate.sql seems like natural choice, but
stats are not updating realtime, so I'd need to borrow some tricks from
stats.sql or better put the new tests in the stats.sql itself?

--
Alex


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

Re: [HACKERS] [COMMITTERS] pgsql: Keep track of transaction commit timestamps

[Alex Shulgin]

Heikki Linnakangas hlinnakan...@vmware.com writes:

 On 12/03/2014 04:54 PM, Alvaro Herrera wrote:
 ir commit timestamp directly as they commit,
 or an external transaction c

 Sorry, I'm late to the party, but here's some random comments on this
 after a quick review:

Also this commit breaks initdb of `make check' for me:

creating template1 database in 
/home/ash/build/postgresql/HEAD/src/test/regress/./tmp_check/data/base/1 ... 
TRAP: FailedAssertion(!(((xmax) = ((TransactionId) 3))), File: 
/home/ash/src/postgresql/src/backend/storage/ipc/procarray.c, Line: 1414)
Aborted (core dumped)
child process exited with exit code 134

--
Alex


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

Re: [HACKERS] [COMMITTERS] pgsql: Keep track of transaction commit timestamps

[Alex Shulgin]

Alvaro Herrera alvhe...@2ndquadrant.com writes:

 Alex Shulgin wrote:

 Also this commit breaks initdb of `make check' for me:
 
 creating template1 database in
 /home/ash/build/postgresql/HEAD/src/test/regress/./tmp_check/data/base/1
 ... TRAP: FailedAssertion(!(((xmax) = ((TransactionId) 3))),
 File:
 /home/ash/src/postgresql/src/backend/storage/ipc/procarray.c,
 Line: 1414)
 Aborted (core dumped)
 child process exited with exit code 134

 Uh, that's odd.  Can you please get a stack trace?  Do you have unusual
 settings or a patched build?

Not really, and I would mention that.  Will get a trace.

--
Alex


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

Re: [HACKERS] [COMMITTERS] pgsql: Keep track of transaction commit timestamps

[Alex Shulgin]

Alvaro Herrera alvhe...@2ndquadrant.com writes:

 Uh, that's odd.  Can you please get a stack trace?  Do you have unusual
 settings or a patched build?

Is there a way to pause the bootstrap process so I can attach gdb to it?

--
Alex


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

Re: [HACKERS] [COMMITTERS] pgsql: Keep track of transaction commit timestamps

[Alex Shulgin]

Craig Ringer cr...@2ndquadrant.com writes:

 On 12/04/2014 10:50 PM, Alex Shulgin wrote:
 Is there a way to pause the bootstrap process so I can attach gdb to it?

 With a newer gdb, you can instead tell gdb to follow all forks. I wrote
 some notes on it recently.

 http://blog.2ndquadrant.com/processes-breakpoints-watchpoints-postgresql/

 I've found it really handy when debugging crashes in regression tests.

 However, it's often simpler to just:

 ulimit -c unlimited
 make check

 (or whatever your crashing test is) then look at the core files that are
 produced.

Good one, it didn't occur to me that assert makes core files.

Figured it out with a pg_usleep in bootstrap.c anyway.  Does this look sane?


DEBUG:  inserting column 6 value 0
DEBUG:  inserted - 0
DEBUG:  inserting column 7 value varchar_transform
TRAP: FailedAssertion(!(((xmax) = ((TransactionId) 3))), File: 
/home/ash/src/postgresql/src/backend/storage/ipc/procarray.c, Line: 1414)

Program received signal SIGABRT, Aborted.
0x7f2757128d27 in __GI_raise (sig=sig@entry=6)
at ../nptl/sysdeps/unix/sysv/linux/raise.c:56
56  ../nptl/sysdeps/unix/sysv/linux/raise.c: No such file or directory.
(gdb) bt
#0  0x7f2757128d27 in __GI_raise (sig=sig@entry=6)
at ../nptl/sysdeps/unix/sysv/linux/raise.c:56
#1  0x7f275712a418 in __GI_abort () at abort.c:89
#2  0x009088b2 in ExceptionalCondition (
conditionName=0xac0710 !(((xmax) = ((TransactionId) 3))), 
errorType=0xac01d8 FailedAssertion, 
fileName=0xac0178 
/home/ash/src/postgresql/src/backend/storage/ipc/procarray.c, lineNumber=1414)
at /home/ash/src/postgresql/src/backend/utils/error/assert.c:54
#3  0x0079e125 in GetSnapshotData (
snapshot=0xdb2d60 CatalogSnapshotData)
at /home/ash/src/postgresql/src/backend/storage/ipc/procarray.c:1414
#4  0x0094e02d in GetNonHistoricCatalogSnapshot (relid=1255)
at /home/ash/src/postgresql/src/backend/utils/time/snapmgr.c:298
#5  0x0094dfdd in GetCatalogSnapshot (relid=1255)
at /home/ash/src/postgresql/src/backend/utils/time/snapmgr.c:272
#6  0x004c8f0d in systable_beginscan (heapRelation=0x1d0e5c0, 
indexId=2691, indexOK=1 '\001', snapshot=0x0, nkeys=1, key=0x7fff201bbc40)
at /home/ash/src/postgresql/src/backend/access/index/genam.c:275
#7  0x00885070 in regprocin (fcinfo=0x7fff201bbce0)
at /home/ash/src/postgresql/src/backend/utils/adt/regproc.c:106
#8  0x00914fe7 in InputFunctionCall (flinfo=0x7fff201bc0c0, 
str=0x1d349b8 varchar_transform, typioparam=24, typmod=-1)
---Type return to continue, or q return to quit---
at /home/ash/src/postgresql/src/backend/utils/fmgr/fmgr.c:1914
#9  0x0091533e in OidInputFunctionCall (functionId=44, 
str=0x1d349b8 varchar_transform, typioparam=24, typmod=-1)
at /home/ash/src/postgresql/src/backend/utils/fmgr/fmgr.c:2045
#10 0x0052af91 in InsertOneValue (value=0x1d349b8 varchar_transform, 
i=7) at /home/ash/src/postgresql/src/backend/bootstrap/bootstrap.c:840
#11 0x00527409 in boot_yyparse ()
at /home/ash/src/postgresql/src/backend/bootstrap/bootparse.y:455
#12 0x0052a26b in BootstrapModeMain ()
at /home/ash/src/postgresql/src/backend/bootstrap/bootstrap.c:494
#13 0x0052a177 in AuxiliaryProcessMain (argc=6, argv=0x1cc8378)
at /home/ash/src/postgresql/src/backend/bootstrap/bootstrap.c:414
#14 0x006a327c in main (argc=7, argv=0x1cc8370)
at /home/ash/src/postgresql/src/backend/main/main.c:212
(gdb)


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

Re: [HACKERS] [COMMITTERS] pgsql: Keep track of transaction commit timestamps

[Alex Shulgin]

Alvaro Herrera alvhe...@2ndquadrant.com writes:

 Alex Shulgin wrote:

 DEBUG:  inserting column 7 value varchar_transform
 
 Breakpoint 1, GetSnapshotData (snapshot=0xdb2d60 CatalogSnapshotData)
 at /home/ash/src/postgresql/src/backend/storage/ipc/procarray.c:1413
 1413 xmax = ShmemVariableCache-latestCompletedXid;
 
 (gdb) p ShmemVariableCache-latestCompletedXid 
 $1 = 4294967295

 I think you might need to make distclean, then recompile.  If you're
 not passing --enable-depend to configure, I suggest you do so; that
 greatly reduces such problems.

Well I tried that before posting the complaint, let me try again though.

--
Alex


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

Re: [HACKERS] [COMMITTERS] pgsql: Keep track of transaction commit timestamps

[Alex Shulgin]

Alex Shulgin a...@commandprompt.com writes:

 Figured it out with a pg_usleep in bootstrap.c anyway.  Does this look sane?


 DEBUG:  inserting column 6 value 0
 DEBUG:  inserted - 0
 DEBUG:  inserting column 7 value varchar_transform
 TRAP: FailedAssertion(!(((xmax) = ((TransactionId) 3))), File: 
 /home/ash/src/postgresql/src/backend/storage/ipc/procarray.c, Line: 1414)

I've tried to debug this and I feel really dumbfound...


DEBUG:  inserting column 7 value varchar_transform

Breakpoint 1, GetSnapshotData (snapshot=0xdb2d60 CatalogSnapshotData)
at /home/ash/src/postgresql/src/backend/storage/ipc/procarray.c:1413
1413xmax = ShmemVariableCache-latestCompletedXid;

(gdb) p ShmemVariableCache-latestCompletedXid 
$1 = 4294967295

(gdb) p *ShmemVariableCache
$2 = {nextOid = 1, oidCount = 0, nextXid = 3, oldestXid = 3, 
  xidVacLimit = 20003, xidWarnLimit = 2136483650, 
  xidStopLimit = 2146483650, xidWrapLimit = 2147483650, oldestXidDB = 1, 
  oldestCommitTs = 1, newestCommitTs = 0, latestCompletedXid = 4294967295}

(gdb) p xmax
$3 = 0

(gdb) n
1414Assert(TransactionIdIsNormal(xmax));

(gdb) p xmax
$4 = 1

(gdb) p *ShmemVariableCache
$5 = {nextOid = 1, oidCount = 0, nextXid = 3, oldestXid = 3, 
  xidVacLimit = 20003, xidWarnLimit = 2136483650, 
  xidStopLimit = 2146483650, xidWrapLimit = 2147483650, oldestXidDB = 1, 
  oldestCommitTs = 1, newestCommitTs = 0, latestCompletedXid = 4294967295}

(gdb) p ShmemVariableCache-latestCompletedXid 
$6 = 4294967295

(gdb) 


How?  Is there an another concurrent process with the old view of
VariableCacheData struct where latestCompletedXid still points to
oldestCommitTs?

This only happens with the CommitTs commit in effect.

--
Alex


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

Re: [HACKERS] [COMMITTERS] pgsql: Keep track of transaction commit timestamps

[Alex Shulgin]

Alvaro Herrera alvhe...@2ndquadrant.com writes:

 Alex Shulgin wrote:

 DEBUG:  inserting column 7 value varchar_transform
 
 Breakpoint 1, GetSnapshotData (snapshot=0xdb2d60 CatalogSnapshotData)
 at /home/ash/src/postgresql/src/backend/storage/ipc/procarray.c:1413
 1413 xmax = ShmemVariableCache-latestCompletedXid;
 
 (gdb) p ShmemVariableCache-latestCompletedXid 
 $1 = 4294967295

 I think you might need to make distclean, then recompile.  If you're
 not passing --enable-depend to configure, I suggest you do so; that
 greatly reduces such problems.

Yeah, that did it.  Sorry for the noise, I must have messed it up with
the recompilations (note to self to always use --enable-depend).

--
Alex


-- 
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] Turning recovery.conf into GUCs

[Alex Shulgin]

Michael Paquier michael.paqu...@gmail.com writes:

 On Tue, Dec 2, 2014 at 1:58 AM, Alex Shulgin a...@commandprompt.com wrote:
 Here's the patch rebased against current HEAD, that is including the
 recently committed action_at_recovery_target option.
 If this patch gets in, it gives a good argument to jump to 10.0 IMO.
 That's not a bad thing, only the cost of making recovery params as
 GUCs which is still a feature wanted.

 The default for the new GUC is 'pause', as in HEAD, and
 pause_at_recovery_target is removed completely in favor of it.
 Makes sense. Another idea that popped out was to rename this parameter
 as recovery_target_action as well, but that's not really something
 this patch should care about.

Indeed, but changing the name after the fact is straightforward.

 I've also taken the liberty to remove that part that errors out when
 finding $PGDATA/recovery.conf.
 I am not in favor of this part. It may be better to let the users know
 that their old configuration is not valid anymore with an error. This
 patch cuts in the flesh with a huge axe, let's be sure that users do
 not ignore the side pain effects, or recovery.conf would be simply
 ignored and users would not be aware of that.

Yeah, that is good point.

I'd be in favor of a solution that works the same way as before the
patch, without the need for extra trigger files, etc., but that doesn't
seem to be nearly possible.  Whatever tricks we might employ will likely
be defeated by the fact that the oldschool user will fail to *include*
recovery.conf in the main conf file.

--
Alex


-- 
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] Turning recovery.conf into GUCs

[Alex Shulgin]

Andres Freund and...@2ndquadrant.com writes:

 On 2014-12-02 17:25:14 +0300, Alex Shulgin wrote:
 I'd be in favor of a solution that works the same way as before the
 patch, without the need for extra trigger files, etc., but that doesn't
 seem to be nearly possible.  Whatever tricks we might employ will likely
 be defeated by the fact that the oldschool user will fail to *include*
 recovery.conf in the main conf file.

 I think removing the ability to define a trigger file is pretty much
 unacceptable. It's not *too* bad to rewrite recovery.conf's contents
 into actual valid postgresql.conf format, but it's harder to change an
 existing complex failover setup that relies on the existance of such a
 trigger.  I think aiming for removal of that is a sure way to prevent
 the patch from getting in.

To make it clear, I was talking not about trigger_file, but about
standby.enabled file which triggers the recovery/pitr/standby scenario
in the current form of the patch and stands as a replacement check
instead of the original check for the presense of recovery.conf.

--
Alex


-- 
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] Turning recovery.conf into GUCs

[Alex Shulgin]

Alex Shulgin a...@commandprompt.com writes:

 Here's an attempt to revive this patch.

Here's the patch rebased against current HEAD, that is including the
recently committed action_at_recovery_target option.

The default for the new GUC is 'pause', as in HEAD, and
pause_at_recovery_target is removed completely in favor of it.

I've also taken the liberty to remove that part that errors out when
finding $PGDATA/recovery.conf.  Now get your rotten tomatoes ready. ;-)

--
Alex



recovery_guc_v5.4.patch.gz
Description: application/gzip

-- 
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 : Allow toast tables to be moved to a different tablespace

[Alex Shulgin]

Julien Tachoires jul...@gmail.com writes:

 2011/12/15 Alvaro Herrera alvhe...@commandprompt.com:

 Uhm, surely you could compare the original toast tablespace to the heap
 tablespace, and if they differ, handle appropriately when creating the
 new toast table? =A0Just pass down the toast tablespace into
 AlterTableCreateToastTable, instead of having it assume that
 rel-rd_rel-relnamespace is sufficient. =A0This should be done in all
 cases where a toast tablespace is created, which shouldn't be more than
 a handful of them.

 Thank you, that way seems right.
 Now, I distinguish before each creation of a TOAST table with
 AlterTableCreateToastTable() : if it will create a new one or recreate
 an existing one.
 Thus, in create_toast_table() when toastTableSpace is equal to
 InvalidOid, we are able :
 - to fallback to the main table tablespace in case of new TOAST table creat=
 ion
 - to keep it previous tablespace in case of recreation.

 Here's a new version rebased against HEAD.

Almost 3 years later, here's a version rebased against current HEAD. :-)

It compiles and even passes the included regression test.

There were doubts originally if this is actually a useful feature, but
there is at least one plausible use case (main table on SSD, TOAST on
HDD):

  http://www.postgresql.org/message-id/4f3267ee.80...@deltasoft.no

I didn't add anything on top of the latest version, but I notice we've
added mat. views since then, so it looks like we now also need this:

  ALTER MATERIALIZED VIEW SET [VIEW | TOAST] TABLESPACE


Should I add this patch to the next CommitFest?

--
Alex



set_toast_tablespace_v0.11.patch.gz
Description: application/gzip

-- 
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 shutdown_at_recovery_target option to recovery.conf

[Alex Shulgin]

Christoph Berg c...@df7cb.de writes:

 Re: Petr Jelinek 2014-11-25 5474efea.2040...@2ndquadrant.com
 Patch committed.

Before I go and rebase that recovery.conf - GUC patch on top of
this...  is it final?

 
 Thanks!

 I'm a bit late to the party, but wouldn't

 recovery_target_action = ...

 have been a better name for this? It'd be in line with the other
 recovery_target_* parameters, and also a bit shorter than the imho
 somewhat ugly action_at_recovery_target.

FWIW, I too think that recovery_target_action is a better name.

--
Alex


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

Re: [HACKERS] [PATCH] add ssl_protocols configuration option

[Alex Shulgin]

Dag-Erling Smørgrav d...@des.no writes:

 Alex Shulgin a...@commandprompt.com writes:
 OK, looks like I've come up with something workable: I've added
 sslprotocol connection string keyword similar to pre-existing
 sslcompression, etc.  Please see attached v2 of the original patch.
 I'm having doubts about the name of openssl.h header though,
 libpq-openssl.h?

 Perhaps ssloptions.[ch], unless you plan to add non-option-related code
 there later?

I don't think anything else than common options-related code fits in
there, so ssloptions.c makes sense to me.

 BTW, there is no Regent code in your openssl.c, so the copyright
 statement is incorrect.

Good catch, I just blindly copied that from some other file.

--
Alex


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

[HACKERS] Follow up to irc on CREATE INDEX vs. maintenance_work_mem on 9.3

[Alex Shulgin]

Tom,

First of all, thanks for your help on IRC last time with that CREATE
INDEX memory consumption problem.

As has been pointed out in a stackexchange answer to my question[1], it
is indeed the limitation of pre-9.4 versions, but the limit is imposed
on memtuples array, rather than total memory the sort in CREATE INDEX
may allocate.  The memtuples won't grow further than MaxAllocSize and
I've got 24x50x10^6 = 1200MB, which just doesn't fit.

We've got a customer who is testing a migration to PostgreSQL-9.3 (from
$some_other_db), thus they load the tables first (some of their tables
have 10-100 million rows), then create the indexes and they constantly
see disk sort being used despite lots of available RAM and
maintenance_work_mem set to increasingly higher values.

Now my question, is it feasible to back-patch this to 9.3?  Or should we
tell the customer to wait before 9.4 is released?

Thanks.
--
Alex

[1] 
http://dba.stackexchange.com/questions/83600/postgresql-create-index-memory-requirement


-- 
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] Follow up to irc on CREATE INDEX vs. maintenance_work_mem on 9.3

[Alex Shulgin]

Andrew Gierth and...@tao11.riddles.org.uk writes:

 Alex == Alex Shulgin a...@commandprompt.com writes:

   Tom Lane t...@sss.pgh.pa.us writes:
   Must've been my evil twin.

  Alex Sorry, I must be under false impression that RhodiumToad is
  Alex *your* nick on #postgresql at freenode.  I don't recall who
  Alex told me that, but I was pretty sure it's you. :-p

 ... what

 People do occasionally make jokes on IRC about me being Tom's clone; I
 know they mean it in a positive way but I still find it *extremely*
 annoying, so I do try and discourage it. (If they're making those same
 jokes elsewhere, I haven't been aware of it, but please consider this
 a polite public request to stop.)

 My first name is easily visible in the irc gecos field:

 *** RhodiumToad is ~andrew@[my hostname] (Andrew)

 and there is also the IRC users list on the wiki:
 http://wiki.postgresql.org/wiki/IRC2RWNames

Andrew, Tom,

Sorry for the confusion.  And, Andrew, thanks again for the help! :-)

--
Alex


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

Re: [HACKERS] [PATCH] add ssl_protocols configuration option

[Alex Shulgin]

Alex Shulgin a...@commandprompt.com writes:

 I can do that too, just need a hint where to look at in libpq/psql to
 add the option.

 The place to *enforce* the option is src/interfaces/libpq/fe-secure.c
 (look for SSLv23_method() and SSL_CTX_set_options()).  I haven't looked
 into how to set it.

 Yes, I've figured it out.  Guess we'd better share the ssl_protocol
 value parser code between libpq and the backend.  Any precedent?

OK, looks like I've come up with something workable: I've added
sslprotocol connection string keyword similar to pre-existing
sslcompression, etc.

Please see attached v2 of the original patch.  I'm having doubts about
the name of openssl.h header though, libpq-openssl.h?

--
Alex

From 7cd60e962ce5e7fc10dc52ed9c92b0b2b5c4c7f1 Mon Sep 17 00:00:00 2001
From: Alex Shulgin a...@commandprompt.com
Date: Wed, 19 Nov 2014 19:49:29 +0300
Subject: [PATCH] DRAFT: ssl_protocols GUC

---
 doc/src/sgml/config.sgml  |  29 ++
 doc/src/sgml/libpq.sgml   |  25 ++
 src/backend/libpq/Makefile|   2 +-
 src/backend/libpq/be-secure-openssl.c |  29 --
 src/backend/libpq/be-secure.c |   3 +-
 src/backend/libpq/openssl.c   | 124 ++
 src/backend/utils/misc/guc.c  |  15 
 src/backend/utils/misc/postgresql.conf.sample |   1 +
 src/include/libpq/libpq.h |   1 +
 src/include/libpq/openssl.h   |  50 +++
 src/interfaces/libpq/Makefile |   8 +-
 src/interfaces/libpq/fe-connect.c |   4 +
 src/interfaces/libpq/fe-secure-openssl.c  |  18 +++-
 src/interfaces/libpq/libpq-int.h  |   1 +
 14 files changed, 300 insertions(+), 10 deletions(-)
 create mode 100644 src/backend/libpq/openssl.c
 create mode 100644 src/include/libpq/openssl.h

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
new file mode 100644
index ab8c263..8b701df
*** a/doc/src/sgml/config.sgml
--- b/doc/src/sgml/config.sgml
*** include_dir 'conf.d'
*** 1027,1032 
--- 1027,1061 
/listitem
   /varlistentry
  
+  varlistentry id=guc-ssl-protocols xreflabel=ssl_protocols
+   termvarnamessl_protocols/varname (typestring/type)
+   indexterm
+primaryvarnamessl_protocols/ configuration parameter/primary
+   /indexterm
+   /term
+   listitem
+para
+ Specifies a colon-separated list of acronymSSL/ protocols that are
+ allowed to be used on secure connections. Each entry in the list can
+ be prefixed by a literal+/ (add to the current list)
+ or literal-/ (remove from the current list). If no prefix is used,
+ the list is replaced with the specified protocol.
+/para
+para
+ The full list of supported protocols can be found in the
+ the applicationopenssl/ manual page.  In addition to the names of
+ individual protocols, the following keywords can be
+ used: literalALL/ (all protocols supported by the underlying
+ crypto library), literalSSL/ (all supported versions
+ of acronymSSL/) and literalTLS/ (all supported versions
+ of acronymTLS/).
+/para
+para
+ The default is literalALL:-SSL/literal.
+/para
+   /listitem
+  /varlistentry
+ 
   varlistentry id=guc-ssl-ciphers xreflabel=ssl_ciphers
termvarnamessl_ciphers/varname (typestring/type)
indexterm
diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
new file mode 100644
index e23e91d..9ea71a4
*** a/doc/src/sgml/libpq.sgml
--- b/doc/src/sgml/libpq.sgml
*** postgresql://%2Fvar%2Flib%2Fpostgresql/d
*** 1230,1235 
--- 1230,1250 
/listitem
   /varlistentry
  
+  varlistentry id=libpq-connect-sslprotocols xreflabel=sslprotocols
+   termliteralsslprotocols/literal/term
+   listitem
+para
+ Specifies a colon-separated list of acronymSSL/ protocols that are
+ allowed to be used on secure connections.
+ See xref linkend=guc-ssl-protocols server configuration parameter
+ for format.
+/para
+para
+ Defaults to literalALL:-SSL/.
+/para
+   /listitem
+  /varlistentry
+ 
   varlistentry id=libpq-connect-sslcompression xreflabel=sslcompression
termliteralsslcompression/literal/term
listitem
*** myEventProc(PGEventId evtId, void *evtIn
*** 6711,6716 
--- 6726,6741 
   /para
  /listitem
  
+ listitem
+  para
+   indexterm
+primaryenvarPGSSLPROTOCOLS/envar/primary
+   /indexterm
+   envarPGSSLPROTOCOLS/envar behaves the same as the xref
+   linkend=libpq-connect-sslprotocols connection parameter.
+  /para
+ /listitem
+ 
  listitem
   para
indexterm
diff --git a/src/backend/libpq/Makefile b/src/backend

Re: Missing OOM checks in libpq (was Re: [HACKERS] Replication connection URI?)

[Alex Shulgin]

Heikki Linnakangas hlinnakan...@vmware.com writes:

 On 11/24/2014 06:05 PM, Alex Shulgin wrote:
 The first patch is not on topic, I just spotted this missing check.

 *** a/src/interfaces/libpq/fe-connect.c
 --- b/src/interfaces/libpq/fe-connect.c
 *** conninfo_array_parse(const char *const *
 *** 4402,4407 
 --- 4402,4415 
  if 
 (options[k].val)
  
 free(options[k].val);
  options[k].val 
 = strdup(str_option-val);
 +if 
 (!options[k].val)
 +{
 +
 printfPQExpBuffer(errorMessage,
 +
   libpq_gettext(out of memory\n));
 +
 PQconninfoFree(options);
 +
 PQconninfoFree(dbname_options);
 +return 
 NULL;
 +}
  break;
  }
  }

 Oh. There are actually many more places in connection option parsing
 that don't check the return value of strdup(). The one in fillPGConn
 even has an XXX comment saying it probably should check it. You can
 get quite strange behavior if one of them fails. If for example the
 strdup() on dbname fails, you might end up connecting to different
 database than intended. And if the conn-sslmode =
 strdup(DefaultSSLMode); call in connectOptions2 fails, you'll get a
 segfault later because at least connectDBstart assumes that sslmode is
 not NULL.

 I think we need to fix all of those, and backpatch. Per attached.

Yikes!  Looks sane to me.

I've checked that patches #2 and #3 can be applied on top of this, w/o
rebasing.

--
Alex


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

Re: Missing OOM checks in libpq (was Re: [HACKERS] Replication connection URI?)

[Alex Shulgin]

Heikki Linnakangas hlinnakan...@vmware.com writes:

 I think we need to fix all of those, and backpatch. Per attached.

 Yikes!  Looks sane to me.

 Ok thanks, committed. It didn't apply cleanly to 9.0, 9.1 and 9.2, so
 the patch for those branches looks a bit different.

Great.  Are you looking at the actual replication URI patch?  Or should
I add it to commitfest so we don't lose track of it?

--
Alex


-- 
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] Replication connection URI?

[Alex Shulgin]

Heikki Linnakangas hlinnakan...@vmware.com writes:

 The second one is a self-contained fix, but the third one which is the
 actual patch depends on the second one, because it specifies the dbname
 keyword two times: first to parse the conninfo/URI, then to override any
 dbname provided by the user with replication pseudo-database name.

 Hmm. Should we backpatch the second patch? It sure seems like an
 oversight rather than deliberate that you can't override dbname from the
 connection string with a later dbname keyword. I'd say yes.

 How about the third patch? Probably not; it was an oversight with the
 connection URI patch that it could not be used in primary_conninfo, but
 it's not a big deal in practice as you can always use a non-URI
 connection string instead.

 Ok, committed the second patch to all stable branches, and the third
 patch to master.

 In the second patch, I added a sentence to the docs to mention that
 only the first dbname parameter is expanded. It's debatable if
 that's what we actually want. It would be handy to be able to merge
 multiple connection strings, by specifying multiple dbname
 parameters. But now the docs at least match the code, changing the
 behavior would be a bigger change.

 From the third patch, I removed the docs changes. It's necessary to
 say connection string or URI everywhere, the URI format is just one
 kind of a connection string. I also edited the code that builds the
 keyword/value array, to make it a bit more readable.

Yay, many thanks! :-)

--
Alex


-- 
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] Turning recovery.conf into GUCs

[Alex Shulgin]

Alex Shulgin a...@commandprompt.com writes:

  * Why do you include xlog_internal.h in guc.c and not xlog.h?

 we actually need both but including xlog_internal.h also includes xlog.h
 i added xlog.h and if someone things is enough only putting
 xlog_internal.h let me know

 What's required from xlog_internal.h?

 Looks like this was addressed before me.

Actually, it's there to bring in MAXFNAMELEN which is used in
check_recovery_target_name.  Not sure how it even compiled without that,
but after some branch switching it doesn't anymore. :-p

Maybe we should move these check/assign hooks to xlog.c, but that's
likely going to create header files dependency problem due to use of
GucSource in the hook prototype...

--
Alex


-- 
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] Turning recovery.conf into GUCs

[Alex Shulgin]

Josh Berkus j...@agliodbs.com writes:
 
 Well, the latest version of this patch fails to start when it sees
 'recovery.conf' in PGDATA:
 
   FATAL:  recovery.conf is not supported anymore as a recovery method
   DETAIL:  Refer to appropriate documentation about migration methods
 
 I've missed all the discussion behind this decision and after reading
 the ALTER SYSTEM/conf.d thread I'm even more confused so I'd like
 someone more knowledgeable to speak up on the status of this.

 The argument was that people wanted to be able to have an empty
 recovery.conf file as a we're in standby trigger so that they could
 preserve backwards compatibility with external tools.  I don't agree
 with this argument, but several people championed it.

It doesn't look like we can provide for 100% backwards compatibility
with existing tools, here's why:

a) The only sensible way to use recovery.conf as GUC source is via
   include/include_dir from postgresql.conf.

b) The server used to rename $PGDATA/recovery.conf to
   $PGDATA/recovery.done so when it is re-started it doesn't
   accidentally start in recovery mode.

We can't keep doing (b) because that will make postgres fail to start on
a missing include.  Well, it won't error out if we place the file under
include_dir, as only files with the .conf suffix are included, but
then again the server wouldn't know which file to rename unless we tell
it or hard-code the the filename.  Either way this is not 100% backwards
compatible.

Now the question is if we are going to break compatibility anyway:
should we try to minimize the difference or will it disserve the user by
providing a false sense of compatibility?

For one, if we're breaking things, the trigger_file GUC should be
renamed to end_recovery_trigger_file or something like that, IMO.
That's if we decide to keep it at all.

 The docs use the term continuous recovery.
 
 Either way, from the code it is clear that we only stay in recovery if
 standby_mode is directly turned on.  This makes the whole check for a
 specially named file unnecessary, IMO: we should just check the value of
 standby_mode (which is off by default).

 So, what happens when someone does pg_ctl promote?  Somehow
 standby_mode needs to get set to off.  Maybe we write standby_mode =
 off to postgresql.auto.conf?

Well, standby_mode is only consulted at startup after crash recovery.
But suddenly, a tool that *writes* recovery.conf needs to also
consult/edit postgresql.auto.conf...  Maybe that's inevitable, but still
a point to consider.

 By the way, is there any use in setting standby_mode=on and any of the
 recovery_target* GUCs at the same time?

 See my thread on this topic from last week.  Short answer: No.

 I think it can only play together if you set the target farther than the
 latest point you've got in the archive locally.  So that's sort of
 Point-in-Future-Recovery.  Does that make any sense at all?

 Again, see the thread.  This doesn't work in a useful way, so there's no
 reason to write code to enable it.

Makes sense.  Should we incorporate the actual tech and doc fix in this
patch?

  /* File path names (all relative to $PGDATA) */
 -#define RECOVERY_COMMAND_FILE recovery.conf
 -#define RECOVERY_COMMAND_DONE recovery.done
 +#define RECOVERY_ENABLE_FILE  standby.enabled

 Imo the file and variable names should stay coherent.
 
 Yes, once we settle on the name (and if we really need that extra
 trigger file.)

 Personally, if we have three methods of promotion:

 1) pg_ctl promote
 2) edit postgresql.conf and reload
 3) ALTER SYSTEM SET and reload

 ... I don't honestly think we need a 4th method for promotion.

Me neither.  It is tempting to make everything spin around GUCs without
the need for any external trigger files.

 The main reason to want a we're in recovery file is for PITR rather
 than for replication, where it has a number of advantages as a method,
 the main one being that recovery.conf is unlikely to be overwritten by
 the contents of the backup.

 HOWEVER, there's a clear out for this with conf.d.  If we enable conf.d
 by default, then we can simply put recovery settings in conf.d, and
 since that specific file (which can have whatever name the user chooses)
 will not be part of backups, it would have the same advantage as
 recovery.conf, without the drawbacks.

 Discuss?

Well, I don't readily see how conf.d is special with regard to base
backup, wouldn't you need to exclude it explicitly still?

--
Alex


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

[HACKERS] Replication connection URI?

[Alex Shulgin]

Hackers,

It appears that replication connection doesn't support URI but only the
traditional conninfo string.

src/backend/replication/libpqwalreceiver/libpqwalreceiver.c:99: in 
libpqrcv_connect():

snprintf(conninfo_repl, sizeof(conninfo_repl),
 %s dbname=replication replication=true 
fallback_application_name=walreceiver,
 conninfo);

A patch to fix this welcome?

--
Alex

PS: I wrote the original URI parser used in libpq.


-- 
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] Replication connection URI?

[Alex Shulgin]

Heikki Linnakangas hlinnakan...@vmware.com writes:

 It appears that replication connection doesn't support URI but only the
 traditional conninfo string.

 src/backend/replication/libpqwalreceiver/libpqwalreceiver.c:99: in 
 libpqrcv_connect():

  snprintf(conninfo_repl, sizeof(conninfo_repl),
   %s dbname=replication replication=true 
 fallback_application_name=walreceiver,
   conninfo);

 A patch to fix this welcome?

 Yeah, seems like an oversight. Hopefully you can fix that without
 teaching libpqwalreceiver what connection URIs look like..

Please see attached.  We're lucky that PQconnectdbParams has an option
to parse and expand the first dbname parameter if it looks like a
connection string (or a URI).

The first patch is not on topic, I just spotted this missing check.

The second one is a self-contained fix, but the third one which is the
actual patch depends on the second one, because it specifies the dbname
keyword two times: first to parse the conninfo/URI, then to override any
dbname provided by the user with replication pseudo-database name.

Have a nice day!
--
Alex

From 156e6faa96ad6a2ce58055ad72883ed78c576e5b Mon Sep 17 00:00:00 2001
From: Alex Shulgin a...@commandprompt.com
Date: Mon, 24 Nov 2014 16:55:50 +0300
Subject: [PATCH 1/3] Add missing check on OOM in expand_dbname path of
 conninfo_array_parse().

---
 src/interfaces/libpq/fe-connect.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
new file mode 100644
index 3fe8c21..d7f2ec2
*** a/src/interfaces/libpq/fe-connect.c
--- b/src/interfaces/libpq/fe-connect.c
*** conninfo_array_parse(const char *const *
*** 4402,4407 
--- 4402,4415 
  if (options[k].val)
  	free(options[k].val);
  options[k].val = strdup(str_option-val);
+ if (!options[k].val)
+ {
+ 	printfPQExpBuffer(errorMessage,
+ 	  libpq_gettext(out of memory\n));
+ 	PQconninfoFree(options);
+ 	PQconninfoFree(dbname_options);
+ 	return NULL;
+ }
  break;
  			}
  		}
-- 
2.1.0

From 44d9d6a2c9cf5af83988f9d3b6eeb39c36104ef9 Mon Sep 17 00:00:00 2001
From: Alex Shulgin a...@commandprompt.com
Date: Mon, 24 Nov 2014 18:12:51 +0300
Subject: [PATCH 2/3] Allow further dbname=value to override conninfo parsed
 from an expanded dbname in conninfo_array_parse().

---
 src/interfaces/libpq/fe-connect.c | 18 +-
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
new file mode 100644
index d7f2ec2..5b45128
*** a/src/interfaces/libpq/fe-connect.c
--- b/src/interfaces/libpq/fe-connect.c
*** conninfo_parse(const char *conninfo, PQE
*** 4302,4311 
   * Defaults are supplied (from a service file, environment variables, etc)
   * for unspecified options, but only if use_defaults is TRUE.
   *
!  * If expand_dbname is non-zero, and the value passed for keyword dbname is a
!  * connection string (as indicated by recognized_connection_string) then parse
!  * and process it, overriding any previously processed conflicting
!  * keywords. Subsequent keywords will take precedence, however.
   */
  static PQconninfoOption *
  conninfo_array_parse(const char *const * keywords, const char *const * values,
--- 4302,4313 
   * Defaults are supplied (from a service file, environment variables, etc)
   * for unspecified options, but only if use_defaults is TRUE.
   *
!  * If expand_dbname is non-zero, and the value passed for the first occurrence
!  * of dbname keyword is a connection string (as indicated by
!  * recognized_connection_string) then parse and process it, overriding any
!  * previously processed conflicting keywords. Subsequent keywords will take
!  * precedence, however. In particular, a further occurrence of dbname may
!  * override the dbname provided in the connection string.
   */
  static PQconninfoOption *
  conninfo_array_parse(const char *const * keywords, const char *const * values,
*** conninfo_array_parse(const char *const *
*** 4381,4387 
  			}
  
  			/*
! 			 * If we are on the dbname parameter, and we have a parsed
  			 * connection string, copy those parameters across, overriding any
  			 * existing previous settings.
  			 */
--- 4383,4389 
  			}
  
  			/*
! 			 * If we are on the *first* dbname parameter, and we have a parsed
  			 * connection string, copy those parameters across, overriding any
  			 * existing previous settings.
  			 */
*** conninfo_array_parse(const char *const *
*** 4415,4420 
--- 4417,4428 
  		}
  	}
  }
+ /*
+  * Clear out the parsed dbname, so that a possible further
+  * occurrence of dbname have the chance to override it.
+  */
+ PQconninfoFree(dbname_options);
+ dbname_options = NULL;
  			}
  			else
  			{
-- 
2.1.0

Re: [HACKERS] Turning recovery.conf into GUCs

[Alex Shulgin]

Jaime Casanova ja...@2ndquadrant.com writes:

 Either way, from the code it is clear that we only stay in recovery if
 standby_mode is directly turned on.  This makes the whole check for a
 specially named file unnecessary, IMO: we should just check the value of
 standby_mode (which is off by default).

 no. currently we enter in recovery mode when postgres see a
 recovery.conf and stays in recovery mode when standby_mode is on or an
 appropiate restore_command is provided.

 which means recovery.conf has two uses:
 1) start in recovery mode (not continuous)
 2) provide parameters for recovery mode and for streaming

 we still need a recovery trigger file that forces postgres to start
 in recovery mode and acts accordingly to recovery GUCs

Yes, these were my latest findings also, but if instead of removing the
trigger file upon successful recovery the server would set
standby_mode=off, that would also work.

--
Alex


-- 
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] Turning recovery.conf into GUCs

[Alex Shulgin]

Josh Berkus j...@agliodbs.com writes:
 
 only that you need to start in recovery mode to start replication

 Right, but my point is that having a trigger file *is not necessary for
 replication, only for PITR* -- and maybe not necessary even for PITR.
 That is, in a streaming replication configuration, having a
 standby_mode = on|off parameter is 100% sufficient to control
 replication with the small detail that pg_ctl promote needs to set it
 in pg.auto.conf or conf.d.

 And, now, having given it some thought, I'm going to argue that it's not
 required for PITR either, provided that we can use the auto.conf method.

 Before I go into my ideas, though, what does the current patch do
 regarding non-replication PITR?

It removes that $PGDATA/standby.enable trigger file it relies on to
start the PITR in the first place.

--
Alex


-- 
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] Turning recovery.conf into GUCs

[Alex Shulgin]

Josh Berkus j...@agliodbs.com writes:

 Before I go into my ideas, though, what does the current patch do
 regarding non-replication PITR?
 
 It removes that $PGDATA/standby.enable trigger file it relies on to
 start the PITR in the first place.

 OK, and that's required for replication too?  I'm OK with that if it
 gets the patch in.

In the current form of the patch, yes.  Thought I don't think I like it.

--
Alex


-- 
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] Replication connection URI?

[Alex Shulgin]

Alex Shulgin a...@commandprompt.com writes:

 Heikki Linnakangas hlinnakan...@vmware.com writes:

 It appears that replication connection doesn't support URI but only the
 traditional conninfo string.

 src/backend/replication/libpqwalreceiver/libpqwalreceiver.c:99: in 
 libpqrcv_connect():

  snprintf(conninfo_repl, sizeof(conninfo_repl),
   %s dbname=replication replication=true 
 fallback_application_name=walreceiver,
   conninfo);

 A patch to fix this welcome?

 Yeah, seems like an oversight. Hopefully you can fix that without
 teaching libpqwalreceiver what connection URIs look like..

 Please see attached.  We're lucky that PQconnectdbParams has an option
 to parse and expand the first dbname parameter if it looks like a
 connection string (or a URI).

 The first patch is not on topic, I just spotted this missing check.

 The second one is a self-contained fix, but the third one which is the
 actual patch depends on the second one, because it specifies the dbname
 keyword two times: first to parse the conninfo/URI, then to override any
 dbname provided by the user with replication pseudo-database name.

These patches are really simple, I hope a committer will pick them up?
Or should I add them to the commitfest?

Also, I'd rather get this committed first, then rebase that
recovery.conf-GUC patch onto it and submit an updated version.

Thanks.
--
Alex


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

Re: [HACKERS] [PATCH] add ssl_protocols configuration option

[Alex Shulgin]

Dag-Erling Smørgrav d...@des.no writes:

 Alex Shulgin a...@commandprompt.com writes:
 I can do that too, just need a hint where to look at in libpq/psql to
 add the option.

 The place to *enforce* the option is src/interfaces/libpq/fe-secure.c
 (look for SSLv23_method() and SSL_CTX_set_options()).  I haven't looked
 into how to set it.

Yes, I've figured it out.  Guess we'd better share the ssl_protocol
value parser code between libpq and the backend.  Any precedent?

--
Alex


-- 
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] Turning recovery.conf into GUCs

[Alex Shulgin]

/xlog.c 
 b/src/backend/access/transam/xlog.c
 index b53ae87..54f6a0d 100644
 --- a/src/backend/access/transam/xlog.c
 +++ b/src/backend/access/transam/xlog.c
 @@ -64,11 +64,12 @@
  extern uint32 bootstrap_data_checksum_version;
  
  /* File path names (all relative to $PGDATA) */
 -#define RECOVERY_COMMAND_FILE   recovery.conf
 -#define RECOVERY_COMMAND_DONE   recovery.done
 +#define RECOVERY_ENABLE_FILEstandby.enabled

 Imo the file and variable names should stay coherent.

Yes, once we settle on the name (and if we really need that extra
trigger file.)

 +/* recovery.conf is not supported anymore */
 +#define RECOVERY_COMMAND_FILE   recovery.conf

 +boolStandbyModeRequested = false;
 +static TimestampTz recoveryDelayUntilTime;

 This imo should be lowercase now, the majority of GUC variables are.

This is not a GUC variable, though it's calculated based on a GUC
recovery_min_apply_delay.

 +/* are we currently in standby mode? */
 +bool StandbyMode = false;

 Why did you move this?

It was easy to move it back though.

 -if (rtliGiven)
 +if (strcmp(recovery_target_timeline_string, ) != 0)
  {

 Why not have the convention that NULL indicates a unset target_timeline
 like you use for other GUCs? Mixing things like this is confusing.

 Why is recovery_target_timeline stored as a string? Because it's a
 unsigned int? If so, you should have an assign hook setting up a)
 rtliGiven, b) properly typed variable.

Yes, I believe setting these to NULL by default makes a lot more sense.
Then one can check if there was a non-default setting by checking the
*_string variable, which is not possible with int or TimestampTz.

 -if (rtli)
 +if (recovery_target_timeline)
  {
  /* Timeline 1 does not have a history file, all else 
 should */
 -if (rtli != 1  !existsTimeLineHistory(rtli))
 +if (recovery_target_timeline != 1  
 +
 !existsTimeLineHistory(recovery_target_timeline))
  ereport(FATAL,
  (errmsg(recovery target 
 timeline %u does not exist,
 -rtli)));
 -recoveryTargetTLI = rtli;
 +
 recovery_target_timeline)));
 +recoveryTargetTLI = recovery_target_timeline;
  recoveryTargetIsLatest = false;

 So, now we have a recoveryTargetTLI and recovery_target_timeline
 variable? Really? Why do we need recoveryTargetTLI at all now?

Looks like we still do need both of them.  The initial value of
recoveryTargetTLI is derived from pg_control, but later it can be
overriden by this setting.

However, if the recovery_target_timeline was latest we need to find
the latest TLI, based on the initial value from pg_control.  But we
can't set final recoveryTargetTLI value in the GUC assign hook, because
we might need to fetch some history files first.

 +static void
 +assign_recovery_target_time(const char *newval, void *extra)
 +{
 +recovery_target_time = *((TimestampTz *) extra);
 +
 +if (recovery_target_xid != InvalidTransactionId)
 +recovery_target = RECOVERY_TARGET_XID;
 +else if (recovery_target_name[0])
 +recovery_target = RECOVERY_TARGET_NAME;
 +else if (recovery_target_time != 0)
 +recovery_target = RECOVERY_TARGET_TIME;
 +else
 +recovery_target = RECOVERY_TARGET_UNSET;
 +}
 +

 I don't think it's correct to do such hangups in the assign hook - you
 have no ideas in which order they will be called and such. Imo that
 should happen at startup, like we also do it for other interdependent
 variables like wal_buffers.

Yeah, that looked weird.  Moved to StartupXLOG().

I disliked the strtoul handling in the earlier version of the patch,
especially given that with the base=0 it can parse 0x-prefixed hex
strings.  I would rather error out on non-hex digit instead of stopping
and calling it OK.  This change is included in the new version.

Should we really allow specifying negative values for XID/timeline?
Right now it will happily consume -1 for recovery_target_xid and
complain if it's out of range, like this:

LOG:  starting point-in-time recovery to XID 4294967295
LOG:  invalid primary checkpoint record
LOG:  invalid secondary checkpoint record

Allowing negative values makes even less sense for timelines, IMO.

--
Alex

From de59408e524e3818a7cea98a7c1f049e09eb9f79 Mon Sep 17 00:00:00 2001
From: Alex Shulgin a...@commandprompt.com
Date: Fri, 21 Nov 2014 12:22:22 +0300
Subject: [PATCH] DRAFT: rebased recovery_guc_v5.2.patch

---
 contrib/pg_archivecleanup/pg_archivecleanup.c   |   2 +-
 contrib/pg_standby/pg_standby.c |   2 +-
 doc/src/sgml/backup.sgml|  40 +-
 doc/src/sgml/config.sgml

Re: [HACKERS] [PATCH] add ssl_protocols configuration option

[Alex Shulgin]

Dag-Erling Smørgrav d...@des.no writes:

 Alex Shulgin a...@commandprompt.com writes:
 * The patch works as advertised, though the only way to verify that
   connections made with the protocol disabled by the GUC are indeed
   rejected is to edit fe-secure-openssl.c to only allow specific TLS
   versions.  Adding configuration on the libpq side as suggested in the
   original discussion might help here.

 I can easily do that, but I won't have time until next week or so.

I can do that too, just need a hint where to look at in libpq/psql to
add the option.

For SSL we have sslmode and sslcompression, etc. in conninfo, so adding
sslprotocols seems to be an option.  As an aside note: should we also
expose a parameter to choose SSL ciphers (would be a separate patch)?

--
Alex


-- 
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] Merging recovery.conf into PostgreSQL.conf -- where did this go?

[Alex Shulgin]

Andres Freund and...@2ndquadrant.com writes:

 On 2014-11-19 15:09:10 -0800, Josh Berkus wrote:
 One patch that got deferred from 9.4 was the merger of recovery.conf and
 postgresql.conf, due to conflicts with ALTER SYSTEM SET.  However, this
 is still a critical TODO, because recovery.conf is still an ongoing
 major management problem for PostgreSQL DBAs.
 
 So, what happened to this?  I kinda expected it to get committed right
 after we opened 9.5.

 Nobody did the remaining work.

I'd like to work on this.  AFAICT, this CommitFest entry is the latest
on the subject, right?

  https://commitfest.postgresql.org/action/patch_view?id=1293

Should/may I move it to the next Open fest?

--
Alex


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

Re: [HACKERS] [PATCH] add ssl_protocols configuration option

[Alex Shulgin]

Dag-Erling Smørgrav d...@des.no writes:

 The attached patches add an ssl_protocols configuration option which
 control which versions of SSL or TLS the server will use.  The syntax is
 similar to Apache's SSLProtocols directive, except that the list is
 colon-separated instead of whitespace-separated, although that is easy
 to change if it proves unpopular.

Hello,

Here is my review of the patch against master branch:

* The patch applies and compiles cleanly, except for sgml docs:

postgres.xml:66141: element varlistentry: validity error : Element
varlistentry content does not follow the DTD, expecting (term+ ,
listitem), got (term indexterm listitem)

   /para/listitem/varlistentryvarlistentry
^

  The fix is to move indexterm under the term tag in the material
  added to config.sgml by the patch.

* The patch works as advertised, though the only way to verify that
  connections made with the protocol disabled by the GUC are indeed
  rejected is to edit fe-secure-openssl.c to only allow specific TLS
  versions.  Adding configuration on the libpq side as suggested in the
  original discussion might help here.

* The code allows specifying SSLv2 and SSLv3 in the GUC, but removes
  them forcibly after parsing the complete string (a warning is issued).
  Should we also add a note about this to the documentation?

* In case the list of allowed protocols comes empty a FATAL error
  message is logged: could not set the protocol list (no valid
  protocols available).  I think it's worth changing that to could not
  set SSL protocol list...  All other related error/warning logs
  include SSL.

* The added code follows conventions and looks good to me.  I didn't
  spot any problems in the parser.  I've tried a good deal of different
  values and all seem to be parsed correctly.

I would try to apply patches for older branches if there is consensus
that we really need this patch and we want to back-patch it.

Thanks.
--
Alex


-- 
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] Idle transaction cancel/timeout and SSL revisited

[Alex Shulgin]

Andres Freund and...@2ndquadrant.com writes:
 On 2014-11-15 00:11:36 +0300, Alex Shulgin wrote: 
 After reading up through archives on the two $subj related TODO items
 I'm under impression that the patches[1,2] didn't make it mainly because
 of the risk of breaking SSL internals if we try to longjump out of the
 signal handler in the middle of a blocking SSL read and/or if we try to
 report that final transaction/connection aborted notice to the client.

 I've written a patch that allows that - check
 http://archives.postgresql.org/message-id/20140927225421.GE5423%40alap3.anarazel.de

 I feel pretty confident that that's the way to go. I just need some time
 to polish it.

 What if instead of trying to escape from the signal handler we would set
 a flag and use it to simulate EOF after the recv() is restarted due to
 EINTR?  From the backend perspective it should look like the client has
 just hang up.

 That's pretty much what the above does. Except that it actually deals
 with blocking syscalls by not using them and relying on latches/select
 instead.

Yay, that's nice, because my EOF approach is sort of fishy.

I've checked the patches: they apply and compile on top of current HEAD
(one failed hunk in pqcomm.c), so I can help with testing if needed.

There is a (short) list of items that caught my attention.  I will post
in reply to your patches thread.

--
Alex


-- 
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] Escaping from blocked send() reprised.

[Alex Shulgin]

Andres Freund and...@2ndquadrant.com writes:

 I've invested some more time in this:
 0002 now makes sense on its own and doesn't change anything around the
  interrupt handling. Oh, and it compiles without 0003.

In this patch, the endif appears to be misplaced in PostgresMain:

+   if (MyProcPort != NULL)
+   {
+#ifdef WIN32
+   pgwin32_noblock = true;
+#else
+   if (!pg_set_noblock(MyProcPort-sock))
+   ereport(COMMERROR,
+   (errmsg(could not set socket to 
nonblocking mode: %m)));
+   }
+#endif
+
pqinitmask();

 0003 Sinval/notify processing got simplified further. There really isn't
  any need for DisableNotifyInterrupt/DisableCatchupInterrupt
  anymore. Also begin_client_read/client_read_ended don't make much
  sense anymore. Instead introduce ProcessClientReadInterrupt (which
  wants a better name).
 There's also a very WIP
 0004 Allows secure_read/write be interrupted when ProcDiePending is
  set. All of that happens via the latch mechanism, nothing happens
  inside signal handlers. So I do think it's quite an improvement
  over what's been discussed in this thread.
  But it (and the other approaches) do noticeably increase the
  likelihood of clients not getting the error message if the client
  isn't actually dead. The likelihood of write() being blocked
  *temporarily* due to normal bandwidth constraints is quite high
  when you consider COPY FROM and similar. Right now we'll wait till
  we can put the error message into the socket afaics.

 1-3 need some serious comment work, but I think the approach is
 basically sound. I'm much, much less sure about allowing send() to be
 interrupted.

After re-reading these I don't see the rest of items I wanted to inqury
about anymore, so it just makes more sense now.

One thing I did try is sending a NOTICE to the client when in
ProcessInterrupts() and DoingCommandRead is true.  I think[1] it was
expected to be delivered instantly, but actually the client (psql) only
displays it after sending the next statement.

While I'm reading on FE/BE protocol someone might want to share his
wisdom on this subject.  My guess: psql blocks on readline/libedit call
and can't effectively poll the server socket before complete input from
user.

--
Alex

[1] http://www.postgresql.org/message-id/1262173040.19367.5015.camel@ebony

  ``AFAIK, NOTICE was suggested because it can be sent at any time,
whereas ERRORs are only associated with statements.''


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

[HACKERS] Idle transaction cancel/timeout and SSL revisited

[Alex Shulgin]

Hello Hackers,

After reading up through archives on the two $subj related TODO items
I'm under impression that the patches[1,2] didn't make it mainly because
of the risk of breaking SSL internals if we try to longjump out of the
signal handler in the middle of a blocking SSL read and/or if we try to
report that final transaction/connection aborted notice to the client.

What if instead of trying to escape from the signal handler we would set
a flag and use it to simulate EOF after the recv() is restarted due to
EINTR?  From the backend perspective it should look like the client has
just hang up.

Both SSL and cleartext connections are routed through secure_raw_read,
so it seems like a good candidate for checking the flag we would set in
StatementCancelHandler().  (Should we also add the same check in
interactive_getc()?)

Ultimately, in PostgresMain() the ReadCommand() would return EOF and we
can either let the existing code shutdown the backend, or add special
handling that will only abort the transaction and report the case to the
client, which was the intent in[1].  And if that works out, the timeout
handler in[2] can simply reuse the flag.

A potential problem with this is that SSL might perform some eager
cleanup when seeing EOF, so the backend might need to reset/restart the
connection (is that possible?)

I'm attaching a patch (that doesn't compile yet :-p) in hope it can be
useful for the purpose of the discussion.  Notably, secure_raw_read()
can't know about IdleTransactionCancelPending and I'm not sure what
would be the best way to let it see that (is it too much of a shortcut
anyway?)

--
Alex

[1] http://www.postgresql.org/message-id/1262268211.19367.10736.camel@ebony
[2] http://www.postgresql.org/message-id/538dc843.2070...@dalibo.com

diff --git a/src/backend/libpq/be-secure-openssl.c b/src/backend/libpq/be-secure-openssl.c
new file mode 100644
index b05364c..377984d
*** a/src/backend/libpq/be-secure-openssl.c
--- b/src/backend/libpq/be-secure-openssl.c
*** rloop:
*** 543,550 
  	 errmsg(SSL error: %s, SSLerrmessage(;
  			/* fall through */
  		case SSL_ERROR_ZERO_RETURN:
! 			errno = ECONNRESET;
! 			n = -1;
  			break;
  		default:
  			ereport(COMMERROR,
--- 543,553 
  	 errmsg(SSL error: %s, SSLerrmessage(;
  			/* fall through */
  		case SSL_ERROR_ZERO_RETURN:
! 			if (!IdleTransactionCancelPending) /* HACK */
! 			{
! errno = ECONNRESET;
! n = -1;
! 			}
  			break;
  		default:
  			ereport(COMMERROR,
diff --git a/src/backend/libpq/be-secure.c b/src/backend/libpq/be-secure.c
new file mode 100644
index 41ec1ad..44079ff
*** a/src/backend/libpq/be-secure.c
--- b/src/backend/libpq/be-secure.c
*** secure_raw_read(Port *port, void *ptr, s
*** 145,155 
  {
  	ssize_t		n;
  
! 	prepare_for_client_read();
  
! 	n = recv(port-sock, ptr, len, 0);
  
! 	client_read_ended();
  
  	return n;
  }
--- 145,160 
  {
  	ssize_t		n;
  
! 	if (IdleTransactionCancelPending)
! 		n = 0; /* simulate EOF */
! 	else
! 	{
! 		prepare_for_client_read();
  
! 		n = recv(port-sock, ptr, len, 0);
  
! 		client_read_ended();
! 	}
  
  	return n;
  }
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
new file mode 100644
index cc62b2c..37437fc
*** a/src/backend/tcop/postgres.c
--- b/src/backend/tcop/postgres.c
*** interactive_getc(void)
*** 310,318 
  {
  	int			c;
  
! 	prepare_for_client_read();
! 	c = getc(stdin);
! 	client_read_ended();
  	return c;
  }
  
--- 310,323 
  {
  	int			c;
  
! 	if (IdleTransactionCancelPending)
! 		c = EOF;
! 	else
! 	{
! 		prepare_for_client_read();
! 		c = getc(stdin);
! 		client_read_ended();
! 	}
  	return c;
  }
  
*** StatementCancelHandler(SIGNAL_ARGS)
*** 2629,2642 
  	if (!proc_exit_inprogress)
  	{
  		InterruptPending = true;
! 		QueryCancelPending = true;
  
  		/*
  		 * If it's safe to interrupt, and we're waiting for input or a lock,
  		 * service the interrupt immediately
  		 */
  		if (ImmediateInterruptOK  InterruptHoldoffCount == 0 
! 			CritSectionCount == 0)
  		{
  			/* bump holdoff count to make ProcessInterrupts() a no-op */
  			/* until we are done getting ready for it */
--- 2634,2655 
  	if (!proc_exit_inprogress)
  	{
  		InterruptPending = true;
! 
! 		if (!DoingCommandRead)
! 			QueryCancelPending = true;
! 		else if (IsTransactionOrTransactionBlock())
! 			IdleTransactionCancelPending = true;
  
  		/*
  		 * If it's safe to interrupt, and we're waiting for input or a lock,
  		 * service the interrupt immediately
  		 */
  		if (ImmediateInterruptOK  InterruptHoldoffCount == 0 
! 			CritSectionCount == 0  QueryCancelPending)
! 			/*
! 			 * ProcessInterrupts() does nothing in case of
! 			 * IdleTransactionCancelPending, it is handled in PostgresMain
! 			 */
  		{
  			/* bump holdoff count to make ProcessInterrupts() a no-op */
  			/* until we are done getting ready for it */
*** PostgresMain(int argc, char 

Re: [HACKERS] Idle transaction cancel/timeout and SSL revisited

[Alex Shulgin]

Andres Freund and...@2ndquadrant.com writes:

 On 2014-11-15 00:11:36 +0300, Alex Shulgin wrote: 
 After reading up through archives on the two $subj related TODO items
 I'm under impression that the patches[1,2] didn't make it mainly because
 of the risk of breaking SSL internals if we try to longjump out of the
 signal handler in the middle of a blocking SSL read and/or if we try to
 report that final transaction/connection aborted notice to the client.

 I've written a patch that allows that - check
 http://archives.postgresql.org/message-id/20140927225421.GE5423%40alap3.anarazel.de

 I feel pretty confident that that's the way to go. I just need some time
 to polish it.

 What if instead of trying to escape from the signal handler we would set
 a flag and use it to simulate EOF after the recv() is restarted due to
 EINTR?  From the backend perspective it should look like the client has
 just hang up.

 That's pretty much what the above does. Except that it actually deals
 with blocking syscalls by not using them and relying on latches/select
 instead.

Neat, I must have missed it.

--
Alex


-- 
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] Libxml2 load error on Windows

[Alex Shulgin]

Dave Page dp...@pgadmin.org writes:

 On Tue, Jun 19, 2012 at 4:43 AM, Robert Haas robertmh...@gmail.com wrote:


 Please add this patch here so that it doesn't get lost in the shuffle:

 https://commitfest.postgresql.org/action/commitfest_view/open

 Hmm, that raises an interesting question (though maybe I've just
 missed this happening in the past). This is a bug fix, that we'll want
 in the next back branch updates, not just 9.3. Sticking it in the open
 commit fest means it may well not get looked at for 2-3 months or so.
 How do we prevent that happening?

In a real bug-tracking system we would create a new bug/ticket and set
it's target version to 'candidate for next minor release' or something
like that.  This way, if we don't release unless all targeted bugs are
resolved, this would be taken care of (hopefully.)

-- 
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] Libxml2 load error on Windows

[Alex Shulgin]

Dave Page dp...@pgadmin.org writes:

 On Tue, Jun 19, 2012 at 11:04 AM, Alex Shulgin a...@commandprompt.com wrote:

 In a real bug-tracking system we would create a new bug/ticket and set
 it's target version to 'candidate for next minor release' or something
 like that.  This way, if we don't release unless all targeted bugs are
 resolved, this would be taken care of (hopefully.)

 Well yes, but the point is that that is not how the project works. I'm
 asking how we do handle this problem at the moment, because I realised
 I haven't seen this happen in the past (largely because I haven't been
 paying attention).

It only works as long as there is some mechanism to ensure that the bugs
which were not submitted to commitfest are looked at.  If there is no,
then it doesn't work actually.

So is there a real and reliable mechanism for that?

--
Alex

-- 
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] libpq URL syntax vs SQLAlchemy

[Alex Shulgin]

Alex a...@commandprompt.com writes:

 Peter Eisentraut pete...@gmx.net writes:

 I have been reviewing how our new libpq URL syntax compares against
 existing implementations of URL syntaxes in other drivers or
 higher-level access libraries.  In the case of SQLAlchemy, there is an
 incompatibility regarding how Unix-domain sockets are specified.

 First, here is the documentation on that:
 http://docs.sqlalchemy.org/en/latest/dialects/postgresql.html

 The recommended way to access a server over a Unix-domain socket is to
 leave off the host, as in:

 postgresql://user:password@/dbname

 In libpq, this is parsed as host='/dbname', no database.

 Ah, good catch: thanks for heads up.

 I believe this was introduced lately in the dev cycle when we've noticed
 that users will have to specify some defaults explicitly to be able to
 override other defaults, while avoiding the whole ?keyword=value...
 business.

 I'll give this another look and will get back with a proposal to fix
 this in form of a patch.

Upon closer inspection of the issue I came to believe that the proper
fix is to drop support for special treatment of host part starting
with slash altogether.

Attached is a patch to do that.

While the following type of URIs is still valid, the interpretation is
now different and is standards-conforming: the part after the
double-slash is treated as path specification and not authority
(host.)

  postgres:///path-spec

Since the path from a URI translates into dbname connection option, the
only meaningful use of this type of URIs is the following:

  postgres:///mydb

The host part in this case is empty (it is hidden between the // and
the following /,) thus local socket connection is employed for this
type of URIs.  To specify non-standard path to the local sockets
directory use the familiar URI parameter:

  postgres:///db?host=/path/to/socket/dir


Also, if my reading of the RFC is correct, the username, password and
port specifiers may be omitted even if the corresponding designators are
present in URI, so we need to remove some checks on empty URI parts.

The test input and expected output files, the docs and code comments are
also updated, of course.

Finally, to complete the RFC compliance, I've added code to properly
handle percent-encoding in query parameter keywords.


At this point, I feel rather silly that we've produced and committed an
almost compliant version, which still requires quite a bit of
patching to become an RFC-conforming implementation...

--
Regards,
Alex



libpq-uri-rfc-compliance.patch
Description: libpq-uri-rfc-compliance.patch

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

Re: [HACKERS] Bug tracker tool we need

[Alex Shulgin]

Robert Haas robertmh...@gmail.com writes:

 On Tue, Apr 17, 2012 at 1:47 AM, Magnus Hagander mag...@hagander.net wrote:
 That's probably one reason people aren't jumping on this. Because
 there is no tracker out there that people actually *like*...

 I think this is a point worth serious thought.

I wonder why do people keep complaining how their bug tracker of choice
sucks, instead of doing something about that.  I can see a few possible
factors:

a) people do like to complain, and it's easier than submitting
meaningful bug reports or feature requests, patches :-)

b) the developers don't listen to their users, which happens far too
often unfortunately

c) (I had yet another idea here, but I forgot what it was :-p)

d) a wild mix of the above

However, this doesn't imply existing tracker software cannot be improved
and more of that must be written from scratch (unless the code is
cryptic and/or is written, probably poorly, in some rarely used
programming language, and is unmaintainable.)

Also, the reasons outlined above do not pertain only to bug tracker
software somehow: any piece of software could suffer from that and I
believe many of us have seen it.

So maybe there's something fundamentally wrong with every existing bug
tracker (e.g. they don't fix bugs for you?)  Well, just kidding. ;-)

--
Alex

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

Re: [HACKERS] Bug tracker tool we need

[Alex Shulgin]

Jay Levitt jay.lev...@gmail.com writes:

 (A quick Google shows redmine and especially Trac having spam issues
 of their own.)

Ugh, redmine (or trac for that matters) has nothing to with handling
spam.  I believe a typical bug tracker doesn't handle spam itself, it
lets the mailing system do that.

Surely you can throw in some captcha plugins to try to reduce the spam
posted from the web UI.

--
Alex

-- 
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] Last gasp

[Alex Shulgin]

Jay Levitt jay.lev...@gmail.com writes:

 No meaningful search, eh?  Works for me.

 Redmine searches return partial-word matches, and there's no way to
 disable that.  Searching for test finds latest. To me, that's
 broken.

Well, I believe one can plug in a different search engine, like lucene
or xapian.  However it doesn't look like some one already did (for
ticket/wiki history, but there's xapian search plugin[1] to index attachments.)

 Also, the UI is very 5 years ago; e.g., compare revisions uses the
 same columns-of-radio-buttons approach as MediaWiki. If the goal is a
 tool to reduce friction and increase involvement, you want a smoother
 UX.

Nothing that could not be tweaked with a plugin or core code
modification here either.  Not sure about the magnitude of the effort
required, though.

--
Alex

[1] https://github.com/xelkano/redmine_xapian

-- 
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] Another review of URI for libpq, v7 submission

[Alex Shulgin]

Heikki Linnakangas heikki.linnakan...@enterprisedb.com writes:

 On 22.03.2012 23:42, Alex wrote:
 Okay, at last here's v9, rebased against current master branch.

 Some quick comments on this patch:

Heikki, thank you for taking a look at this!

 I see a compiler warning:
 fe-connect.c: In function ‘conninfo_parse’:
 fe-connect.c:4113: warning: unused variable ‘option’

The warning is due to the earlier commit e9ce658b.  I believe the above
line supposed to go away.

 Docs are missing.

Yes, my plan is to focus on the documentation and code comments while
sorting out any remaining issues with the code.

 I wonder if you should get an error if you try specify the same option
 multiple times. At least the behavior needs to be documented.

Since conninfo strings may contain duplicated keywords and the latter
just takes precedence, I think we should just do the same with URIs
(which we already do.)  I don't see the behavior of conninfo strings
documented anywhere, however.

 Should %00 be forbidden?

Probably yes, good spot.

 The error message is a bit confusing for
 postgres://localhost?dbname=%XXfoo:
 WARNING: ignoring unrecognized URI query parameter: dbname
 There is in fact nothing wrong with dbname, it's the %XX in the
 value that's bogus.

Hm, yes, that's a bug.

Looks like conninfo_uri_parse_params needs to be adjusted to properly
pass the error message generated by conninfo_store_uri_encoded_value.  I
wonder if examining the errorMessage buffer to tell if it's a hard error
(it's going to be empty in the case of ignoreMissing=true) is a good
practice.

 Looking at conninfo_uri_decode(), I think it's missing a bounds check,
 and peeks at two bytes beyond the end of string if the input ends in a
 %'.

No, in that case what happens on L4919 is this: we dereference q which
is pointing at NUL terminator and pass the value to the first
get_hexdigit in the if condition, the pointer itself is then
incremented and does point beyond the end of string, but since
get_hexdigit returns FALSE we don't call the second get_hexdigit, so we
don't dereference the invalid pointer.

There is a comment right before that if which says just that.

--
Alex

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

Trivial libpq refactoring patch (was: Re: [HACKERS] Another review of URI for libpq, v7 submission)

[Alex Shulgin]

Alex a...@commandprompt.com writes:

 Marko Kreen mark...@gmail.com writes:

 On Thu, Mar 15, 2012 at 11:29:31PM +0200, Alex wrote:
 https://github.com/a1exsh/postgres/commits/uri

 The point of the patch is to have one string with all connection options,
 in standard format, yes?  So why does not this work:

   db = PQconnectdb(postgres://localhost);

 ?

 Good catch.

 I've figured out that we'll need a bit more intrusive change than simply
 overriding the expand_dbname check in conninfo_array_parse (like the
 current version does) to support URIs in all PQconnect* variants.

 I still need to figure out some details, but this is to give people a
 status update.

While working on this fix I've figured out that I need my
conninfo_uri_parse to support use_defaults parameter, like
conninfo(_array)_parse functions do.

The problem is that the block of code which does the defaults handling
is duplicated in both of the above functions.  What I'd like to do is
extract it into a separate function to call.  What I wouldn't like is
bloating the original URI patch with this unrelated change.

So here's a trivial patch to do the refactoring.  Also, it uses the
newly added conninfo_fill_defaults directly in PQconndefaults, instead
of doing the parse empty conninfo string trick.  If this could be
applied, I'd rebase my patch against the updated master branch and
submit the new version.

As it goes, the patch adds a little duplication when it comes to
creating a working copy of PQconninfoOptions array.  Attached is the
second patch on top of the first one to extract this bit of code into
conninfo_init.  Not sure if we should go all the way through this, so
it's not critical if this one is not applied.

--
Regards,
Alex
*** a/src/interfaces/libpq/fe-connect.c
--- b/src/interfaces/libpq/fe-connect.c
***
*** 297,302  static PQconninfoOption *conninfo_parse(const char *conninfo,
--- 297,304 
  static PQconninfoOption *conninfo_array_parse(const char *const * keywords,
 const char *const * values, 
PQExpBuffer errorMessage,
 bool use_defaults, int expand_dbname);
+ static bool conninfo_fill_defaults(PQconninfoOption *options,
+  PQExpBuffer errorMessage);
  static char *conninfo_getval(PQconninfoOption *connOptions,
const char *keyword);
  static void defaultNoticeReceiver(void *arg, const PGresult *res);
***
*** 836,842  PQconndefaults(void)
initPQExpBuffer(errorBuf);
if (PQExpBufferDataBroken(errorBuf))
return NULL;/* out of memory already :-( */
!   connOptions = conninfo_parse(, errorBuf, true);
termPQExpBuffer(errorBuf);
return connOptions;
  }
--- 838,860 
initPQExpBuffer(errorBuf);
if (PQExpBufferDataBroken(errorBuf))
return NULL;/* out of memory already :-( */
! 
!   /* Make a working copy of PQconninfoOptions */
!   connOptions = malloc(sizeof(PQconninfoOptions));
!   if (connOptions == NULL)
!   {
!   printfPQExpBuffer(errorBuf,
! libpq_gettext(out of 
memory\n));
!   return NULL;
!   }
!   memcpy(connOptions, PQconninfoOptions, sizeof(PQconninfoOptions));
! 
!   if (!conninfo_fill_defaults(connOptions, errorBuf))
!   {
!   PQconninfoFree(connOptions);
!   connOptions = NULL;
!   }
! 
termPQExpBuffer(errorBuf);
return connOptions;
  }
***
*** 4002,4008  conninfo_parse(const char *conninfo, PQExpBuffer 
errorMessage,
char   *pname;
char   *pval;
char   *buf;
-   char   *tmp;
char   *cp;
char   *cp2;
PQconninfoOption *options;
--- 4020,4025 
***
*** 4170,4245  conninfo_parse(const char *conninfo, PQExpBuffer 
errorMessage,
free(buf);
  
/*
!* Stop here if caller doesn't want defaults filled in.
!*/
!   if (!use_defaults)
!   return options;
! 
!   /*
!* If there's a service spec, use it to obtain any not-explicitly-given
!* parameters.
 */
!   if (parseServiceInfo(options, errorMessage))
{
PQconninfoFree(options);
return NULL;
}
  
-   /*
-* Get the fallback resources for parameters not specified in the 
conninfo
-* string nor the service.
-*/
-   for (option = options; option-keyword != NULL; option++)
-   {
-   if (option-val != NULL)
-   continue;   /* Value was in 
conninfo or service */
- 
-   /*
-* Try to get the environment variable fallback
-*/
-   if (option-envvar != NULL)
-   {

Re: [HACKERS] Another review of URI for libpq, v7 submission

[Alex Shulgin]

Daniel Farina dan...@heroku.com writes:

 I reviewed this and so far have not found any serious problems,
 although as is par for the course it contains some of the fiddly bits
 involved in any string manipulations in C.  I made a few edits -- none
 strictly necessary for correctness -- that the original author is free
 audit and/or include[0].

Thank you for the review, Daniel!

Apparently, I was on drugs when I've submitted v7, as it still contains
the bug for which to fix I was forced to move parts of the code back
into the main parser routine...

 I did put in some defensive programming choices (instead of if/else
 if/elseif/else raise an error, even if the latter is allegedly
 impossible) that I think are a good idea.

Yes, this is a good idea, I'll incorporate them in the patch.  However,
this one doesn't work:
https://github.com/fdr/postgres/commit/4fad90fb243d9266b1003cfbcf8397f67269fad3

Neither '@' or '/' are mandatory in the URI anywhere after scheme://,
so we can't just say it should never happen.  Running the regression
test with this commit applied highlights the problem.

 One thing I found puzzling was that in the latest revision the tests
 appeared to be broken for me: all @ signs were translated to (at).
  Is that mangling applied by the archives, or something?

This is definitely mangling by the lists.  I can see this has happened
to people before, e.g.:
http://archives.postgresql.org/pgsql-hackers/2010-01/msg00938.php

But that discussion didn't seem to lead to a solution for the problem.

I wonder if there's any evidence as to that mangling the email addresses
helps to reduce spam at all?  I mean replacing (at) back to @ and
(dot) to . is piece of cake for a spam crawler.

What I see though, is that most of the message-id URLs are broken by the
mangling.  Also I don't think everyone should just tolerate that it
messes with the attachments.  Should we all suddenly use
application/octet-stream or application/gzip instead of text/plain?

 The test suite neatly tries to copy pg_regress's general make
 installcheck style, but it likes to use my username as the database
 rather than the standard regression as seen by pg_regress.  It is
 nice that a place to test connection strings and such is there, where
 there was none before.

Oh, I don't see why we can't use regression too.

While testing this I've also noticed that there was some funny behavior
when you left out the final slash after hostname, e.g.:
postgres://localhost/ vs. postgres://localhost.  It turned out in
the former case we were setting dbname conninfo parameter to an empty
string and in the latter one we didn't set it at all.  The difference is
that when we do set it, the default dbname is derived from username, but
when we don't--$PGDATABASE is used.  I've fixed this, so now both URIs
work the same way (both do not set dbname.)

It also appeared to me that with the current code, a wide range of
funny-looking URIs are considered valid, e.g.:

postgres://user@
postgres://@host
postgres://host:/

and, taking approach to the extreme:

postgres://:@:

This specifies empty user, password, host and port, and looks really
funny.  I've added (actually revived) some checks to forbid setting
empty URI parts where it doesn't make much sense.

Finally, attached is v8.  Hopefully I didn't mess things up too much.

--
Regards,
Alex



binaKreQKSDSW.bin
Description: libpq-uri-v8.patch

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

Re: [HACKERS] WIP: URI connection string support for libpq

[Alex Shulgin]

Peter Eisentraut pete...@gmx.net writes:

 I've included a (separate) test shell script based on Greg's cases in 
 one of the updates.  What would be preferred place to plug it in? 
 Override installcheck in libpq Makefile?

 I think that would be the right place.

I figured that adding this right into src/interfaces/libpq is polluting
the source dir, so I've used src/test instead.

Attached v6 adds src/test/uri directory complete with the test script,
expected output file and a Makefile which responds to installcheck.
README file also included.

--
Alex

*** a/src/interfaces/libpq/fe-connect.c
--- b/src/interfaces/libpq/fe-connect.c
***
*** 282,287  static const PQEnvironmentOption EnvironmentOptions[] =
--- 282,290 
}
  };
  
+ /* The connection URI must start with either of the following designators: */
+ static const char uri_designator[] = postgresql://;
+ static const char short_uri_designator[] = postgres://;
  
  static bool connectOptions1(PGconn *conn, const char *conninfo);
  static bool connectOptions2(PGconn *conn);
***
*** 297,303  static PQconninfoOption *conninfo_parse(const char *conninfo,
  static PQconninfoOption *conninfo_array_parse(const char *const * keywords,
 const char *const * values, 
PQExpBuffer errorMessage,
 bool use_defaults, int expand_dbname);
! static char *conninfo_getval(PQconninfoOption *connOptions,
const char *keyword);
  static void defaultNoticeReceiver(void *arg, const PGresult *res);
  static void defaultNoticeProcessor(void *arg, const char *message);
--- 300,333 
  static PQconninfoOption *conninfo_array_parse(const char *const * keywords,
 const char *const * values, 
PQExpBuffer errorMessage,
 bool use_defaults, int expand_dbname);
! static PQconninfoOption *conninfo_uri_parse(const char *uri,
!   PQExpBuffer errorMessage);
! static bool conninfo_uri_parse_options(PQconninfoOption *options,
!   const char *uri, char *buf,
!   PQExpBuffer errorMessage);
! static char *conninfo_uri_parse_local_socket_dir(PQconninfoOption *options,
!   const char *uri,
!   char *buf, char *lastc,
!   PQExpBuffer errorMessage);
! static char *conninfo_uri_parse_remote_host(PQconninfoOption *options,
!   const char *uri,
!   char *buf, char *lastc,
!   PQExpBuffer errorMessage);
! static bool conninfo_uri_parse_params(char *params,
!   PQconninfoOption *connOptions,
!   PQExpBuffer errorMessage);
! static char *conninfo_uri_decode(const char *str, PQExpBuffer errorMessage);
! static bool get_hexdigit(char digit, int *value);
! static const char *conninfo_getval(PQconninfoOption *connOptions,
!   const char *keyword);
! static PQconninfoOption *conninfo_storeval(PQconninfoOption *connOptions,
!   const char *keyword, const char *value,
!   PQExpBuffer errorMessage, bool ignoreMissing);
! static PQconninfoOption *conninfo_store_uri_encoded_value(
!   PQconninfoOption *connOptions,
!   const char *keyword, const char *encoded_value,
!   PQExpBuffer errorMessage, bool ignoreMissing);
! static PQconninfoOption *conninfo_find(PQconninfoOption *connOptions,
const char *keyword);
  static void defaultNoticeReceiver(void *arg, const PGresult *res);
  static void defaultNoticeProcessor(void *arg, const char *message);
***
*** 580,586  PQconnectStart(const char *conninfo)
  static void
  fillPGconn(PGconn *conn, PQconninfoOption *connOptions)
  {
!   char   *tmp;
  
/*
 * Move option values into conn structure
--- 610,616 
  static void
  fillPGconn(PGconn *conn, PQconninfoOption *connOptions)
  {
!   const char *tmp;
  
/*
 * Move option values into conn structure
***
*** 3741,3747  ldapServiceLookup(const char *purl, PQconninfoOption 
*options,
  static int
  parseServiceInfo(PQconninfoOption *options, PQExpBuffer errorMessage)
  {
!   char   *service = conninfo_getval(options, service);
charserviceFile[MAXPGPATH];
char   *env;
boolgroup_found = false;
--- 3771,3777 
  static int
  parseServiceInfo(PQconninfoOption *options, PQExpBuffer errorMessage)
  {
!   const char *service = conninfo_getval(options, service);
charserviceFile[MAXPGPATH];
char   *env;
bool 

Re: [HACKERS] WIP: URI connection string support for libpq

[Alex Shulgin]

Greg Smith g...@2ndquadrant.com writes:

Thank you for the review, Greg!

 Given that, there really isn't a useful path forward that helps out
 all those developers without supporting both prefixes.  That's where
 this left off before, I just wanted to emphasize how clear that need
 seems now.

OK, I've used the code from your earlier review to support the short
prefix.  I sincerely hope we don't make the situation any worse by being
flexible about the prefix...

 Next thing, also mentioned at that Flask page.  SQLite has
 standardized the idea that sqlite:absolute/path/to/foo.db is a URI
 pointing to a file.  Given that, I wonder if Alex's syntax for
 specifying a socket file name might adopt that syntax, rather than
 requiring the hex encoding:  postgresql://%2Fvar%2Fpgsql%2Ftmp/mydb
 It's not a big deal, but it would smooth another rough edge toward
 making the Postgres URI implementation look as close as possible to
 others.

Yeah, this is really appealing, however how do you tell if the part
after the last slash is a socket directory name or a dbname?  E.g:

psql postgres:///path/to/different/socket/dir(default dbname)
psql postgres:///path/to/different/socket/dir/other  (dbname=other ?)

If we treat the whole URI string as the path to the socket dir (which I
find the most intuitive way to do it,) the only way to specify a
non-default dbname is to use query parameter:

psql postgres:///path/to/different/socket/dir?dbname=other

or pass another -d flag to psql *after* the URI:

psql [-d] postgres:///path/to/different/socket/dir -d other

Reasonable?

 So far I've found only one syntax that I expected this to handle that
 it rejects:

 psql -d postgresql://gsmith@localhost

 It's picky about needing that third slash, but that shouldn't be hard
 to fix.

Yeah, good that you've spotted it.  If my reading of the URI RFC (2396)
is correct, the question mark and query parameters may follow the
hostname, w/o that slash too, like this:

psql -d postgresql://localhost?user=gsmith

So this made me relax some checks and rewrite the code a bit.

 I started collecting up all the variants that do work as an initial
 shell script regression test, so that changes don't break something
 that already works.  Here are all the variations that already work,
 setup so that a series of 1 outputs is passing:

[snip]

Yes, the original code was just a bit too picky about URI component
separators.  Attached also is a simplified test shell script.

I have also added a warning message for when a query parameter is not
recognized and being ignored.  Not sure if plain fprintf to stderr is
accepted practice for libpq, please correct if you have better idea.

--
Regards,
Alex

*** a/src/interfaces/libpq/fe-connect.c
--- b/src/interfaces/libpq/fe-connect.c
***
*** 282,287  static const PQEnvironmentOption EnvironmentOptions[] =
--- 282,290 
  	}
  };
  
+ /* The connection URI must start with one the following designators: */
+ static const char uri_designator[] = postgresql://;
+ static const char short_uri_designator[] = postgres://;
  
  static bool connectOptions1(PGconn *conn, const char *conninfo);
  static bool connectOptions2(PGconn *conn);
***
*** 297,303  static PQconninfoOption *conninfo_parse(const char *conninfo,
  static PQconninfoOption *conninfo_array_parse(const char *const * keywords,
  	 const char *const * values, PQExpBuffer errorMessage,
  	 bool use_defaults, int expand_dbname);
! static char *conninfo_getval(PQconninfoOption *connOptions,
  const char *keyword);
  static void defaultNoticeReceiver(void *arg, const PGresult *res);
  static void defaultNoticeProcessor(void *arg, const char *message);
--- 300,325 
  static PQconninfoOption *conninfo_array_parse(const char *const * keywords,
  	 const char *const * values, PQExpBuffer errorMessage,
  	 bool use_defaults, int expand_dbname);
! static PQconninfoOption *conninfo_uri_parse(const char *uri,
! PQExpBuffer errorMessage);
! static bool conninfo_uri_parse_options(PQconninfoOption *options,
! const char *uri, char *buf,
! PQExpBuffer errorMessage);
! static bool conninfo_uri_parse_params(char *params,
! PQconninfoOption *connOptions,
! PQExpBuffer errorMessage);
! static char *conninfo_uri_decode(const char *str, PQExpBuffer errorMessage);
! static bool get_hexdigit(char digit, int *value);
! static const char *conninfo_getval(PQconninfoOption *connOptions,
! const char *keyword);
! static PQconninfoOption *conninfo_storeval(PQconninfoOption *connOptions,
! const char *keyword, const char *value,
! PQExpBuffer errorMessage, bool ignoreMissing);
! static PQconninfoOption *conninfo_store_uri_encoded_value(
! PQconninfoOption *connOptions,
! const char *keyword, const char *encoded_value,
! PQExpBuffer errorMessage, bool ignoreMissing);
! static PQconninfoOption *conninfo_find(PQconninfoOption *connOptions,
  const char *keyword);
  static 

Re: [HACKERS] WIP: URI connection string support for libpq

[Alex Shulgin]

Florian Weimer fwei...@bfk.de writes:

 * Alex Shulgin:

 Yeah, this is really appealing, however how do you tell if the part
 after the last slash is a socket directory name or a dbname?  E.g:

 psql postgres:///path/to/different/socket/dir(default dbname)
 psql postgres:///path/to/different/socket/dir/other  (dbname=other ?)

 The HTTP precent is to probe the file system until you find something.
 Most HTTP servers have something similar to the PATH_INFO variable which
 captures trailing path segments.

 It's ugly, but it's standard practice, and seems better than a separate
 -d parameter (which sort of defeats the purpose of URIs).

Hm, do you see anything what's wrong with ?dbname=other if you don't
like a separate -d?

--
Alex

-- 
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] Reviewing patch URI connection string support for libpq

[Alex Shulgin]

Harold Giménez harold.gime...@gmail.com writes:

I have interest in the URI connection string support patch[1], so I'm
in the process of reviewing it. I have a couple of comments and
questions:

Hello, thank you for your interest and the review!

1. I see no tests in the patch. I'd like to start getting together a
set of tests, likely based on the connection string permutations found
on Greg Smith's response[2]. However I don't find an obvious place to
put them. They could possibly live in the test/examples directory.
Another thought is to use dblink in a test, although it may be
problematic to depend on a contrib package for a test, to say the
least. Any thoughts on how to test this are most welcome.

I was also curious on how to add any sort of regression testing (likely
using psql,) but didn't get any advice as far as I can recall.

2. The documentation/manual was not updated as part of this patch, so
this is pending.

I've marked the patch as Work-In-Progress and specifically omitted
documentation changes until we settle on functionality.

3. I for one do prefer the `postgres` prefix, as opposed to
`postgresql` for the reasons stated on an earlier thread [3]. In my
opinion the best way to move forward is to support them both.

The updated v4 version of the patch does cover this:
http://archives.postgresql.org/pgsql-hackers/2012-02/msg01141.php

--
Alex

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

[HACKERS] Review of pg_archivecleanup -x option patch

[Alex Shulgin]

Hello,

This is my first patch review ever, so please bear with me.

The patch[1] is in the context diff format and applies cleanly to
current git HEAD.  Documentation is updated by the patch.

The code does implement what's the patch is supposed to do.

Do we want that?  According to Greg's original mail, yes.

One problem I've found while trying the example workflow is this:

~/local/postgresql/HEAD$ ./bin/pg_archivecleanup -d -x .gz data/archive/ 
00010002.0020.backup.gz
pg_archivecleanup: invalid filename input
Try pg_archivecleanup --help for more information.

I think we could accept the suffixed WAL filename and strip the suffix;
another idea is to actually detect the suffix (if any) from the last
command line argument, thus rendering the -x option unnecessary.

By the way, I for one would use the word 'suffix' instead of 'extension'
which sounds like some MS-DOS thingy, but after briefly grepping the
docs I can see that both words are used in this context already (and the
ratio is not in favor of my choice of wording.)

Other than that the patch looks good.

--
Alex

[1] http://archives.postgresql.org/pgsql-hackers/2011-02/msg00547.php

-- 
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] automating CF submissions (was xlog location arithmetic)

[Alex Shulgin]

Greg Smith g...@2ndquadrant.com writes:

 One unicorn I would like to have here would give the CF app a database
 of recent e-mails to pgsql-hackers.  I login to the CF app, click on
 Add recent submission, and anything matching my e-mail address
 appears with a checkbox next to it.  Click on the patch submissions,
 and then something like you described would happen.  That would save
 me the annoying work around looking up message IDs so much.

Another idea: introduce some simple tag system in mails sent to -hackers
to be treated specially, e.g:

@fest add-to-current

to add new patch to the commit fest currently in progress, or

@fest add-to-next

to add it to the next scheduled fest.

Attribute your mail with

@fest comment COMMENT TEXT

or

@fest comment EOF
...
EOF

to add a (long) comment, ditto for patch and review.

How does that sound?

--
Alex

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

Re: [HACKERS] Postgres ReviewFest Patch: URI connection string support for libpq

[Alex Shulgin]

Nick Roosevelt nro...@gmail.com writes:

I am sorry, seems like my new MUA was misconfigured so the two previous
attempts to reply to -hackers@ failed.  So here goes again.

Just reviewed the patch for adding URI connection string support for
libpg.

Thank you for taking your time on this.

There seem to be many tabs in the patch.  Perhaps the indentation is
not correct.

I believe tabs for indentation is the project's standard.

Also, the patch did not run correctly:
patching file src/interfaces/libpq/fe-connect.c
Hunk #1 succeeded at 282 with fuzz 1.
Hunk #2 FAILED at 300.
Hunk #3 FAILED at 583.
Hunk #4 FAILED at 3742.
Hunk #5 FAILED at 4132.
Hunk #6 FAILED at 4279.
Hunk #7 FAILED at 4455.
6 out of 7 hunks FAILED -- saving rejects to file
src/interfaces/libpq/fe-connect.c.rej
Seems like the file has changed since this patch was created.  Please
recreate the patch.

I've just tried to apply the original patch against a clean checkout of
master branch and it applies without a problem (no hunk failed, no
fuzziness.)

Could you please try again on a clean master branch?

--
Regards,
Alex

-- 
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] automating CF submissions (was xlog location arithmetic)

[Alex Shulgin]

Andrew Dunstan and...@dunslane.net writes:

 On 01/19/2012 12:59 PM, Alex Shulgin wrote:

 Another idea: introduce some simple tag system in mails sent to -hackers
 to be treated specially, e.g:

 @fest add-to-current

 to add new patch to the commit fest currently in progress, or

 @fest add-to-next

 to add it to the next scheduled fest.

 Attribute your mail with

 @fest comment COMMENT TEXT

 or

 @fest commentEOF
 ...
 EOF

 to add a (long) comment, ditto for patch and review.

 How does that sound?


 Like a recipe for something that requires constant fixups, to be honest.

 Seriously, adding something to the CF isn't *that* hard. I like Greg's
 idea of a list of recent emails that you could choose from.

I've just added a comment about a patch and it took me to:

a. Login to commitfest app
b. Locate the patch and review I was replying to
c. Fetch archives thread index, refresh the index page for ~10 minutes
to see my reply appear
d. Copy message id and finally register comment in the commitfest app

(IIRC, something close to that was already described in this thread)

With the proposed approach it would only take me to include

@fest comment Patch applies cleanly

and possibly

@fest status Needs Review

to update the patch status and that'd be it.

--
Alex

PS: yes, I could just copy message id from the sent mail in my MUA, but
I like to make sure links I post aren't broke, so still I'll need to
wait until archives catches up to double check.

-- 
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] Should we get rid of custom_variable_classes altogether?

[Alex Shulgin]

On Mon, Oct 3, 2011 at 00:05, Tom Lane t...@sss.pgh.pa.us wrote:

 So at this point I'd vote for just dropping it and always allowing
 custom (that is, qualified) GUC names to be set, whether the prefix
 corresponds to any loaded module or not.

 Comments, other proposals?

While working on E.164 telephone numbers datatype contrib module
(https://github.com/commandprompt/e164/commits/guc) I've stumbled
across this problem: how do I add regression tests involving the
module-defined GUC option?

Trying to hack postgresql.conf to include e164 in the
custom_variable_classes then send it a HUP doesn't seem to be an
option.  But it seems that you cannot (re)set it otherwise.  See:

$ psql -d contrib_regression
psql (9.1.0)
Type help for help.

contrib_regression=# SET e164.area_codes_format='';
ERROR:  unrecognized configuration parameter e164.area_codes_format
contrib_regression=# SET custom_variable_classes='e164';
ERROR:  parameter custom_variable_classes cannot be changed now

I wonder how/if other contrib modules ever do regression tests on
their GUC options?

At this rate, removing the custom_variable_classes option altogether
is pretty much going to solve my problem.

--
Regards,
Alex

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