Re: percentile value check can be slow

[David Fetter]

On Sat, Nov 18, 2017 at 11:05:47PM +0100, Tomas Vondra wrote:
> Hi,
> 
> On 11/18/2017 10:30 PM, David Fetter wrote:
> > On Sat, Nov 18, 2017 at 10:44:36AM -0500, Tom Lane wrote:
> >> jotpe  writes:
> >>> I tried to enter invalid percentile fractions, and was astonished
> >>> that it seems to be checked after many work is done?
> >>
> >> IIRC, only the aggregate's final-function is concerned with direct
> >> arguments, so it's not all that astonishing.
> > 
> > It may not be surprising from the point of view of a systems
> > programmer, but it's pretty surprising that this check is deferred to
> > many seconds in when the system has all the information it need in
> > order to establish this before execution begins.
> > 
> > I'm not sure I see an easy way to do this check early, but it's worth
> > trying on grounds of POLA violation.  I have a couple of ideas on how
> > to do this, one less invasive but hinky, the other a lot more invasive
> > but better overall.
> > 
> > Ugly Hack With Ugly Performance Consequences:
> > Inject a subtransaction at the start of execution that supplies an
> > empty input to the final function with the supplied direct
> > arguments.
> 
> I'm pretty sure you realize this is quite unlikely to get accepted.

I should hope not, but I mentioned it because I'd like to get it on
the record as rejected.

> > Bigger Lift:
> > Require a separate recognizer function direct arguments and fire
> > it during post-parse analysis.  Perhaps this could be called
> > recognizer along with the corresponding mrecognizer.  It's not
> > clear that it's sane to have separate ones, but I thought I'd
> > bring it up for completeness.
> > 
> 
> Is 'recognizer' an established definition I should know? Is it the same
> as 'validator' or is it something new/different?

I borrowed it from http://langsec.org/

I'm not entirely sure what you mean by a validator, but a recognizer
is something that gives a quick and sure read as to whether the input
is well-formed. In general, it's along the lines of a tokenizer, a
parser, and something that does very light post-parse analysis for
correctness of form.

For the case that started the thread, a recognizer would check
something along the lines of 

CHECK('[0,1]' @> ALL(input_array))

> > Way Bigger Lift, As Far As I Can Tell, But More Fun For Users:
> > Allow optional CHECK constraints in CREATE AGGREGATE for direct
> > arguments.
> > 
> 
> How will any of the approaches deal with something like
> 
> select percentile_cont((select array_agg(v) from p))
>within group (order by a) from t;
> 
> In this case the the values are unknown after the parse analysis, so I
> guess it does not really address that.

It doesn't.  Does it make sense to do a one-shot execution for cases
like that?  It might well be worth it to do the aggregate once in
advance as a throw-away if the query execution time is already going
to take awhile.  Of course, you can break that one by making p a JOIN
to yet another thing...

> FWIW while I won't stand in the way of improving this, I wonder if this
> is really worth the additional complexity. If you get errors like this
> with a static list of values, you will fix the list and you're done. If
> the list is dynamic (computed in the query itself), you'll still get the
> error much later during query execution.
> 
> So if you're getting many failures like this for the "delayed error
> reporting" to be an issue, perhaps there's something wrong in you stack
> and you should address that instead?

I'd like to think that getting something to fail quickly and cheaply
when it can will give our end users a better experience.  Here,
"cheaply" refers to their computing resources and time.  Clearly, not
having this happen in this case bothered Johannes enough to wade in
here.

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

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

Re: [HACKERS] pg_basebackup --progress output for batch execution

[Tomas Vondra]

On 11/10/2017 02:32 PM, Martín Marqués wrote:
> Hi,
> 
> Thanks for having a look at this patch.
> 
> 2017-11-09 20:55 GMT-03:00 Jeff Janes :
>> On Fri, Sep 29, 2017 at 4:00 PM, Martin Marques
>>  wrote:
>>>
>>> Hi,
>>>
>>> Some time ago I had to work on a system where I was cloning a standby
>>> using pg_basebackup, that didn't have screen or tmux. For that reason I
>>> redirected the output to a file and ran it with nohup.
>>>
>>> I normally (always actually ;) ) run pg_basebackup with --progress and
>>> --verbose so I can follow how much has been done. When done on a tty you
>>> get a nice progress bar with the percentage that has been cloned.
>>>
>>> The problem came with the execution and redirection of the output, as
>>> the --progress option will write a *very* long line!
>>>
>>> Back then I thought of writing a patch (actually someone suggested I do
>>> so) to add a --batch-mode option which would change the behavior
>>> pg_basebackup has when printing the output messages.
>>
>>
>>
>> While separate lines in the output file is better than one very long line,
>> it still doesn't seem so useful.  If you aren't watching it in real time,
>> then you really need to have a timestamp on each line so that you can
>> interpret the output.  The lines are about one second apart, but I don't
>> know robust that timing is under all conditions.
> 
> I kind of disagree with your view here.
> 
> If the cloning process takes many hours to complete (in my case, it
> was around 12 hours IIRC) you might want to peak at the log every now
> and then with tail.
> 
> I do agree on adding a timestamp prefix to each line, as it's not
> clear from the code how often progress_report() is called.
> 
>> I think I agree with Arthur that I'd rather have the decision made by
>> inspecting whether output is going to a tty, rather than by adding another
>> command line option.  But maybe that is not detected robustly enough across
>> all platforms and circumstances?
> 
> In this case, Arthur's idea is good, but would make the patch less 
> generic/flexible for the end user.
> 

I'm not quite sure about that. We're not getting the flexibility for
free, but for additional complexity (additional command-line option).
And what valuable capability would we (or the users) loose, really, by
relying on the isatty call?

So what use case is possible with --batch-mode but not with isatty (e.g.
when combined with tee)?

>
> That's why I tried to reproduce what top does when executed with -b 
> (Batch mode operation). There, it's the end user who decides how the 
> output is formatted (well, saying it decides on formatting a bit of
> an overstatement, but you get it ;) )
> 

In 'top' a batch mode also disables input, and runs for a certain number
of interactions (as determined by '-n' option). That's not the case in
pg_basebackup, where it only adds the extra newline.

>
> An example where using isatty() might fail is if you run 
> pg_basebackup from a tty but redirect the output to a file, I
> believe that in that case isatty() will return true, but it's very
> likely that the user might want batch mode output.
> 

IMHO that should work correctly, as already explained by Arthur.

>
> But maybe we should also add Arthurs idea anyway (when not in batch 
> mode), as it seems pretty lame to output progress in one line if you 
> are not in a tty.
> 

I'd say to just use isatty, unless we can come up with a compelling use
case that is not satisfied by it.

And if we end up using --batch-mode, perhaps we should only allow it
when --progress is used. It's the only thing it affects, and it makes no
difference when used without it.

Furthermore, I propose to reword this:

  
   Runs pg_basebackup in batch mode. This is useful
   if the output is to be pipped so the other end of the pipe reads each
   line.
  
  
   Using this option with --progress will result in
   printing each progress output with a newline at the end, instead of a
   carrige return.
  

like this:

  
   Runs pg_basebackup in batch mode, in which
   --progress terminates the lines with a regular
   newline instead of carriage return.
  
  
   This is useful if the output is redirected to a file or a pipe,
   instead of a plain console.
  

regards

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

Re: [HACKERS] new function for tsquery creartion

[Tomas Vondra]

Hi,

On 09/13/2017 10:57 AM, Victor Drobny wrote:
> On 2017-09-09 06:03, Thomas Munro wrote:
>> Please send a rebased version of the patch for people to review and
>> test as that one has bit-rotted.
> Hello,
> Thank you for interest. In the attachment you can find rebased
> version(based on 69835bc8988812c960f4ed5aeee86b62ac73602a commit)
> 

I did a quick review of the patch today. The patch unfortunately no
longer applies, so I had to use an older commit from September. Please
rebase to current master.

I've only looked on the diff at this point, will do more testing once
the rebase happens.

Some comments:

1) This seems to mix multiple improvements into one patch. There's the
support for alternative query syntax, and then there are the new
operators (AROUND and ). I propose to split the patch into two or
more parts, each addressing one of those bits.

I guess there will be two or three parts - first adding the syntax,
second adding  and third adding the AROUND(n). Seems reasonable?

2) I don't think we should mention Google in the docs explicitly. Not
that I'm somehow anti-google, but this syntax was certainly not invented
by Google - I vividly remember using something like that on Altavista
(yeah, I'm old). And it's used by pretty much every other web search
engine out there ...

3) In the SGML docs, please use  instead of just
quoting the values. So it should be | instead of '|'
etc. Just like in the parts describing plainto_tsquery, for example.

4) Also, I recommend adding a brief explanation what the examples do.
Right now there's just a bunch of queryto_tsquery, and the reader is
expected to understand the output. I suggest adding a sentence or two,
explaining what's happening (just like for plainto_tsquery examples).

5) I'm not sure about negative values in the  operator. I don't
find it particularly confusing - once you understand that (a  b)
means "there are 'k' words between 'a' and 'b' (n <= k <= m)", then
negative values seem like a fairly straightforward extension.

But I guess the main question is "Is there really a demand for the new
 operator, or have we just invented if because we can?"

6) There seem to be some new constants defined, with names not following
the naming convention. I mean this

#define WAITOPERAND 1
#define WAITOPERATOR2
#define WAITFIRSTOPERAND3
#define WAITSINGLEOPERAND   4
#define INSIDEQUOTES5   <-- the new one

and

#define TSPO_L_ONLY0x01
#define TSPO_R_ONLY0x02
#define TSPO_BOTH  0x04
#define TS_NOT_EXAC0x08 <-- the new one

Perhaps that's OK, but it seems a bit inconsistent.


regards

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

Re: Logical Replication and triggers

[Craig Ringer]

On 15 November 2017 at 21:12, Thomas Rosenstein <
thomas.rosenst...@creamfinance.com> wrote:

> I would like somebody to consider Petr Jelineks patch for worker.c from
> here (https://www.postgresql.org/message-id/619c557d-93e6-1833-16
> 92-b010b176ff77%402ndquadrant.com)
>
> I'm was facing the same issue with 10.1 and BEFORE INSERT OR UPDATE
> triggers.
>

Please:

- Apply it to current HEAD
- Test its functionality
- Report back on the patch thread
- Update the commitfest app with your results and sign on as a reviewer
- If you're able, read over the patch and make any comments you can

"Somebody" needs to be you, if you want this functionality.


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

Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

[Michael Paquier]

On Sun, Nov 19, 2017 at 12:56 AM, Peter Eisentraut
 wrote:
> On 11/18/17 06:32, Michael Paquier wrote:
>> +   cbind_header_len = 4 + strlen(state->channel_binding_type); /*
>> p=type,, */
>> +   cbind_input_len = cbind_header_len + cbind_data_len;
>> +   cbind_input = malloc(cbind_input_len);
>> +   if (!cbind_input)
>> +   goto oom_error;
>> +   snprintf(cbind_input, cbind_input_len, "p=%s",
>> state->channel_binding_type);
>> +   memcpy(cbind_input + cbind_header_len, cbind_data, cbind_data_len);
>> By looking at RFC5802, a base64 encoding of cbind-input is used:
>> cbind-input   = gs2-header [ cbind-data ]
>> gs2-cbind-flag "," [ authzid ] ","
>> However you are missing two commands after p=%s, no?
>
> fixed

s/commands/commas/. You caught my words correctly.

> I have committed the patch with the above fixes.

Thanks, Peter!

> I'll be off for a week, so perhaps by that time you could make a rebased
> version of the rest?  I'm not sure how much more time I'll have, so
> maybe it will end up being moved to the next CF.

OK, let's see then. That's not an issue for me if this gets bumped.
-- 
Michael

Re: [HACKERS] ginInsertCleanup called from vacuum could still miss tuples to be deleted

[Peter Geoghegan]

On Thu, Nov 16, 2017 at 2:16 PM, Jeff Janes  wrote:
> e2c79e14 was to fix a pre-existing bug, but probably e9568083 made that bug
> easier to hit than it was before.  (Which is not to say that  e9568083 can't
> contain bugs of its own, of course)

I get that. I think that the same thing may have happened again, in
fact. A pending list recycling interlock bug may have merely been
unmasked by your commit e9568083; I'm speculating that your commit
made it more likely to hit in practice, just as with the earlier bug
you mentioned.

As you know, there is a significant history of VACUUM page recycling
bugs in GIN. I wouldn't be surprised if we found one more in the
pending list logic. Consider commit ac4ab97e, a bugfix from late 2013
for posting list page deletion, where the current posting list locking
protocol was more or less established. The commit message says:

"""
...
If a page is deleted, and reused for something else, just as a search is
following a rightlink to it from its left sibling, the search would continue
scanning whatever the new contents of the page are. That could lead to
incorrect query results, or even something more curious if the page is
reused for a different kind of a page.

To fix, modify the search algorithm to lock the next page before releasing
the previous one, and refrain from deleting pages from the leftmost branch
of the [posting] tree.
...
"""

I believe that this commit had GIN avoid deletion of the leftmost
branch of posting trees because that makes it safe to get to the
posting tree root from a raw root block number (a raw block number
from the B-tree index constructed over key values). The buffer lock
pairing/crabbing this commit adds to posting lists (similar to the
pairing/crabbing that pending lists had from day one, when first added
in 2011) can prevent a concurrent deletion once you reach the posting
tree root. But, as I said, you still need to reliably get to the root
in the first place, which the "don't delete leftmost" rule ensures (if
you can never delete it, you can never recycle it, and no reader gets
confused).

It's not clear why it's safe to recycle the pending list pages at all
(though deletion alone might well be okay, because readers can notice
a page is deleted if it isn't yet recycled). Why is it okay that we
can jump to the first block from the meta page in the face of
concurrent recycling?

Looking at scanPendingInsert(), it does appear that readers do buffer
lock crabbing/coupling for the meta page and the first pending page.
So that's an encouraging sign. However, ginInsertCleanup() *also* uses
shared buffer locks for both meta page and list head page (never
exclusive buffer locks) in exactly the same way when first
establishing what block is at the head of the list to be zapped. It's
acting as if it is a reader, but it's actually a writer. I don't
follow why this is correct. It looks like scanPendingInsert() can
decide that it knows where the head of the pending list is *after*
ginInsertCleanup() has already decided that that same head of the list
block needs to possibly be deleted/recycled. Have I missed something?

We really ought to make the asserts on gin page type into "can't
happen" elog errors, as a defensive measure, in both
scanPendingInsert() and ginInsertCleanup(). The 2013 bug fix actually
did this for the posting tree, but didn't touch similar pending list
code.

-- 
Peter Geoghegan

Re: [HACKERS] Bug in to_timestamp().

[Tom Lane]

Artur Zakirov  writes:
> [ 0001-to-timestamp-format-checking-v7.patch ]

This patch needs a rebase over the formatting.c fixes that have gone
in over the last couple of days.

Looking at the rejects, I notice that in your changes to parse_format(),
you seem to be making it rely even more heavily on remembering state about
the previous input.  I recommend against that --- it was broken before,
and it's a pretty fragile approach.  Backslashes are not that hard to
deal with in-line.

The larger picture though is that I'm not real sure that this patch is
going in the direction we want.  All of these cases work in both our
code and Oracle:

select to_timestamp('97/Feb/16', 'YY:Mon:DD')
select to_timestamp('97/Feb/16', 'YY Mon DD')
select to_timestamp('97 Feb 16', 'YY/Mon/DD')

(Well, Oracle thinks these mean 2097 where we think 1997, but the point is
you don't get an error.)  I see from your regression test additions that
you want to make some of these throw an error, and I think that any such
proposal is dead in the water.  Nobody is going to consider it an
improvement if it both breaks working PG apps and disagrees with Oracle.

regards, tom lane

Re: spgist rangetypes compiler warning (gcc 7.2.0)

[Tomas Vondra]

On 11/18/2017 10:40 PM, Tom Lane wrote:
> Tomas Vondra  writes:
>> while compiling on gcc 7.2.0 (on ARM), I got this warning:
> 
>> rangetypes_spgist.c: In function 'spg_range_quad_inner_consistent':
>> rangetypes_spgist.c:559:29: warning: comparison between pointer and
>> zero character constant [-Wpointer-compare]
>>   if (in->traversalValue != (Datum) 0)
>>  ^~
> 
> Huh.  I wonder why 7.2.1 on Fedora isn't producing that warning.
> 

Not sure. Maybe Ubuntu uses different flags (on ARM)? This is what I get
from 'gcc -v' on the machine:

Using built-in specs.
COLLECT_GCC=gcc
COLLECT_LTO_WRAPPER=/usr/lib/gcc/arm-linux-gnueabihf/7/lto-wrapper
Target: arm-linux-gnueabihf
Configured with: ../src/configure -v --with-pkgversion='Ubuntu/Linaro
7.2.0-8ubuntu3' --with-bugurl=file:///usr/share/doc/gcc-7/README.Bugs
--enable-languages=c,ada,c++,go,d,fortran,objc,obj-c++ --prefix=/usr
--with-gcc-major-version-only --program-suffix=-7
--program-prefix=arm-linux-gnueabihf- --enable-shared
--enable-linker-build-id --libexecdir=/usr/lib
--without-included-gettext --enable-threads=posix --libdir=/usr/lib
--enable-nls --with-sysroot=/ --enable-clocale=gnu
--enable-libstdcxx-debug --enable-libstdcxx-time=yes
--with-default-libstdcxx-abi=new --enable-gnu-unique-object
--disable-libitm --disable-libquadmath --enable-plugin
--enable-default-pie --with-system-zlib --with-target-system-zlib
--enable-objc-gc=auto --enable-multiarch --enable-multilib
--disable-sjlj-exceptions --with-arch=armv7-a --with-fpu=vfpv3-d16
--with-float=hard --with-mode=thumb --disable-werror --enable-multilib
--enable-checking=release --build=arm-linux-gnueabihf
--host=arm-linux-gnueabihf --target=arm-linux-gnueabihf
Thread model: posix
gcc version 7.2.0 (Ubuntu/Linaro 7.2.0-8ubuntu3)

regards

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

Re: percentile value check can be slow

[Tomas Vondra]

Hi,

On 11/18/2017 10:30 PM, David Fetter wrote:
> On Sat, Nov 18, 2017 at 10:44:36AM -0500, Tom Lane wrote:
>> jotpe  writes:
>>> I tried to enter invalid percentile fractions, and was astonished
>>> that it seems to be checked after many work is done?
>>
>> IIRC, only the aggregate's final-function is concerned with direct
>> arguments, so it's not all that astonishing.
> 
> It may not be surprising from the point of view of a systems
> programmer, but it's pretty surprising that this check is deferred to
> many seconds in when the system has all the information it need in
> order to establish this before execution begins.
> 
> I'm not sure I see an easy way to do this check early, but it's worth
> trying on grounds of POLA violation.  I have a couple of ideas on how
> to do this, one less invasive but hinky, the other a lot more invasive
> but better overall.
> 
> Ugly Hack With Ugly Performance Consequences:
> Inject a subtransaction at the start of execution that supplies an
> empty input to the final function with the supplied direct
> arguments.
> 

I'm pretty sure you realize this is quite unlikely to get accepted.

> Bigger Lift:
> Require a separate recognizer function direct arguments and fire
> it during post-parse analysis.  Perhaps this could be called
> recognizer along with the corresponding mrecognizer.  It's not
> clear that it's sane to have separate ones, but I thought I'd
> bring it up for completeness.
> 

Is 'recognizer' an established definition I should know? Is it the same
as 'validator' or is it something new/different?

> Way Bigger Lift, As Far As I Can Tell, But More Fun For Users:
> Allow optional CHECK constraints in CREATE AGGREGATE for direct
> arguments.
> 

How will any of the approaches deal with something like

select percentile_cont((select array_agg(v) from p))
   within group (order by a) from t;

In this case the the values are unknown after the parse analysis, so I
guess it does not really address that.


FWIW while I won't stand in the way of improving this, I wonder if this
is really worth the additional complexity. If you get errors like this
with a static list of values, you will fix the list and you're done. If
the list is dynamic (computed in the query itself), you'll still get the
error much later during query execution.

So if you're getting many failures like this for the "delayed error
reporting" to be an issue, perhaps there's something wrong in you stack
and you should address that instead?

regards

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

Re: [HACKERS] VACUUM and ANALYZE disagreeing on what reltuples means

[Tom Lane]

Tomas Vondra  writes:
> On 11/02/2017 08:15 PM, Tom Lane wrote:
>> However, I'm not sure we're there yet, because there remains a fairly
>> nasty discrepancy even once we've gotten everyone onto the same page
>> about reltuples counting just live tuples: VACUUM and ANALYZE have
>> different definitions of what's "live".  In particular they do not treat
>> INSERT_IN_PROGRESS and DELETE_IN_PROGRESS tuples the same.  Should we
>> try to do something about that?  If so, what?  It looks like ANALYZE's
>> behavior is pretty tightly constrained, judging by the comments in
>> acquire_sample_rows.

> ISTM we need to unify those definitions, probably so that VACUUM adopts
> what acquire_sample_rows does. I mean, if ANALYZE assumes that the stats
> will be updated at the end of transaction, why shouldn't VACUUM do the
> same thing?

That was the way I was leaning.  I haven't thought very hard about the
implications, but as long as the change in VACUUM's behavior extends
only to the live-tuple count it reports, it seems like adjusting it
couldn't break anything too badly.

>> Another problem is that it looks like CREATE INDEX will set reltuples
>> to the total number of heap entries it chose to index, because that
>> is what IndexBuildHeapScan counts.  Maybe we should adjust that?

> You mean by only counting live tuples in IndexBuildHeapRangeScan,
> following whatever definition we end up using in VACUUM/ANALYZE?

Right.  One issue is that, as I mentioned, the index AMs probably want to
think about total-tuples-indexed not live-tuples; so for their purposes,
what IndexBuildHeapScan currently counts is the right thing.  We need to
look and see if any AMs are actually using that value rather than just
silently passing it back.  If they are, we might need to go to the trouble
of computing/returning two values.

regards, tom lane

Re: [HACKERS] VACUUM and ANALYZE disagreeing on what reltuples means

[Tomas Vondra]

Hi,

On 11/02/2017 08:15 PM, Tom Lane wrote:
> Haribabu Kommi  writes:
>> The changes are fine and now it reports proper live tuples in both
>> pg_class and stats. The other issue of continuous vacuum operation
>> leading to decrease of number of live tuples is not related to this
>> patch and that can be handled separately.
> 
> I did not like this patch too much, because it did nothing to fix
> the underlying problem of confusion between "live tuples" and "total
> tuples"; in fact, it made that worse, because now the comment on
> LVRelStats.new_rel_tuples is a lie.  And there's at least one usage
> of that field value where I think we do want total tuples not live
> tuples: where we pass it to index AM cleanup functions.  Indexes
> don't really care whether heap entries are live or not, only that
> they're there.  Plus the VACUUM VERBOSE log message that uses the
> field is supposed to be reporting total tuples not live tuples.
> 
> I hacked up the patch trying to make that better, finding along the
> way that contrib/pgstattuple shared in the confusion about what
> it was trying to count.  Results attached.
> 

Thanks for looking into this. I agree your patch is more consistent and
generally cleaner.

> However, I'm not sure we're there yet, because there remains a fairly
> nasty discrepancy even once we've gotten everyone onto the same page
> about reltuples counting just live tuples: VACUUM and ANALYZE have
> different definitions of what's "live".  In particular they do not treat
> INSERT_IN_PROGRESS and DELETE_IN_PROGRESS tuples the same.  Should we
> try to do something about that?  If so, what?  It looks like ANALYZE's
> behavior is pretty tightly constrained, judging by the comments in
> acquire_sample_rows.
> 

Yeah :-(

For the record (and people following this thread who are too lazy to
open the analyze.c and search for the comments), the problem is that
acquire_sample_rows does not count HEAPTUPLE_INSERT_IN_PROGRESS as live
(and HEAPTUPLE_DELETE_IN_PROGRESS as dead) as it assumes the transaction
will send the stats at the end.

Which is a correct assumption, but it means that when there is a
long-running transaction that inserted/deleted many rows, the reltuples
value will oscillate during VACUUM / ANALYZE runs.

ISTM we need to unify those definitions, probably so that VACUUM adopts
what acquire_sample_rows does. I mean, if ANALYZE assumes that the stats
will be updated at the end of transaction, why shouldn't VACUUM do the
same thing?

The one reason I can think of is that VACUUM is generally expected to
run longer than ANALYZE, so the assumption that large transactions
complete later is not quite reliable.

Or is there some other reason why VACUUM can't treat the in-progress
tuples the same way?

> Another problem is that it looks like CREATE INDEX will set reltuples
> to the total number of heap entries it chose to index, because that
> is what IndexBuildHeapScan counts.  Maybe we should adjust that?
> 

You mean by only counting live tuples in IndexBuildHeapRangeScan,
following whatever definition we end up using in VACUUM/ANALYZE? Seems
like a good idea I guess, although CREATE INDEX not as frequent as the
other commands.

regards

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

Re: spgist rangetypes compiler warning (gcc 7.2.0)

[Tom Lane]

Tomas Vondra  writes:
> while compiling on gcc 7.2.0 (on ARM), I got this warning:

> rangetypes_spgist.c: In function 'spg_range_quad_inner_consistent':
> rangetypes_spgist.c:559:29: warning: comparison between pointer and
> zero character constant [-Wpointer-compare]
>   if (in->traversalValue != (Datum) 0)
>  ^~

Huh.  I wonder why 7.2.1 on Fedora isn't producing that warning.

> I believe we should simply treat the traversalValue as pointer, and
> change the condition to
> if (in->traversalValue)

Agreed, especially since it's done like that in spgscan.c and
geo_spgist.c.

regards, tom lane

Re: [HACKERS] Repetitive code in RI triggers

[Tom Lane]

Ildar Musin  writes:
> [ ri_triggers_v2.patch ]

Pushed with two minor improvements.  I noticed that ri_setdefault could
just go directly to ri_restrict rather than call the two separate triggers
that would end up there anyway; that lets its argument be "TriggerData
*trigdata" for more consistency with the other cases.  Also, this patch
made it very obvious that we were caching identical queries under hash
keys RI_PLAN_RESTRICT_DEL_CHECKREF and RI_PLAN_RESTRICT_UPD_CHECKREF,
so we might as well just use one hash entry for both cases, saving a few
lines of code as well as a lot of cycles.  Likewise in the other two
functions.

regards, tom lane

Re: WIP: BRIN multi-range indexes

[Tomas Vondra]

Hi,

Apparently there was some minor breakage due to duplicate OIDs, so here
is the patch series updated to current master.

regards

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


0001-Pass-all-keys-to-BRIN-consistent-function-at-once.patch.gz
Description: application/gzip


0002-BRIN-bloom-indexes.patch.gz
Description: application/gzip


0003-BRIN-multi-range-minmax-indexes.patch.gz
Description: application/gzip


0004-Move-IS-NOT-NULL-checks-to-bringetbitmap.patch.gz
Description: application/gzip

Re: [HACKERS] PATCH: multivariate histograms and MCV lists

[Tomas Vondra]

Hi,

Attached is an updated version of the patch, adopting the psql describe
changes introduced by 471d55859c11b.

regards

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


0001-multivariate-MCV-lists.patch.gz
Description: application/gzip


0002-multivariate-histograms.patch.gz
Description: application/gzip

Re: [HACKERS] Consistently catch errors from Python _New() functions

[Tom Lane]

Peter Eisentraut  writes:
> On 11/17/17 12:16, Tom Lane wrote:
>> I'm confused by the places that change PLy_elog calls to pass NULL:
>> 
>> -PLy_elog(ERROR, "could not create globals");
>> +PLy_elog(ERROR, NULL);
>> 
>> How is that an improvement?  Otherwise it looks reasonable.

> If we pass NULL, then the current Python exception becomes the primary
> error, so you'd end up with an "out of memory" error of some kind with a
> stack trace, which seems useful.

Oh, I see.  Objection withdrawn.

regards, tom lane

Re: to_typemod(type_name) information function

[Sophie Herold]

On 18/11/17 16:50, Tom Lane wrote:
> Sophie Herold  writes:
>> I need to test a (user) given column type name, with one in the database
>> for equality. To this end, I have to do some kind of normalization (e.g.
>> 'timestamptz(2)' to 'timestamp (2) with time zone'.)
> 
> Perhaps format_type(oid, integer) would help you.
> 
>   regards, tom lane
> 

I am not sure how. I am exactly looking for the the second argument integer.

The only workaround I can think of is to create a table with a column
with that type, ask the pg_catalog for the typemod afterwards and
rollback the creation. But that doesn't sound like a proper solution to me.

Best,
Sophie

Re: [HACKERS] Consistently catch errors from Python _New() functions

[Peter Eisentraut]

On 11/17/17 12:16, Tom Lane wrote:
> I'm confused by the places that change PLy_elog calls to pass NULL:
> 
> - PLy_elog(ERROR, "could not create globals");
> + PLy_elog(ERROR, NULL);
> 
> How is that an improvement?  Otherwise it looks reasonable.

If we pass NULL, then the current Python exception becomes the primary
error, so you'd end up with an "out of memory" error of some kind with a
stack trace, which seems useful.

The previous coding style invented a bespoke error message for each of
these rare out of memory scenarios, which seems wasteful.  We don't
create "out of memory while creating some internal list you've never
heard of" errors elsewhere either.

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

Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

[Peter Eisentraut]

On 11/18/17 06:32, Michael Paquier wrote:
> Here are some comments.
> 
> +* The client requires channel biding.  Channel binding type
> s/biding/binding/.

fixed

> if (!state->ssl_in_use)
> +   /*
> +* Without SSL, we don't support channel binding.
> +*
> Better to add brackets if adding a comment.

done

> +* Read value provided by client; only tls-unique is supported
> +* for now.  XXX Not sure whether it would be safe to print
> +* the name of an unsupported binding type in the error
> +* message.  Pranksters could print arbitrary strings into the
> +* log that way.
> That's why I didn't show those strings in the error messages of the
> previous versions. Those are useless as well, except for debugging the
> feature and the protocol.

Right.  I left the comment in there as a note to future hackers who want
to improve error messages (often myself).

> +   cbind_header_len = 4 + strlen(state->channel_binding_type); /*
> p=type,, */
> +   cbind_input_len = cbind_header_len + cbind_data_len;
> +   cbind_input = malloc(cbind_input_len);
> +   if (!cbind_input)
> +   goto oom_error;
> +   snprintf(cbind_input, cbind_input_len, "p=%s",
> state->channel_binding_type);
> +   memcpy(cbind_input + cbind_header_len, cbind_data, cbind_data_len);
> By looking at RFC5802, a base64 encoding of cbind-input is used:
> cbind-input   = gs2-header [ cbind-data ]
> gs2-cbind-flag "," [ authzid ] ","
> However you are missing two commands after p=%s, no?

fixed

I have committed the patch with the above fixes.

I'll be off for a week, so perhaps by that time you could make a rebased
version of the rest?  I'm not sure how much more time I'll have, so
maybe it will end up being moved to the next CF.

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

Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

[Michael Paquier]

On Sat, Nov 18, 2017 at 4:31 AM, Peter Eisentraut
 wrote:
> I made some significant changes to the logic.

Thanks!

> The selection of the channel binding flag (n/y/p) in the client seemed
> wrong.  Your code treated 'y' as an error, but I think that is a
> legitimate case, for example a PG11 client connecting to a PG10 server
> over SSL.  The client supports channel binding in that case and
> (correctly) thinks the server does not, so we use the 'y' flag and
> proceed normally without channel binding.
>
> The selection of the mechanism in the client was similarly incorrect, I
> think.  The code would not tolerate a situation were an SSL connection
> is in use but the server does not advertise the -PLUS mechanism, which
> again could be from a PG10 server.

Hm, OK. I have been likely confused by the fact that eSws is a valid
b64-encoded cbind-input on v10 servers. And the spec has no direct
mention of the matter, only of biws.

> The creation of the channel binding data didn't match the spec, because
> the gs2-header (p=type,,) was not included in the data put through
> base64.  This was done incorrectly on both server and client, so the
> protocol still worked.  (However, in the non-channel-binding case we
> hardcode "biws", which is exactly the base64-encoding of the gs2-header.
>  So that was inconsistent.)

Meh-to-self, you are right. Still it seems to me that your version is
forgetting something.. Please see below.

> I think we also need to backpatch a bug fix into PG10 so that the server
> can accept base64("y,,") as channel binding data.  Otherwise, the above
> scenario of a PG11 client connecting to a PG10 server over SSL will
> currently fail because the server will not accept the channel binding data.

Yes, without the additional comparison, the failure is weird for the
user. Here is what I have with an unpatched v10 server:
psql: FATAL:  unexpected SCRAM channel-binding attribute in client-final-message
This is going to need a one-liner in read_client_final_message()'s auth-scram.c.

> Please check my patch and think through these changes.  I'm happy to
> commit the patch as is if there are no additional insights.

Here are some comments.

+* The client requires channel biding.  Channel binding type
s/biding/binding/.

if (!state->ssl_in_use)
+   /*
+* Without SSL, we don't support channel binding.
+*
Better to add brackets if adding a comment.

+* Read value provided by client; only tls-unique is supported
+* for now.  XXX Not sure whether it would be safe to print
+* the name of an unsupported binding type in the error
+* message.  Pranksters could print arbitrary strings into the
+* log that way.
That's why I didn't show those strings in the error messages of the
previous versions. Those are useless as well, except for debugging the
feature and the protocol.

+   cbind_header_len = 4 + strlen(state->channel_binding_type); /*
p=type,, */
+   cbind_input_len = cbind_header_len + cbind_data_len;
+   cbind_input = malloc(cbind_input_len);
+   if (!cbind_input)
+   goto oom_error;
+   snprintf(cbind_input, cbind_input_len, "p=%s",
state->channel_binding_type);
+   memcpy(cbind_input + cbind_header_len, cbind_data, cbind_data_len);
By looking at RFC5802, a base64 encoding of cbind-input is used:
cbind-input   = gs2-header [ cbind-data ]
gs2-cbind-flag "," [ authzid ] ","
However you are missing two commands after p=%s, no?
-- 
Michael

percentile value check can be slow

[jotpe]

I tried to enter invalid percentile fractions, and was astonished that 
it seems to be checked after many work is done?


psql (11devel)
Type "help" for help.

jotpe=# \timing
Timing is on.
jotpe=# select percentile_cont(array[0,0.25,0.5,1,1,null,2]) within 
group(order by txk) from klima_tag;

ERROR:  percentile value 2 is not between 0 and 1
Time: 19155,565 ms (00:19,156)
jotpe=# select count(*) from klima_tag;
  count
--
 13950214
(1 row)

Time: 933,847 ms


Best regards Johannes