Re: psql - add SHOW_ALL_RESULTS option

[Kyotaro Horiguchi]

Hello.

On 2019/07/29 6:36, Fabien COELHO wrote:> 
>> Thanks for the feedback. Attached v3 with further documentation updates.
> 
> Attached v4 also fixes pg_stat_statements non regression tests, per pg 
> patch tester travis run.

Thanks. I looked this more closely.


+ * Marshal the COPY data.  Either subroutine will get the
+ * connection out of its COPY state, then call PQresultStatus()
+ * once and report any error.

This comment doesn't explain what the result value means.

+ * When our command string contained a COPY FROM STDIN or COPY TO STDOUT,
+ * the PGresult associated with these commands must be processed.  In that
+ * event, we'll marshal data for the COPY.

I think this is not needed. This phrase was needed to explain why
we need to loop over subsequent results after PQexec in the
current code, but in this patch PQsendQuery is used instead,
which doesn't suffer somewhat confusing behavior. All results are
handled without needing an unusual processing.

+ * Update result if further processing is necessary.  (Returning NULL
+ * prevents the command status from being printed, which we want in that
+ * case so that the status line doesn't get taken as part of the COPY data.)

It seems that the purpose of the returned PGresult is only
printing status of this COPY. If it is true, I'd like to see
something like the following example.

| Returns result in the case where queryFout is safe to output
| result status. That is, in the case of COPY IN, or in the case
| where COPY OUT is written to other than pset.queryFout.


+if (!AcceptResult(result, false))
+{
+  /* some error occured, record that */
+  ShowNoticeMessage();

The comment in the original code was:

-  /*
-   * Failure at this point is always a server-side failure or a
-   * failure to submit the command string.  Either way, we're
-   * finished with this command string.
-   */

The first half of the comment seems to be true for this
patch. Don't we preserve that comment?


+success = handleCopyOut(pset.db,
+copystream,
+_result)
+  && success
+  && (copystream != NULL);

success is always true at thie point so "&& success" is no longer
useful. (It is same for the COPY IN case).


+/* must handle COPY before changing the current result */
+result_status = PQresultStatus(result);
+if (result_status == PGRES_COPY_IN ||
+  result_status == PGRES_COPY_OUT)

I didn't get "before changing the curren result" in the
comment. Isn't "handle COPY stream if any" enough?

+if (result_status == PGRES_COPY_IN ||
+  result_status == PGRES_COPY_OUT)
+{
+  ShowNoticeMessage();
+  HandleCopyResult();
+}

It seems that it is wrong that this ignores the return value of
HandleCopyResult().


+/* timing measure before printing the last result */
+if (last && pset.timing)

I'm not sure whether we reached any consensus with ths
behavior. This means the timing includes result-printing time of
other than the last one. If we don't want include printing time
at all, we can exclude it with a small amount of additional
complexity.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: Duplicated LSN in ReorderBuffer

[Simon Riggs]

On Mon, 29 Jul 2019 at 00:09, Petr Jelinek  wrote:


> Given that we don't process any other records in this function besides
> XLOG_HEAP2_MULTI_INSERT and XLOG_HEAP2_NEW_CID, it seems like simplest
> fix is to just remove the first check for fast forward and keep the one
> in XLOG_HEAP2_MULTI_INSERT.
>

Fix proposed by Petr, with comments as explained by Andres.

-- 
Simon Riggshttp://www.2ndQuadrant.com/

PostgreSQL Solutions for the Enterprise


allow_XLOG_HEAP2_NEW_CID_while_building_snapshot.v1.patch
Description: Binary data

Re: how to run encoding-dependent tests by default

[Peter Eisentraut]

On 2019-07-28 21:42, Tom Lane wrote:
> Oh ... one other thought, based on forcing the collate.linux.utf8
> test to run on platforms where it can be expected to fail: I think
> you'd be well advised to make that test verify that the required
> collations are present, the same as you did in the collate.icu.utf8
> test.  I noticed for instance that it fails if en_US.utf8 is not
> present (or not spelled exactly like that), but I doubt that that
> locale is necessarily present on every Linux platform.  tr_TR is
> even more likely to be subject to packagers' whims.

This was already done in my v2 test posted in this thread.

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

Re: how to run encoding-dependent tests by default

[Peter Eisentraut]

On 2019-07-28 20:12, Tom Lane wrote:
> So I wish we could get rid of the Makefile changes, have the test
> scripts be completely responsible for whether to run themselves or
> not, and put them into the schedule files normally.
> 
> It's pretty obvious how we might do this for collate.icu.utf8:
> make it look to see if there are any ICU-supplied collations in
> pg_collation.
> 
> I'm less clear on a reasonable way to detect a glibc platform
> from SQL.  The best I can think of is to see if the string
> "linux" appears in the output of version(), and that's probably
> none too robust.  Can we do anything based on the content of
> pg_collation?  Probably not :-(.
> 
> Still, even if you only fixed collate.icu.utf8 this way, that
> would be a step forward since it would solve the Windows aspect.

Good points.  Updated patch attach.

(The two tests create the same schema name, so they cannot be run in
parallel.  I opted against changing that here, since it would blow up
the patch and increase the diff between the two tests.)

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From 0a4abb39d68406313e373aac82faa101bb62b5dd Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Mon, 29 Jul 2019 07:10:42 +0200
Subject: [PATCH v3] Run UTF8-requiring collation tests by default

The tests collate.icu.utf8 and collate.linux.utf8 were previously only
run when explicitly selected via EXTRA_TESTS.  They require a UTF8
database, because the error messages in the expected files refer to
that, and they use some non-ASCII characters in the tests.  Since
users can select any locale and encoding for the regression test run,
it was not possible to include these tests automatically.

To fix, use psql's \if facility to check various prerequisites such as
platform and the server encoding and quit the tests at the very
beginning if the configuration is not adequate.  We then need to
maintain alternative expected files for these tests, but they are very
tiny and never need to change after this.

These two tests are now run automatically as part of the regression
tests.

Discussion: 
https://www.postgresql.org/message-id/flat/052295c2-a2e1-9a21-bd36-8fbff8686cf3%402ndquadrant.com
---
 doc/src/sgml/regress.sgml  |  8 
 src/test/regress/expected/collate.icu.utf8.out |  7 +++
 src/test/regress/expected/collate.icu.utf8_1.out   |  9 +
 src/test/regress/expected/collate.linux.utf8.out   |  7 +++
 src/test/regress/expected/collate.linux.utf8_1.out | 11 +++
 src/test/regress/parallel_schedule |  5 +++--
 src/test/regress/serial_schedule   |  2 ++
 src/test/regress/sql/collate.icu.utf8.sql  |  8 
 src/test/regress/sql/collate.linux.utf8.sql|  8 
 9 files changed, 55 insertions(+), 10 deletions(-)
 create mode 100644 src/test/regress/expected/collate.icu.utf8_1.out
 create mode 100644 src/test/regress/expected/collate.linux.utf8_1.out

diff --git a/doc/src/sgml/regress.sgml b/doc/src/sgml/regress.sgml
index 7b68213266..d98187c970 100644
--- a/doc/src/sgml/regress.sgml
+++ b/doc/src/sgml/regress.sgml
@@ -363,14 +363,6 @@ Extra Tests
 
 make check EXTRA_TESTS=numeric_big
 
-To run the collation tests:
-
-make check EXTRA_TESTS='collate.linux.utf8 collate.icu.utf8' LANG=en_US.utf8
-
-The collate.linux.utf8 test works only on Linux/glibc
-platforms.  The collate.icu.utf8 test only works when
-support for ICU was built.  Both tests will only succeed when run in a
-database that uses UTF-8 encoding.

   
 
diff --git a/src/test/regress/expected/collate.icu.utf8.out 
b/src/test/regress/expected/collate.icu.utf8.out
index 01bd9fb5dd..51262e0bf4 100644
--- a/src/test/regress/expected/collate.icu.utf8.out
+++ b/src/test/regress/expected/collate.icu.utf8.out
@@ -1,6 +1,13 @@
 /*
  * This test is for ICU collations.
  */
+/* skip test if not UTF8 server encoding or no ICU collations installed */
+SELECT getdatabaseencoding() <> 'UTF8' OR
+   (SELECT count(*) FROM pg_collation WHERE collprovider = 'i') = 0
+   AS skip_test \gset
+\if :skip_test
+\quit
+\endif
 SET client_encoding TO UTF8;
 CREATE SCHEMA collate_tests;
 SET search_path = collate_tests;
diff --git a/src/test/regress/expected/collate.icu.utf8_1.out 
b/src/test/regress/expected/collate.icu.utf8_1.out
new file mode 100644
index 00..a6a33b39ab
--- /dev/null
+++ b/src/test/regress/expected/collate.icu.utf8_1.out
@@ -0,0 +1,9 @@
+/*
+ * This test is for ICU collations.
+ */
+/* skip test if not UTF8 server encoding or no ICU collations installed */
+SELECT getdatabaseencoding() <> 'UTF8' OR
+   (SELECT count(*) FROM pg_collation WHERE collprovider = 'i') = 0
+   AS skip_test \gset
+\if :skip_test
+\quit
diff --git a/src/test/regress/expected/collate.linux.utf8.out 
b/src/test/regress/expected/collate.linux.utf8.out
index 619688f851..ad56ff9caa 

Re: LLVM compile failing in seawasp

[Fabien COELHO]

Let's just commit the #undef so that seawasp is green and back to
being ready to tell us if something else breaks.


+1.  I was afraid that working around this would be impossibly
painful ... but if it just takes one judiciously placed #undef,
let's do that and not argue about it.


Done.


Seawasp is back to green.

--
Fabien.

Re: LLVM compile failing in seawasp

[Andres Freund]

Hi,

On 2019-07-29 10:27:54 +1200, Thomas Munro wrote:
> On Mon, Jul 29, 2019 at 9:55 AM Tom Lane  wrote:
> > Thomas Munro  writes:
> > > Let's just commit the #undef so that seawasp is green and back to
> > > being ready to tell us if something else breaks.
> >
> > +1.  I was afraid that working around this would be impossibly
> > painful ... but if it just takes one judiciously placed #undef,
> > let's do that and not argue about it.
> 
> Done.

cool, thanks.

Greetings,

Andres Freund

Re: refactoring - share str2*int64 functions

[Andres Freund]

Hi,

On 2019-07-29 10:48:41 +0900, Michael Paquier wrote:
> On Thu, Jul 18, 2019 at 09:16:22PM -0700, Andres Freund wrote:
> >> - One set of functions for the backend, called pg_stro[u]intXX_backend
> >> or pg_backend_stro[u]intXX which can take as extra argument error_ok,
> >> calling the portions in src/port/, and move those functions in a new
> >> file prefixed with "backend_" in src/backend/utils/misc/ with a name
> >> consistent with the one in src/port/ (with the previous naming that
> >> would be backend_strtoint.c)
> > 
> > I'm not following. What would be the point of any of this?  The error_ok
> > bit is unnecessary, because the function is exactly the same as the
> > generic function.  And the backend_ prefix would be pretty darn weird,
> > given that that's already below src/backend.
> 
> Do you have a better idea of name for those functions?

I don't understand why they need any prefix. pg_strto[u]int{32,64}{,_checked}.
The unchecked variant would be for both frontend backend. The checked one
either for both frontend/backend, or just for backend. I also could live with
_raises, _throws or such instead of _checked. Implement all of them in one
file in /common/, possibly hidin gthe ones not currently implemented for the
frontend.

Imo if _checked is implemented for both frontend/backend they'd need
different error messages. In my opinion
out_of_range:
if (!errorOK)
fprintf(stderr,
"value \"%s\" is out of range for type 
bigint\n", str);
return false;

invalid_syntax:
if (!errorOK)
fprintf(stderr,
"invalid input syntax for type bigint: 
\"%s\"\n", str);

is unsuitable for generic code. In fact, I'm doubtful that it's applicable for
any use, except for int8in(), which makes me think it better ought to use the
a non-checked variant, and include the errors directly.  If we still want to
have _checked - which is reasonable imo - it should refer to 64bit integers or 
somesuch.

Greetings,

Andres Freund

Re: refactoring - share str2*int64 functions

[Fabien COELHO]

Bonjour Michaël,

Fabien, are you planning to send an updated patch?  This stuff has 
value.


Yep, I will try for this week.

--
Fabien.

Re: ANALYZE: ERROR: tuple already updated by self

[Andres Freund]

Hi,

On 2019-07-28 21:21:51 +0200, Tomas Vondra wrote:
> AFAICS it applies to 10+ versions, because that's where extended stats
> were introduced. We certainly can't mess with catalogs there, so this is
> about the only backpatchable fix I can think of.

AFAIU the inh version wouldn't be used anyway, and this has never
worked. So I think it's imo fine to fix it that way for < master. For
master we should have something better, even if perhaps not immediately.

- Andres

Re: COPY command on a table column marked as GENERATED ALWAYS

[Ashutosh Sharma]

On Mon, Jul 29, 2019 at 7:27 AM Michael Paquier  wrote:
>
> On Fri, Jul 26, 2019 at 03:12:28PM +0530, Ashutosh Sharma wrote:
> > Hi All,
> >
> > I'm able to insert data into a table column marked as GENERATED ALWAYS
> > using COPY command however, it fails with INSERT command. Isn't that a
> > bug with COPY command?
>
> Per the documentation in the section for GENERATED ALWAYS:
> https://www.postgresql.org/docs/devel/sql-createtable.html
>
> "The clauses ALWAYS and BY DEFAULT determine how the sequence value is
> given precedence over a user-specified value in an INSERT
> statement. If ALWAYS is specified, a user-specified value is only
> accepted if the INSERT statement specifies OVERRIDING SYSTEM VALUE. If
> BY DEFAULT is specified, then the user-specified value takes
> precedence. See INSERT for details. (In the COPY command,
> user-specified values are always used regardless of this setting.)"
>
> So it behaves as documented.

Okay, Thanks for the pointer!

-- 
With Regards,
Ashutosh Sharma
EnterpriseDB:http://www.enterprisedb.com

Re: [proposal] de-TOAST'ing using a iterator

[John Naylor]

On Thu, Jul 25, 2019 at 10:21 PM Binguo Bao  wrote:
>
> Hi John!
> Sorry for the late reply. It took me some time to fix a random bug.

Don't worry, it's not late at all! :-)

>> In the case where we don't know the slice size, how about the other
>> aspect of my question above: Might it be simpler and less overhead to
>> decompress entire chunks at a time? If so, I think it would be
>> enlightening to compare performance.
>
>
> Good idea. I've tested  your propopal with scripts and patch v5 in the 
> attachment:
>
>   master  patch v4  patch v5
> comp. beg.4364ms  1505ms  1529ms
> comp. end   28321ms31202ms  26916ms
> uncomp. beg. 3474ms  1513ms  1523ms
> uncomp. end 27416ms30260ms  25888ms
>
> The proposal improves suffix query performance greatly
> with less calls to the decompression function.

Looks good. I repeated my CIA fact book test and found no difference
with compression, but found that suffix search in the uncompressed
case had less regression (~5%) than v4 (>8%). Let's pursue this
further.

> Besides, do you have any other suggestions for the structure of 
> DetoastIterator or ToastBuffer?

My goal for this stage of review was to understand more fully what the
code is doing, and make it as simple and clear as possible, starting
at the top level. In doing so, it looks like I found some additional
performance gains. I haven't looked much yet at the TOAST fetching
logic.


1). For every needle comparison, text_position_next_internal()
calculates how much of the value is needed and passes that to
detoast_iterate(), which then calculates if it has to do something or
not. This is a bit hard to follow. There might also be a performance
penalty -- the following is just a theory, but it sounds plausible:
The CPU can probably correctly predict that detoast_iterate() will
usually return the same value it did last time, but it still has to
call the function and make sure, which I imagine is more expensive
than advancing the needle. Ideally, we want to call the iterator only
if we have to.

In the attached patch (applies on top of your v5),
text_position_next_internal() simply compares hptr to the detoast
buffer limit, and calls detoast_iterate() until it can proceed. I
think this is clearer. (I'm not sure of the error handling, see #2.)
In this scheme, the only reason to know length is to pass to
pglz_decompress_iterate() in the case of in-line compression. As I
alluded to in my first review, I don't think it's worth the complexity
to handle that iteratively since the value is only a few kB. I made it
so in-line datums are fully decompressed as in HEAD and removed struct
members to match. I also noticed that no one updates or looks at
"toast_iter.done" so I removed that as well.

Now pglz_decompress_iterate() doesn't need length at all. For testing
I just set decompress_all = true and let the compiler optimize away
the rest. I left finishing it for you if you agree with these changes.

With this additional patch, the penalty for suffix search in my CIA
fact book test is only ~2% in the compressed case, and might even be
slightly faster than HEAD in the uncompressed case.


2). detoast_iterate() and fetch_datum_iterate() return a value but we
don't check it or do anything with it. Should we do something with it?
It's also not yet clear if we should check the iterator state instead
of return values. I've added some XXX comments as a reminder. We
should also check the return value of pglz_decompress_iterate().


3). Speaking of pglz_decompress_iterate(), I diff'd it with
pglz_decompress(), and I have some questions on it:

a).
+ srcend = (const unsigned char *) (source->limit == source->capacity
? source->limit : (source->limit - 4));

What does the 4 here mean in this expression? Is it possible it's
compensating for this bit in init_toast_buffer()?

+ buf->limit = VARDATA(buf->buf);

It seems the initial limit should also depend on whether the datum is
compressed, right? Can we just do this:

+ buf->limit = buf->position;

b).
- while (sp < srcend && dp < destend)
...
+ while (sp + 1 < srcend && dp < destend &&
...

Why is it here "sp + 1"?


4. Note that varlena.c has a static state variable, and a cleanup
function that currently does:

static void
text_position_cleanup(TextPositionState *state)
{
/* no cleanup needed */
}

It seems to be the detoast iterator could be embedded in this state
variable, and then free-ing can happen here. That has a possible
advantage that the iterator struct would be on the same cache line as
the state data. That would also remove the need to pass "iter" as a
parameter, since these functions already pass "state". I'm not sure if
this would be good for other users of the iterator, so maybe we can
hold off on that for now.

5. Would it be a good idea to add tests (not always practical), or
more Assert()'s? You probably already know this, but as a reminder
it's 

Re: Fix typos and inconsistencies for HEAD (take 8)

[Michael Paquier]

On Sun, Jul 28, 2019 at 07:44:44AM +0300, Alexander Lakhin wrote:
> 8.3. lag_with_offset_and_default, * ->
> window_lag_with_offset_and_default, window_* (in windowfuncs.c)

The intention here is to refer to the SQL-visible names.

> In passing, I found a legacy script, FAQ2txt, that should be deleted as
> unusable.

Right.  This got forgotten with the cleanup from f3f45c87 and
bf4497cc.

Applied.  Thanks, as usual.
--
Michael


signature.asc
Description: PGP signature

Re: psql - add SHOW_ALL_RESULTS option

[Kyotaro Horiguchi]

Hello, Fabien.

At Fri, 26 Jul 2019 08:19:47 + (GMT), Fabien COELHO  
wrote in 
> 
> Hello Kyotaro-san,
> 
> >> Attached a v2 for the always-show-all-results variant. Thanks for the
> >> debug!
> >
> > I have some comments on this patch.
> >
> > I'm +1 for always output all results without having knobs.
> 
> That makes 4 opinions expressed towards this change of behavior, and
> none against.
> 
> > Documentation (psql-ref.sgml) has another place that needs the
> > same amendment.
> 
> Indeed.
> 
> > Looking the output for -t, -0, -A or something like, we might need
> > to introduce result-set separator.
> 
> Yep, possibly. I'm not sure this is material for this patch, though.

I'm fine with that.

> > # -eH looks broken for me but it would be another issue.
> 
> It seems to work for me. Could you be more precise about how it is
> broken?

It emits bare command string before html result. It's not caused
by this patch.


> > Valid setting of FETCH_COUNT disables this feature. I think it is
> > unwanted behavior.
> 
> Yes and no: this behavior (bug, really) is pre-existing, FETCH_COUNT
> does not work with combined queries:
> 
>   sh> /usr/bin/psql
>   psql (12beta2 ...)
>   fabien=# \set FETCH_COUNT 2
>   fabien=# SELECT 1234 \; SELECT 5432 ;
>   fabien=#
> 
>   same thing with pg 11.4, and probably down to every version of
>   postgres
>   since the feature was implemented...
> 
> I think that fixing this should be a separate bug report and
> patch. I'll try to look at it.

Ah, I didin't notieced that. Thanks for the explanation.

> Thanks for the feedback. Attached v3 with further documentation
> updates.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Bruce Momjian]

On Thu, Jul 25, 2019 at 01:03:06PM -0400, Bruce Momjian wrote:
> On Tue, Jul 16, 2019 at 01:24:54PM +0900, Masahiko Sawada wrote:
> > On Sat, Jul 13, 2019 at 12:33 AM Bruce Momjian  wrote:
> > > then each row change gets its own LSN.  You are asking if an update that
> > > just expires one row and adds it to a new page gets the same LSN.  I
> > > don't know.
> > 
> > The following scripts can reproduce that different two pages have the same 
> > LSN.
> > 
> > =# create table test (a int);
> > CREATE TABLE
> > =# insert into test select generate_series(1, 226);
> > INSERT 0 226
> > =# update test set a = a where a = 1;
> > UPDATE 1
> > =# select lsn from page_header(get_raw_page('test', 0));
> > lsn
> > ---
> >  0/1690488
> > (1 row)
> > 
> > =# select lsn from page_header(get_raw_page('test', 1));
> > lsn
> > ---
> >  0/1690488
> > (1 row)
> > 
> > So I think it's better to use LSN and page number to create IV. If we
> > modify different tables by single WAL we also would need OID or
> > relfilenode but I don't think currently we have such operations.
> 
> OK, good to know, thanks.

I did some more research on which cases use a single LSN to modify
multiple 8k pages.  The normal program flow is:

XLogBeginInsert();
...
XLogRegisterBuffer(0, meta, ...
--> recptr = XLogInsert(RM_BRIN_ID, XLOG_...

page = BufferGetPage(meta);
PageSetLSN(page, recptr);

XLogInsert() calls BufferGetTag(), which fills in the buffer's
RelFileNode (which internally is the tablespace, database, and
pg_class.relfilenode).  So, to use the LSN and page-number for the IV,
we need to make sure that there is no other encryption of those values
in a different relation.  What I did was to find cases where
XLogRegisterBuffer/PageSetLSN are called more than once for a single
LSN.  I found cases in:

brin_doupdate
brin_doinsert
brinRevmapDesummarizeRange
revmap_physical_extend
GenericXLogFinish
ginPlaceToPage
shiftList
ginDeletePage
gistXLogSplit
gistXLogPageDelete
gistXLogUpdate
hashbucketcleanup
_hash_doinsert
_hash_vacuum_one_page
_hash_addovflpage
_hash_freeovflpage
_hash_squeezebucket
_hash_init
_hash_expandtable
_hash_splitbucket
log_heap_visible
log_heap_update
_bt_insertonpg
_bt_split
_bt_newroot
_bt_getroot
_bt_mark_page_halfdead
_bt_unlink_halfdead_page
addLeafTuple
moveLeafs
doPickSplit
spgSplitNodeAction
log_newpage_range

Most of these are either updating the different pages in the same
relation (so the page-number for the IV would be different), or are
modifying other types of files, like vm.  (We have not discussed if we
are going to encrypt vm or fsm.  I am guessing we are not.)

You might say, well, is it terrible if we reuse the LSN in a different
relation with the same page number?  Yes.  The way CTR works, it
generates a stream of bits using the key and IV (which will be LSN and
page number).  It then XORs it with the page contents to encrypt it.  If
we encrypt an all-zero gap in a page, or a place in the page where the
page format is known, a user can XOR that with the encrypted data and
get the bit stream at that point.  They can then go to another page that
uses the same key and IV and XOR that to get the decrypted data.  CBC
mode is slightly better because it mixes the user data into the future
16-byte blocks, but lots of our early-byte page format is known, so it
isn't great.

You might say, wow, that is a lot of places to make sure we don't reuse
the LSN in a different relation with the same page number --- let's mix
the relfilenode in the IV so we are sure the IV is not reused.

Well, the pg_class.relfilenode is only unique within the
tablespace/database, i.e., from src/include/storage/relfilenode.h:

 * relNode identifies the specific relation.  relNode corresponds to
 * pg_class.relfilenode (NOT pg_class.oid, because we need to be able
 * to assign new physical files to relations in some situations).
 * Notice that relNode is only unique within a database in a particular
 * tablespace.

So, we would need to mix the tablespace, database oid, and relfilenode
into the IV to be unique.  We would then need to have pg_upgrade
preserve the relfilenode, change CREATE DATABASE to decrypt/encrypt when
creating a new database, and no longer allow files to be moved between
tablespaces without decryption/encryption.

There are just a whole host of complexities we add to encryption if we
add the requirement of preserving the refilenode, tablespace, and
database to decrypt each page.  I just don't think we want go there
unless we have a valid reason.

I am thinking of writing some Assert() code that checks that all buffers
using a single LSN are from the same relation (and therefore different
page 

Re: COPY command on a table column marked as GENERATED ALWAYS

[Michael Paquier]

On Fri, Jul 26, 2019 at 03:12:28PM +0530, Ashutosh Sharma wrote:
> Hi All,
> 
> I'm able to insert data into a table column marked as GENERATED ALWAYS
> using COPY command however, it fails with INSERT command. Isn't that a
> bug with COPY command?

Per the documentation in the section for GENERATED ALWAYS:
https://www.postgresql.org/docs/devel/sql-createtable.html

"The clauses ALWAYS and BY DEFAULT determine how the sequence value is
given precedence over a user-specified value in an INSERT
statement. If ALWAYS is specified, a user-specified value is only
accepted if the INSERT statement specifies OVERRIDING SYSTEM VALUE. If
BY DEFAULT is specified, then the user-specified value takes
precedence. See INSERT for details. (In the COPY command,
user-specified values are always used regardless of this setting.)"

So it behaves as documented.
--
Michael


signature.asc
Description: PGP signature

RE: Multivariate MCV list vs. statistics target

[Jamison, Kirk]

On Saturday, July 27, 2019 7:06 AM(GMT+9), Tomas Vondra wrote:
> On Fri, Jul 26, 2019 at 07:03:41AM +, Jamison, Kirk wrote:
> >On Sat, July 20, 2019 8:12 AM (GMT+9), Tomas Vondra wrote:
> >
> >> >+ /* XXX What if the target is set to 0? Reset the statistic?
> >> */
> >> >
> >> >This also makes me wonder. I haven't looked deeply into the code,
> >> >but since 0 is a valid value, I believe it should reset the stats.
> >>
> >> I agree resetting the stats after setting the target to 0 seems quite
> >> reasonable. But that's not what we do for attribute stats, because in
> >> that case we simply skip the attribute during the future ANALYZE runs
> >> - we don't reset the stats, we keep the existing stats. So I've done
> >> the same thing here, and I've removed the XXX comment.
> >>
> >> If we want to change that, I'd do it in a separate patch for both the
> >> regular and extended stats.
> >
> >Hi, Tomas
> >
> >Sorry for my late reply.
> >You're right. I have no strong opinion whether we'd want to change that
> behavior.
> >I've also confirmed the change in the patch where setting statistics
> >target 0 skips the statistics.
> >
> 
> OK, thanks.
> 
> >Maybe only some minor nitpicks in the source code comments below:
> >1. "it's" should be "its":
> >> +   * Compute statistic target, based on what's set for the
> statistic
> >> +   * object itself, and for it's attributes.
> >
> >2. Consistency whether you'd use either "statistic " or "statisticS ".
> >Ex. statistic target vs statisticS target, statistics object vs statistic
> object, etc.
> >
> >> Attached is v4 of the patch, with a couple more improvements:
> >>
> >> 1) I've renamed the if_not_exists flag to missing_ok, because that's
> >> more consistent with the "IF EXISTS" clause in the grammar (the old
> >> flag was kinda the exact opposite), and I've added a NOTICE about the skip.
> >
> >+boolmissing_ok;  /* do nothing if statistics does
> not exist */
> >
> >Confirmed. So we ignore if statistic does not exist, and skip the error.
> >Maybe to make it consistent with other data structures in
> >parsernodes.h, you can change the comment of missing_ok to:
> >/* skip error if statistics object does not exist */
> >
> 
> Thanks, I've fixed all those places in the attached v5.

I've confirmed the fix.

> >> 2) I've renamed ComputeExtStatsTarget to ComputeExtStatsRows, because
> >> that's what the function was doing anyway (computing sample size).
> >>
> >> 3) I've added a couple of regression tests to stats_ext.sql
> >>
> >> Aside from that, I've cleaned up a couple of places and improved a
> >> bunch of comments. Nothing huge.
> >
> >I have a question though regarding ComputeExtStatisticsRows.
> >I'm just curious with the value 300 when computing sample size.
> >Where did this value come from?
> >
> >+/* compute sample size based on the statistic target */
> >+return (300 * result);
> >
> >Overall, the patch is almost already in good shape for commit.
> >I'll wait for the next update.
> >
> 
> That's how we compute number of rows to sample, based on the statistics 
> target.
> See std_typanalyze() in analyze.c, which also cites the paper where this comes
> from.
Noted. Found it. Thank you for the reference.


There's just a small whitespace (extra space) below after running git diff 
--check.
>src/bin/pg_dump/pg_dump.c:7226: trailing whitespace.
>+ 
It would be better to post an updated patch,
but other than that, I've confirmed the fixes.
The patch passed the make-world and regression tests as well.
I've marked this as "ready for committer".


Regards,
Kirk Jamison

Re: [PATCH] minor bugfix for pg_basebackup (9.6 ~ )

[Michael Paquier]

On Fri, Jul 26, 2019 at 05:52:43PM -0400, Alvaro Herrera wrote:
> Yeah, I had removed those on purpose, but that was probably inconsistent
> with my own reviews of others' patches.  I pushed it with them.

Thanks!
--
Michael


signature.asc
Description: PGP signature

Re: TopoSort() fix

[Michael Paquier]

On Fri, Jul 26, 2019 at 08:24:16PM -0400, Tom Lane wrote:
> I think this is a sufficiently obvious bug, and a sufficiently
> obvious fix, that we should just fix it and not insist on getting
> a reproducible test case first.  I think a test case would soon
> bit-rot anyway, and no longer exercise the problem.
> 
> I certainly do *not* want to wait so long that we miss the
> upcoming minor releases.

+1.  Any volunteers?
--
Michael


signature.asc
Description: PGP signature

Re: refactoring - share str2*int64 functions

[Michael Paquier]

On Thu, Jul 18, 2019 at 09:16:22PM -0700, Andres Freund wrote:
> On 2019-07-19 12:21:27 +0900, Michael Paquier wrote:
> Why not common? It's not a platform dependent bit. Could even be put
> into the already existing string.c.

That would be fine to me, it is not like this file is bloated now.

>> - One set of functions for the backend, called pg_stro[u]intXX_backend
>> or pg_backend_stro[u]intXX which can take as extra argument error_ok,
>> calling the portions in src/port/, and move those functions in a new
>> file prefixed with "backend_" in src/backend/utils/misc/ with a name
>> consistent with the one in src/port/ (with the previous naming that
>> would be backend_strtoint.c)
> 
> I'm not following. What would be the point of any of this?  The error_ok
> bit is unnecessary, because the function is exactly the same as the
> generic function.  And the backend_ prefix would be pretty darn weird,
> given that that's already below src/backend.

Do you have a better idea of name for those functions?

>> - We also need the unsigned-specific equivalents of
>> pg_mul_s64_overflow and such, so I would suggest putting that in a new
>> header, simply uint.h.  If I finish by committing this stuff, I would
>> handle that in a separate commit.
> 
> Why not the same header? I fail to see what we'd gain by splitting it
> up.

No objections to that at the end.

Fabien, are you planning to send an updated patch?  This stuff has
value.
--
Michael


signature.asc
Description: PGP signature

Re: POC: Cleaning up orphaned files using undo logs

[Thomas Munro]

On Sat, Jul 27, 2019 at 2:27 PM Andres Freund  wrote:
> Note that neither of those mean that it's not a good idea to
> posix_fallocate() and *then* write zeroes, when initializing. For
> several filesystems that's more likely to result in more optimally sized
> filesystem extents, reducing fragmentation. And without an intervening
> f[data]sync, there's not much additional metadata journalling. Although
> that's less of an issue on some newer filesystems, IIRC (due to delayed
> allocation).

Interesting.  One way to bring back posix_fallocate() without
upsetting people on some filesystem out there would be to turn the new
wal_init_zero GUC into a choice: write (current default, and current
behaviour for 'on'), pwrite_hole (write just the final byte, current
behaviour for 'off'), posix_fallocate (like that 2013 patch that was
reverted) and posix_fallocate_and_write (do both as you said, to try
to solve that problem you mentioned that led to the revert).

I suppose there'd be a parallel GUC undo_init_zero.  Or some more
general GUC for any fixed-sized preallocated files like that (for
example if someone were to decide to do the same for SLRU files
instead of growing them block-by-block), called something like
file_init_zero.

-- 
Thomas Munro
https://enterprisedb.com

Re: Duplicated LSN in ReorderBuffer

[Petr Jelinek]

Hi,

On 27. 07. 19 3:15, Andres Freund wrote:

Hi,

Petr, Simon, see the potential issue related to fast forward at the
bottom.

[..snip..]

This actually made me look at the nearby changes due to

commit 9c7d06d60680c7f00d931233873dee81fdb311c6
Author: Simon Riggs 
Date:   2018-01-17 11:38:34 +

 Ability to advance replication slots

and uhm, I'm not sure they're fully baked. Something like:

/*
 * If we don't have snapshot or we are just fast-forwarding, there is no
 * point in decoding changes.
 */
if (SnapBuildCurrentState(builder) < SNAPBUILD_FULL_SNAPSHOT ||
ctx->fast_forward)
return;

case XLOG_HEAP2_MULTI_INSERT:
if (!ctx->fast_forward &&
SnapBuildProcessChange(builder, xid, 
buf->origptr))
DecodeMultiInsert(ctx, buf);
break;

is not very suggestive of that (note the second check).



You mean that it's redundant, yeah.., although given your next point, 
see bellow.




And related to the point of the theorizing above, I don't think skipping
XLOG_HEAP2_NEW_CID entirely when forwarding is correct. As a NEW_CID
record does not imply an invalidation message as discussed above, we'll
afaict compute wrong snapshots when such transactions are encountered
during forwarding. And we'll then log those snapshots to disk. Which
then means the slot cannot safely be used for actual decoding anymore -
as we'll use that snapshot when starting up decoding without fast
forwarding.



Hmm, I guess that's true. I think I have convinced myself that CID does 
not matter outside of this transaction, but since we might actually 
share the computed snapshot via file save/restore with other slots, any 
non-fast-forwarding decoding that reads the same transaction could miss 
the CID thanks to the shared snapshot which does not include it.


Given that we don't process any other records in this function besides 
XLOG_HEAP2_MULTI_INSERT and XLOG_HEAP2_NEW_CID, it seems like simplest 
fix is to just remove the first check for fast forward and keep the one 
in XLOG_HEAP2_MULTI_INSERT.


--
Petr Jelinek
2ndQuadrant - PostgreSQL Solutions for the Enterprise
https://www.2ndQuadrant.com/

Re: Commitfest 2019-07, the first of five* for PostgreSQL 13

[Thomas Munro]

Hello,

Here are the numbers at the end of the 4th week, with just a few days to go:

 status | w1  | w2  | w3  | w4
+-+-+-+-
 Committed  |  32 |  41 |  49 |  59
 Moved to next CF   |   5 |   6 |   6 |   6
 Needs review   | 146 | 128 | 114 | 106
 Ready for Committer|   7 |   9 |  10 |   7
 Rejected   |   2 |   2 |   2 |   2
 Returned with feedback |   2 |   2 |   2 |   2
 Waiting on Author  |  29 |  35 |  39 |  39
 Withdrawn  |   8 |   8 |   9 |  10

One observation is that the number marked "Ready for Committer" floats
around 7-10, and that's also about how many get committed each week
(around 20 were already committed pre-'fest), which seems like a clue
that things are moving through that part of the state transition
diagram reasonably well.

-- 
Thomas Munro
https://enterprisedb.com

Re: LLVM compile failing in seawasp

[Thomas Munro]

On Mon, Jul 29, 2019 at 9:55 AM Tom Lane  wrote:
> Thomas Munro  writes:
> > Let's just commit the #undef so that seawasp is green and back to
> > being ready to tell us if something else breaks.
>
> +1.  I was afraid that working around this would be impossibly
> painful ... but if it just takes one judiciously placed #undef,
> let's do that and not argue about it.

Done.

-- 
Thomas Munro
https://enterprisedb.com

Re: LLVM compile failing in seawasp

[Tom Lane]

Thomas Munro  writes:
> Let's just commit the #undef so that seawasp is green and back to
> being ready to tell us if something else breaks.

+1.  I was afraid that working around this would be impossibly
painful ... but if it just takes one judiciously placed #undef,
let's do that and not argue about it.

regards, tom lane

Re: LLVM compile failing in seawasp

[Thomas Munro]

On Mon, Jul 29, 2019 at 8:03 AM Fabien COELHO  wrote:
> If reordering includes is not an option, too bad. Then "#undef Min" which
> I find disputable, allthough I've done much worse... it might or might not
> work depending on what is done afterwards. Or rename the macro, as I
> suggested first, but there are many instances. Or convince LLVM people
> that they should change their stuff. Or document that pg jit will cannot
> use the latest LLVM, as a feature. Or find another solution:-)

Let's just commit the #undef so that seawasp is green and back to
being ready to tell us if something else breaks.  Personally, I don't
see any reason why  should entertain a request
to change their variable names to avoid our short common word macros
that aren't even all-caps, but if someone asks them and they agree to
do that before the final 9.0 release we can just revert.

-- 
Thomas Munro
https://enterprisedb.com

Re: psql - add SHOW_ALL_RESULTS option

[Fabien COELHO]

Thanks for the feedback. Attached v3 with further documentation updates.


Attached v4 also fixes pg_stat_statements non regression tests, per pg 
patch tester travis run.


--
Fabien.diff --git a/contrib/pg_stat_statements/expected/pg_stat_statements.out b/contrib/pg_stat_statements/expected/pg_stat_statements.out
index 6787ec1efd..de59a5cf65 100644
--- a/contrib/pg_stat_statements/expected/pg_stat_statements.out
+++ b/contrib/pg_stat_statements/expected/pg_stat_statements.out
@@ -49,17 +49,42 @@ BEGIN \;
 SELECT 2.0 AS "float" \;
 SELECT 'world' AS "text" \;
 COMMIT;
+ float 
+---
+   2.0
+(1 row)
+
+ text  
+---
+ world
+(1 row)
+
 -- compound with empty statements and spurious leading spacing
-\;\;   SELECT 3 + 3 \;\;\;   SELECT ' ' || ' !' \;\;   SELECT 1 + 4 \;;
- ?column? 
---
-5
+\;\;   SELECT 3 + 3 AS "+" \;\;\;   SELECT ' ' || ' !' AS "||" \;\;   SELECT 1 + 4 AS "+" \;;
+ + 
+---
+ 6
+(1 row)
+
+ ||  
+-
+   !
+(1 row)
+
+ + 
+---
+ 5
 (1 row)
 
 -- non ;-terminated statements
 SELECT 1 + 1 + 1 AS "add" \gset
 SELECT :add + 1 + 1 AS "add" \;
 SELECT :add + 1 + 1 AS "add" \gset
+ add 
+-
+   5
+(1 row)
+
 -- set operator
 SELECT 1 AS i UNION SELECT 2 ORDER BY i;
  i 
@@ -102,12 +127,12 @@ SELECT query, calls, rows FROM pg_stat_statements ORDER BY query COLLATE "C";
  SELECT $1+| 4 |4
   +|   | 
AS "text"   |   | 
- SELECT $1 + $2| 2 |2
  SELECT $1 + $2 + $3 AS "add"  | 3 |3
+ SELECT $1 + $2 AS "+" | 2 |2
  SELECT $1 AS "float"  | 1 |1
  SELECT $1 AS "int"| 2 |2
  SELECT $1 AS i UNION SELECT $2 ORDER BY i | 1 |2
- SELECT $1 || $2   | 1 |1
+ SELECT $1 || $2 AS "||"   | 1 |1
  SELECT pg_stat_statements_reset() | 1 |1
  WITH t(f) AS (   +| 1 |2
VALUES ($1), ($2)  +|   | 
diff --git a/contrib/pg_stat_statements/sql/pg_stat_statements.sql b/contrib/pg_stat_statements/sql/pg_stat_statements.sql
index 8b527070d4..ea3a31176e 100644
--- a/contrib/pg_stat_statements/sql/pg_stat_statements.sql
+++ b/contrib/pg_stat_statements/sql/pg_stat_statements.sql
@@ -27,7 +27,7 @@ SELECT 'world' AS "text" \;
 COMMIT;
 
 -- compound with empty statements and spurious leading spacing
-\;\;   SELECT 3 + 3 \;\;\;   SELECT ' ' || ' !' \;\;   SELECT 1 + 4 \;;
+\;\;   SELECT 3 + 3 AS "+" \;\;\;   SELECT ' ' || ' !' AS "||" \;\;   SELECT 1 + 4 AS "+" \;;
 
 -- non ;-terminated statements
 SELECT 1 + 1 + 1 AS "add" \gset
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index 7789fc6177..4e6ab5a0a5 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -127,18 +127,11 @@ echo '\x \\ SELECT * FROM foo;' | psql
commands included in the string to divide it into multiple
transactions.  (See 
for more details about how the server handles multi-query strings.)
-   Also, psql only prints the
-   result of the last SQL command in the string.
-   This is different from the behavior when the same string is read from
-   a file or fed to psql's standard input,
-   because then psql sends
-   each SQL command separately.
   
   
-   Because of this behavior, putting more than one SQL command in a
-   single -c string often has unexpected results.
-   It's better to use repeated -c commands or feed
-   multiple commands to psql's standard input,
+   If having several commands executed in one transaction is not desired, 
+   use repeated -c commands or feed multiple commands to
+   psql's standard input,
either using echo as illustrated above, or
via a shell here-document, for example:
 
@@ -3355,10 +3348,8 @@ select 1\; select 2\; select 3;
 commands included in the string to divide it into multiple
 transactions.  (See 
 for more details about how the server handles multi-query strings.)
-psql prints only the last query result
-it receives for each request; in this example, although all
-three SELECTs are indeed executed, psql
-only prints the 3.
+psql prints all results it receives, one
+after the other.
 
 
   
diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
index 44a782478d..4534c45854 100644
--- a/src/bin/psql/common.c
+++ b/src/bin/psql/common.c
@@ -472,6 +472,16 @@ ResetCancelConn(void)
 #endif
 }
 
+static void
+ShowErrorMessage(const PGresult *result)
+{
+	const char *error = PQerrorMessage(pset.db);
+
+	if (strlen(error))
+		

Re: Patch for SortSupport implementation on inet/cdir

[Brandur Leach]

Thanks the follow ups on this one Edmund/Peter!

I've attached a new V4 variant of the patch based on
Peter's V3, mostly containing comment amendments and a few
other minor stylistic fixes.

> An interesting thing about sorting IPv4 inets on 64-bit machines is that
when the inets are the same, the abbreviated comparitor will return 0 which
is taken by the sorting machinery to mean "the datums are the same up to
this point, so you need to call the full comparitor" — but, in this case, 0
means "the datums truly are the same, no need to call the full
comparitor".  Since the full comparitor assumes its arguments to be the
original (typically pass-by-reference) datums, you can't do it there.
You'd need to add another optional comparitor to be called after the
abbreviated one.  In inet's case on a 64-bit machine, it would look at the
abbreviated datums and if they're both in the IPv4 family, would return 0
(knowing that the abbreviated comparitor has already done the real work).
I have no reason to believe this particular optimisation is worth anything
much, though; it's outside the scope of this patch, besides.

Edmund: thanks a lot for the really quick review turn
around, and apologies for not following up sooner!

Agreed that this change is out-of-scope, but it could be an
interesting improvement. You'd have similar potential speed
improvements for other SortSupport data types like uuid,
strings (short ones), and macaddr. Low cardinality data
sets would probably benefit the most.

> When you say "comparison is done" it sounds like more comparing is going
to be done, but what I think you mean is that comparison is finished.

Peter's version of my patch ended up stripping out and/or
changing some of my original comments, so most of them are
fixed by virtue of that. And agreed about the ambiguity in
wording above — I changed "done" to "finished".

> I didn't see any links for [1], [2] and [3] in your email.

Good catch. Here are the original footnote links:

[1] https://github.com/brandur/inet-sortsupport-test
[2]
https://github.com/brandur/inet-sortsupport-test/blob/master/results.md
[3]
https://github.com/brandur/postgres/compare/master...brandur-inet-sortsupport-unit#diff-a28824d1339d3bb74bb0297c60140dd1

> The way that you put a Min() on the subnet size potentially constrains
> the size of the bitmask used for the network component of the
> abbreviated key (the component that comes immediately after the
> ipfamily status bit). Why not just let the bitmask be a bitmask,
> without bringing SIZEOF_DATUM into it? Doing it that way allowed for a
> more streamlined approach, with significantly fewer special cases.

Peter: thanks a lot for the very thorough look and revised
version! Generally agreed that fewer special cases is good,
but I was also trying to make sure that we didn't
compromise the code's understandability by optimizing for
fewer special cases above everything else (especially for
this sort of thing where tricky bit manipulation is
involved).

But I traced through your variant and it looks fine to me
(still looks correct, and readability is still good). I've
pulled most of your changes into V4.

> I'm not sure whether or not your approach had bugs, but I
> didn't like the way you sometimes did a straight "network
> = ipaddr_datum" assignment without masking.

For what it's worth, I believe this did work, even if it
did depend on being within that one branch of code. Agreed
though that avoiding it (as in the new version) is more
hygienic.

> I really liked your diagrams, but much of the text that went with them
> either seemed redundant (it described established rules about how the
> underlying types sort), or seemed to discuss things that were better
> discussed next to the relevant network_abbrev_convert() code.

Thanks! I kept most of your changes, but resurrected some
of my original introductory text. The fine points of the
code's implementation are intricate enough that I think
having some background included is useful to new entrants;
specifically:

1. Norms for naming the different "parts" (network, size,
   subnet) of an inet/cidr value aren't broadly
   well-established, so defining what they are before
   showing diagrams is helpful.

2. Having examples of each part (`1.2.3.0`, `/24`,
   `0.0.0.4`) helps mentally cement them.

3. I know that it's in the PG documentation, but the rules
   for sorting inet/cidr are not very intuitive. Spending a
   few lines re-iterating them so that they don't have to
   be cross-referenced elsewhere is worth the space.

Anyway, once again appreciate the extreme attention to
detail on these reviews — this level of rigor would be a
very rare find in projects outside of the Postgres
community!

Brandur


v4-0001-SortSupport-for-inet-cidr-types.patch
Description: Binary data

Re: LLVM compile failing in seawasp

[Fabien COELHO]

Hello Tom,


They should be fully independent anyway, so the order should
not matter?


On what grounds do you claim that's true anywhere, let alone
everywhere?


I mean that the intersection of Postgres realm, a database written in C, 
and LLVM realm, a compiler written in C++, should not interfere much one 
with the other, bar the jit compilation stuff which mixes both, so having 
one set of realm-specific includes before/after the other *should* not 
matter.


Obviously the Min macro is a counter example of that, but that is indeed 
the problem to solve, and it is really accidental. It would be very 
unlucky if there was an issue the other way around. But maybe not.


Anyway, I'm just trying to suggest a minimum fuss solution. One point of 
"seawasp" and "jellyfish" is to have early warning of compilation issues 
with future compilers, and it is serving this purpose beautifully. Another 
point is to detect compiler bugs early when compiling a significant 
project, and I have reported issues about both clang & gcc in the past, so 
it works there too.


If reordering includes is not an option, too bad. Then "#undef Min" which 
I find disputable, allthough I've done much worse... it might or might not 
work depending on what is done afterwards. Or rename the macro, as I 
suggested first, but there are many instances. Or convince LLVM people 
that they should change their stuff. Or document that pg jit will cannot 
use the latest LLVM, as a feature. Or find another solution:-)


--
Fabien.

Re: how to run encoding-dependent tests by default

[Tom Lane]

Oh ... one other thought, based on forcing the collate.linux.utf8
test to run on platforms where it can be expected to fail: I think
you'd be well advised to make that test verify that the required
collations are present, the same as you did in the collate.icu.utf8
test.  I noticed for instance that it fails if en_US.utf8 is not
present (or not spelled exactly like that), but I doubt that that
locale is necessarily present on every Linux platform.  tr_TR is
even more likely to be subject to packagers' whims.

regards, tom lane

Re: ANALYZE: ERROR: tuple already updated by self

[Tomas Vondra]

On Sun, Jul 28, 2019 at 09:42:44AM -0700, Andres Freund wrote:

Hi,

On 2019-07-28 12:15:20 +0200, Tomas Vondra wrote:

Attached is a patch fixing the error by not building extended stats for
the inh=true case (as already proposed in this thread). That's about the
simplest way to resolve this issue for v12. It should add a simple
regression test too, I guess.


Doesn't this also apply to v11?



AFAICS it applies to 10+ versions, because that's where extended stats
were introduced. We certainly can't mess with catalogs there, so this is
about the only backpatchable fix I can think of.

regards

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

Re: Index Skip Scan

[Dmitry Dolgov]

> On Thu, Jul 25, 2019 at 1:21 PM Kyotaro Horiguchi  
> wrote:
>
> I have some comments.

Thank you for the review!

> +   * The order of columns in the index should be the same, as for
> +   * unique distincs pathkeys, otherwise we cannot use _bt_search
> +   * in the skip implementation - this can lead to a missing
> +   * records.
>
> It seems that it is enough that distinct pathkeys is contained in
> index pathkeys. If it's right, that is almost checked in existing
> code:

Looks like you're right. After looking closely there seems to be an issue in
the original implementation, when we use wrong prefix_size in such cases.
Without this problem this condition is indeed enough.

> >if (enable_indexskipscan &&
> >  IsA(path, IndexPath) &&
> >  ((IndexPath *) path)->indexinfo->amcanskip &&
> >  (path->pathtype == T_IndexOnlyScan ||
> >   path->pathtype == T_IndexScan) &&
> >  (needed_pathkeys == root->distinct_pathkeys ||
> >   pathkeys_contained_in(root->distinct_pathkeys,
> >   path->pathkeys)))
>
> path->pathtype is always one of T_IndexOnlyScan or T_IndexScan so
> the check against them isn't needed. If you have concern on that,
> we can add that as Assert().
>
> +  if (path->pathtype == T_IndexScan &&
> +parse->jointree != NULL &&
> +parse->jointree->quals != NULL &&
> +((List *)parse->jointree->quals)->length != 0)
>
> It's better to use list_length instead of peeping inside. It
> handles the NULL case as well. (The structure has recently
> changed but .length is not, though.)

Yeah, will change both (hopefully soon)

> +  /*
> +   * XXX: In case of index scan quals evaluation happens after
> +   * ExecScanFetch, which means skip results could be fitered out
> +   */
>
> Why can't we use skipscan path if having filter condition?  If
> something bad happens, the reason must be written here instead of
> what we do.

Sorry, looks like I've failed to express this more clear in the commentary. The
point is that when an index scan (not for index only scan) has some conditions,
their evaluation happens after skipping, and I don't see any not too much
invasive way to apply skip correctly.

>
> + * If advancing direction is different from index direction, we must
> + * skip right away, but _bt_skip requires a starting point.
>
> It doesn't seem needed to me. Could you elaborate on the reason
> for that?

This is needed for e.g. scan with a cursor backward without an index condition.
E.g. if we have:

1 1 2 2 3 3
1 2 3 4 5 6

and do

DECLARE c SCROLL CURSOR FOR
SELECT DISTINCT ON (a) a,b FROM ab ORDER BY a, b;

FETCH ALL FROM c;

we should get

1 2 3
1 3 5

When afterwards we do

FETCH BACKWARD ALL FROM c;

we should get

3 2 1
5 2 1

If we will use _bt_next first time without _bt_skip, the first pair would be
3 6 (the first from the end of the tuples, not from the end of the cursor).

> + * If advancing direction is different from index direction, we must
> + * skip right away, but _bt_skip requires a starting point.
> + */
> +if (direction * indexonlyscan->indexorderdir < 0 &&
> +  !node->ioss_FirstTupleEmitted)
>
> I'm confused by this. "direction" there is the physical scan
> direction (fwd/bwd) of index scan, which is already compensated
> by indexorderdir. Thus the condition means we do that when
> logical ordering (ASC/DESC) is DESC. (Though I'm not sure what
> "index direction" exactly means...)

I'm not sure I follow, what do you mean by compensated? In general you're
right, as David Rowley mentioned above, indexorderdir is a general scan
direction, and direction is flipped estate->es_direction, which is a cursor
direction. The goal of this condition is catch when those two are different,
and we need to advance and read in different directions.

Re: how to run encoding-dependent tests by default

[Tom Lane]

I wrote:
> I'm less clear on a reasonable way to detect a glibc platform
> from SQL.  The best I can think of is to see if the string
> "linux" appears in the output of version(), and that's probably
> none too robust.  Can we do anything based on the content of
> pg_collation?  Probably not :-(.

Actually, scraping the buildfarm database suggests that checking
version() for "linux" or even "linux-gnu" would work very well.

regards, tom lane

sysname|  snapshot   |  
 l  
  
---+-+
 alabio| 2019-07-27 16:00:08 | #define PG_VERSION_STR "PostgreSQL 
13devel on x86_64-pc-linux-gnu, compiled by gcc (Debian 6.3.0-18+deb9u1) 6.3.0 
20170516, 64-bit"
 alewife   | 2019-07-28 08:43:19 | #define PG_VERSION_STR "PostgreSQL 
13devel on s390x-ibm-linux-gnu, compiled by gcc (SUSE Linux) 4.8.5, 64-bit"
 anole | 2019-07-28 09:31:14 | #define PG_VERSION_STR "PostgreSQL 
13devel on ia64-hp-hpux11.31, compiled by cc, 64-bit"
 aye-aye   | 2019-07-27 19:01:09 | #define PG_VERSION_STR "PostgreSQL 
13devel on s390x-ibm-linux-gnu, compiled by gcc (GCC) 4.8.5 20150623 (Red Hat 
4.8.5-36), 64-bit"
 ayu   | 2019-07-27 21:43:27 | #define PG_VERSION_STR "PostgreSQL 
13devel on powerpc64le-unknown-linux-gnu, compiled by clang version 
4.0.1-10~deb9u2 (tags/RELEASE_401/final), 64-bit"
 batfish   | 2019-07-16 18:57:42 | #define PG_VERSION_STR "PostgreSQL 
13devel on powerpc64le-unknown-linux-gnu, compiled by clang version 
5.0.0-3~16.04.1 (tags/RELEASE_500/final), 64-bit"
 blenny| 2019-07-27 21:53:19 | #define PG_VERSION_STR "PostgreSQL 
13devel on s390x-ibm-linux-gnu, compiled by gcc (Debian 8.3.0-19) 8.3.0, 64-bit"
 bonito| 2019-07-27 22:22:23 | #define PG_VERSION_STR "PostgreSQL 
13devel on powerpc64le-unknown-linux-gnu, compiled by gcc (GCC) 8.2.1 20181215 
(Red Hat 8.2.1-6), 64-bit"
 brotula   | 2019-05-30 03:01:02 | #define PG_VERSION_STR "PostgreSQL 
12beta1 on powerpc64le-unknown-linux-gnu, compiled by clang version 3.8.1-24 
(tags/RELEASE_381/final), 64-bit"
 bufflehead| 2019-07-27 20:22:17 | #define PG_VERSION_STR "PostgreSQL 
13devel on powerpc64le-unknown-linux-gnu, compiled by gcc (SUSE Linux) 4.8.5, 
64-bit"
 buri  | 2019-07-27 23:00:48 | #define PG_VERSION_STR "PostgreSQL 
13devel on powerpc64le-unknown-linux-gnu, compiled by gcc (GCC) 4.8.5 20150623 
(Red Hat 4.8.5-36), 64-bit"
 butterflyfish | 2019-07-28 12:00:13 | #define PG_VERSION_STR "PostgreSQL 
13devel on x86_64-pc-linux-gnu, compiled by gcc (GCC) 6.3.0, 64-bit"
 caiman| 2019-07-22 19:34:03 | #define PG_VERSION_STR "PostgreSQL 
13devel on x86_64-pc-linux-gnu, compiled by gcc (GCC) 9.1.1 20190605 (Red Hat 
9.1.1-2), 64-bit"
 calliphoridae | 2019-07-28 07:30:07 | #define PG_VERSION_STR "PostgreSQL 
13devel on x86_64-pc-linux-gnu, compiled by gcc (Debian 8.3.0-19) 8.3.0, 64-bit"
 cardinalfish  | 2019-07-01 02:18:58 | #define PG_VERSION_STR "PostgreSQL 
12beta2 on powerpc64le-unknown-linux-gnu, compiled by clang version 7.0.1-8 
(tags/RELEASE_701/final), 64-bit"
 cavefish  | 2019-07-28 03:22:31 | #define PG_VERSION_STR "PostgreSQL 
13devel on powerpc64le-unknown-linux-gnu, compiled by gcc (Ubuntu 
7.3.0-27ubuntu1~18.04) 7.3.0, 64-bit"
 chimaera  | 2019-07-28 01:22:22 | #define PG_VERSION_STR "PostgreSQL 
13devel on powerpc64le-unknown-linux-gnu, compiled by gcc (Debian 
6.3.0-18+deb9u1) 6.3.0 20170516, 64-bit"
 chipmunk  | 2019-07-27 19:52:25 | #define PG_VERSION_STR "PostgreSQL 
13devel on armv6l-unknown-linux-gnueabihf, compiled by gcc (Debian 
4.6.3-14+rpi1) 4.6.3, 32-bit"
 chromis   | 2019-07-28 00:54:52 | #define PG_VERSION_STR "PostgreSQL 
13devel on s390x-ibm-linux-gnu, compiled by clang version 7.0.1-8 
(tags/RELEASE_701/final), 64-bit"
 chub  | 2019-07-28 15:10:01 | #define PG_VERSION_STR "PostgreSQL 
13devel on powerpc64-unknown-linux-gnu, compiled by gcc (GCC) 4.8.5 20150623 
(Red Hat 4.8.5-36), 64-bit"
 clam  | 2019-07-28 15:28:15 | #define PG_VERSION_STR "PostgreSQL 
13devel on powerpc64le-unknown-linux-gnu, compiled by gcc (GCC) 5.2.1 20151016 
(Advance-Toolchain-) [ibm/gcc-5-branch, revision: 229493 merged from 
gcc-5-branch, revision 228917], 64-bit"
 cobia | 2019-05-16 11:43:59 | #define PG_VERSION_STR "PostgreSQL 
12devel on s390x-ibm-linux-gnu, compiled by gcc (Debian 6.3.0-18+deb9u1) 6.3.0 
20170516, 64-bit"
 conchuela | 2019-07-28 10:00:00 | #define PG_VERSION_STR "PostgreSQL 
13devel on x86_64-unknown-dragonfly4.4, compiled by gcc 5.2.1 [DragonFly] 
Release/2015-07-18, 64-bit"
 coypu | 2019-07-10 03:35:55 | #define 

Re: how to run encoding-dependent tests by default

[Tom Lane]

Peter Eisentraut  writes:
>> Cool, that works out quite well.  See attached patch.  I flipped the
>> logic around to make it \quit if not compatible.  That way the
>> alternative expected file is shorter and doesn't need to be updated all
>> the time.  But it gets the job done either way.

I took a look at this and did some light testing.  It seems to work
as advertised, but I do have one gripe, which is the dependency on
the EXTRA_TESTS mechanism.  There are a few things not to like about
doing it that way:

* need additional hacking for Windows (admittedly, moot for
collate.linux.utf8, but I hope it's not for collate.icu.utf8).

* can't put these tests into a parallel group, they run by themselves;

* if user specifies EXTRA_TESTS on make command line, that overrides
the Makefile so these tests aren't run.

So I wish we could get rid of the Makefile changes, have the test
scripts be completely responsible for whether to run themselves or
not, and put them into the schedule files normally.

It's pretty obvious how we might do this for collate.icu.utf8:
make it look to see if there are any ICU-supplied collations in
pg_collation.

I'm less clear on a reasonable way to detect a glibc platform
from SQL.  The best I can think of is to see if the string
"linux" appears in the output of version(), and that's probably
none too robust.  Can we do anything based on the content of
pg_collation?  Probably not :-(.

Still, even if you only fixed collate.icu.utf8 this way, that
would be a step forward since it would solve the Windows aspect.

regards, tom lane

Re: [PATCH] Add support for ON UPDATE/DELETE actions on ALTER CONSTRAINT

[Tom Lane]

Matheus de Oliveira  writes:
> [ postgresql-alter-constraint.v5.patch ]

Somebody seems to have marked this Ready For Committer without posting
any review, which is not very kosher, but I took a quick look at it
anyway.

* It's failing to apply, as noted by the cfbot, because somebody added
an unrelated test to the same spot in foreign_key.sql.  I fixed that
in the attached rebase.

* It also doesn't pass "git diff --check" whitespace checks, so
I ran it through pgindent.

* Grepping around for other references to struct Constraint,
I noticed that you'd missed updating equalfuncs.c.  I did not
fix that.

The main issue I've got though is a definitional one --- I'm not at all
sold on your premise that constraint deferrability syntax should mean
different things depending on the previous state of the constraint.
We don't generally act that way in other ALTER commands and I don't see
a strong argument to start doing so here.  If you didn't do this then
you wouldn't (I think) need the extra struct Constraint fields in the
first place, which is why I didn't run off and change equalfuncs.c.

In short, I'm inclined to argue that this variant of ALTER TABLE
should replace *all* the fields of the constraint with the same
properties it'd have if you'd created it fresh using the same syntax.
This is by analogy to CREATE OR REPLACE commands, which don't
preserve any of the old properties of the replaced object.  Given
the interactions between these fields, I think you're going to end up
with a surprising mess of ad-hoc choices if you do differently.
Indeed, you already have, but I think it'll get worse if anyone
tries to extend the feature set further.

Perhaps the right way to attack it, given that, is to go ahead and
invent "ALTER TABLE t ADD OR REPLACE CONSTRAINT c ...".  At least
in the case at hand with FK constraints, we could apply suitable
optimizations (ie skip revalidation) when the new definition shares
the right properties with the old, and otherwise treat it like a
drop-and-add.

regards, tom lane

diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index 90bf195..198c640 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -55,7 +55,9 @@ ALTER TABLE [ IF EXISTS ] name
 ALTER [ COLUMN ] column_name SET STORAGE { PLAIN | EXTERNAL | EXTENDED | MAIN }
 ADD table_constraint [ NOT VALID ]
 ADD table_constraint_using_index
-ALTER CONSTRAINT constraint_name [ DEFERRABLE | NOT DEFERRABLE ] [ INITIALLY DEFERRED | INITIALLY IMMEDIATE ]
+ALTER CONSTRAINT constraint_name
+  [ ON DELETE action ] [ ON UPDATE action ]
+  [ DEFERRABLE | NOT DEFERRABLE ] [ INITIALLY DEFERRED | INITIALLY IMMEDIATE ]
 VALIDATE CONSTRAINT constraint_name
 DROP CONSTRAINT [ IF EXISTS ]  constraint_name [ RESTRICT | CASCADE ]
 DISABLE TRIGGER [ trigger_name | ALL | USER ]
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index fb2be10..f897986 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -9012,8 +9012,43 @@ ATExecAlterConstraint(Relation rel, AlterTableCmd *cmd,
  errmsg("constraint \"%s\" of relation \"%s\" is not a foreign key constraint",
 		cmdcon->conname, RelationGetRelationName(rel;
 
+	/*
+	 * Verify for FKCONSTR_ACTION_UNKNOWN, if found, replace by current
+	 * action. We could handle FKCONSTR_ACTION_UNKNOWN bellow, but since we
+	 * already have to handle the case of changing to the same action, seems
+	 * simpler to simple replace FKCONSTR_ACTION_UNKNOWN by the current action
+	 * here.
+	 */
+	if (cmdcon->fk_del_action == FKCONSTR_ACTION_UNKNOWN)
+		cmdcon->fk_del_action = currcon->confdeltype;
+
+	if (cmdcon->fk_upd_action == FKCONSTR_ACTION_UNKNOWN)
+		cmdcon->fk_upd_action = currcon->confupdtype;
+
+	/*
+	 * Do the same for deferrable attributes. But consider that if changed
+	 * only initdeferred attribute and to true, force deferrable to be also
+	 * true. On the other hand, if changed only deferrable attribute and to
+	 * false, force initdeferred to be also false.
+	 */
+	if (!cmdcon->was_deferrable_set)
+		cmdcon->deferrable = cmdcon->initdeferred ? true : currcon->condeferrable;
+
+	if (!cmdcon->was_initdeferred_set)
+		cmdcon->initdeferred = !cmdcon->deferrable ? false : currcon->condeferred;
+
+	/*
+	 * This is a safe check only, should never happen here.
+	 */
+	if (cmdcon->initdeferred && !cmdcon->deferrable)
+		ereport(ERROR,
+(errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("constraint declared INITIALLY DEFERRED must be DEFERRABLE")));
+
 	if (currcon->condeferrable != cmdcon->deferrable ||
-		currcon->condeferred != cmdcon->initdeferred)
+		currcon->condeferred != cmdcon->initdeferred ||
+		currcon->confdeltype != cmdcon->fk_del_action ||
+		currcon->confupdtype != cmdcon->fk_upd_action)
 	{
 		HeapTuple	copyTuple;
 		HeapTuple	tgtuple;
@@ -9031,6 +9066,8 @@ ATExecAlterConstraint(Relation rel, 

Re: Add parallelism and glibc dependent only options to reindexdb

[Tom Lane]

Andres Freund  writes:
> On 2019-07-28 10:07:27 -0400, Tom Lane wrote:
>> In the long run, might we ever switch to 64-bit OIDs?  I dunno.

> Depends on the the table, I'd say. Having toast tables have 64bit ids,
> and not advance the oid counter, would be quite the advantage over the
> current situation. Toasting performance craters once the oid counter has
> wrapped. But obviously there are upgrade problems there - presumably
> we'd need 'narrow" and 'wide' toast tables, or such.

Yeah, but I'd be inclined to fix toast tables as a special case,
rather than widening OIDs in general.  We could define the chunk
number as being int8 not OID for the "wide" style.

regards, tom lane

Re: ANALYZE: ERROR: tuple already updated by self

[Andres Freund]

Hi,

On 2019-07-28 12:15:20 +0200, Tomas Vondra wrote:
> Attached is a patch fixing the error by not building extended stats for
> the inh=true case (as already proposed in this thread). That's about the
> simplest way to resolve this issue for v12. It should add a simple
> regression test too, I guess.

Doesn't this also apply to v11?

Greetings,

Andres Freund

Re: Add parallelism and glibc dependent only options to reindexdb

[Andres Freund]

Hi,

On 2019-07-28 10:07:27 -0400, Tom Lane wrote:
> In the long run, might we ever switch to 64-bit OIDs?  I dunno.
> Now that we kicked them out of user tables, it might be feasible,
> but by the same token there's not much pressure to do it.

Depends on the the table, I'd say. Having toast tables have 64bit ids,
and not advance the oid counter, would be quite the advantage over the
current situation. Toasting performance craters once the oid counter has
wrapped. But obviously there are upgrade problems there - presumably
we'd need 'narrow" and 'wide' toast tables, or such.

Greetings,

Andres Freund

Re: Testing LISTEN/NOTIFY more effectively

[Tom Lane]

Andres Freund  writes:
> On 2019-07-27 12:46:51 -0400, Tom Lane wrote:
>> 2. Change psql so that there's a way to get NOTIFY messages without
>> the sending PID.  For testing purposes, it'd be sufficient to know
>> whether the sending PID is our own backend's PID or not.  This idea
>> is not horrible, and it might even be useful for outside purposes
>> if we made it flexible enough; which leads to thoughts like allowing
>> the psql user to set a format-style string, similar to the PROMPT
>> strings but with escapes for channel name, payload, etc.  I foresee
>> bikeshedding, but we could probably come to an agreement on a feature
>> like that.

> I was wondering about just tying it to VERBOSITY. But that'd not allow
> us to see whether our backend was the sender. I'm mildly inclined to
> think that that might still be a good idea, even if we mostly go with
> 3) - some basic plain regression test coverage of actually receiving
> notifies would be good.

BTW, as far as that goes, do you think we could get away with changing
psql to print "from self" instead of "from PID n" when it's a self-notify?
That would be enough to make the output stable for cases that we'd be
able to check in the core test infrastructure.

So far as the backend is concerned, doing anything there is redundant
with the isolation tests I just committed --- but it would allow psql's
own notify code to be covered, so maybe it's worth the trouble.

regards, tom lane

Re: Add parallelism and glibc dependent only options to reindexdb

[Tom Lane]

Michael Paquier  writes:
> Just wondering something...  List cells include one pointer, one
> signed integer and an OID.  The two last entries are basically 4-byte
> each, hence could we reduce a bit the bloat by unifying both of them?

We couldn't really simplify the API that way; for example,
lfirst_int and lfirst_oid *must* be different because they
must return different types.  I think it'd be a bad idea
to have some parts of the API that distinguish the two types
while others pretend they're the same, so there's not much
room for shortening that.

You could imagine unifying the implementations of many of the
_int and _oid functions, but I can't get excited about that.
It would add confusion for not a lot of code savings.

> I understand that the distinction exists because both may not be of
> the same size..

Well, even more to the point, one's signed and one isn't.

In the long run, might we ever switch to 64-bit OIDs?  I dunno.
Now that we kicked them out of user tables, it might be feasible,
but by the same token there's not much pressure to do it.

regards, tom lane

Re: LLVM compile failing in seawasp

[Tom Lane]

Fabien COELHO  writes:
> Otherwise, why not simply move llvm C++ includes *before* postgres 
> includes?

We've been burnt in the past by putting other headers before postgres.h.
(A typical issue is that the interpretation of  varies depending
on _LARGE_FILES or a similar macro, so you get problems if something
causes that to be included before pg_config.h has set that macro.)
Maybe none of the platforms where that's an issue have C++, but that
doesn't seem like a great assumption.

> They should be fully independent anyway, so the order should 
> not matter?

On what grounds do you claim that's true anywhere, let alone
everywhere?

regards, tom lane

Re: ANALYZE: ERROR: tuple already updated by self

[Tomas Vondra]

On Tue, Jul 23, 2019 at 01:01:27PM -0700, Andres Freund wrote:

Hi,

On 2019-06-18 17:08:37 -0700, Andres Freund wrote:

On 2019-06-18 18:48:58 -0500, Justin Pryzby wrote:
> Ah: the table is an inheritence parent.  If I uninherit its child, there's no
> error during ANALYZE.  MV stats on the child are ok:

It's a "classical" inheritance parent, not a builtin-partitioning type
of parent, right? And it contains data?

I assume it ought to not be too hard to come up with a reproducer
then...

I think the problem is pretty plainly that for inheritance tables we'll
try to store extended statistics twice. And thus end up updating the
same row twice.

> #6  0x00588346 in do_analyze_rel (onerel=0x7fee16140de8, options=2, 
params=0x7ffe5b6bf8b0, va_cols=0x0, acquirefunc=0x492b4, relpages=36, inh=true, 
in_outer_xact=false, elevel=13) at analyze.c:627

/* Build extended statistics (if there are any). */
BuildRelationExtStatistics(onerel, totalrows, numrows, rows, 
attr_cnt,
   
vacattrstats);

Note that for plain statistics we a) pass down the 'inh' flag to the
storage function, b) stainherit is part of pg_statistics' key:

Indexes:
"pg_statistic_relid_att_inh_index" UNIQUE, btree (starelid, staattnum, 
stainherit)


Tomas, I think at the very least extended statistics shouldn't be built
when building inherited stats. But going forward I think we should
probably have it as part of the key for pg_statistic_ext.


Tomas, ping?



Apologies, I kinda missed this thread until now. The volume of messages
on pgsql-hackers is getting pretty insane ...

Attached is a patch fixing the error by not building extended stats for
the inh=true case (as already proposed in this thread). That's about the
simplest way to resolve this issue for v12. It should add a simple
regression test too, I guess.

To fix this properly we need to add a flag similar to stainherit
somewhere. And I've started working on that, thinking that maybe we
could do that even for v12 - it'd be yet another catversion bump, but
there's already been one since beta2 so maybe it would be OK.

But it's actually a bit more complicated than just adding a column to
the catalog, for two reasons:

1) The optimizer part has to be tweaked to pick the right object, with
the flag set to either true/false. Not trivial, but doable.

2) We don't actually have a single catalog - we have two catalogs, one
for definition, one for built statistics. The flag does not seem to be
part of the definition, and we don't know whether there will be child
rels added sometime in the future, so presumably we'd store it in the
data catalog (pg_statistic_ext_data). Which means the code gets more
complex, because right now it assumes 1:1 relationship between those
catalogs.


regards

--
Tomas Vondra  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services 
>From 2bd4e4f1499f23c27c3488d73d6fe012232936c6 Mon Sep 17 00:00:00 2001
From: Tomas Vondra 
Date: Sun, 28 Jul 2019 11:41:31 +0200
Subject: [PATCH] don't build stats for inheritance trees

---
 src/backend/commands/analyze.c  | 4 ++--
 src/backend/statistics/extended_stats.c | 6 +-
 src/include/statistics/statistics.h | 2 +-
 3 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/src/backend/commands/analyze.c b/src/backend/commands/analyze.c
index 8d633f2585..9dc2aaadf7 100644
--- a/src/backend/commands/analyze.c
+++ b/src/backend/commands/analyze.c
@@ -575,8 +575,8 @@ do_analyze_rel(Relation onerel, VacuumParams *params,
}
 
/* Build extended statistics (if there are any). */
-   BuildRelationExtStatistics(onerel, totalrows, numrows, rows, 
attr_cnt,
-  
vacattrstats);
+   BuildRelationExtStatistics(onerel, inh, totalrows, numrows, 
rows,
+  attr_cnt, 
vacattrstats);
}
 
/*
diff --git a/src/backend/statistics/extended_stats.c 
b/src/backend/statistics/extended_stats.c
index 23c74f7d90..8a9a396765 100644
--- a/src/backend/statistics/extended_stats.c
+++ b/src/backend/statistics/extended_stats.c
@@ -80,7 +80,7 @@ static void statext_store(Oid relid,
  * requested stats, and serializes them back into the catalog.
  */
 void
-BuildRelationExtStatistics(Relation onerel, double totalrows,
+BuildRelationExtStatistics(Relation onerel, bool inh, double totalrows,
   int numrows, HeapTuple *rows,
   int natts, VacAttrStats 
**vacattrstats)
 {
@@ -90,6 +90,10 @@ BuildRelationExtStatistics(Relation onerel, double totalrows,
MemoryContext cxt;
MemoryContext oldcxt;
 
+   /* don't build stats for inheritance trees for now */
+   if (inh)
+   

Re: LLVM compile failing in seawasp

[Fabien COELHO]

Hello Thomas,


I would just #undef Min for our small number of .cpp files that
include LLVM headers.  It's not as though you need it in C++, which
has std::min() from .


Like so.  Fixes the problem for me (llvm-devel-9.0.d20190712).


Hmmm. Not so nice, but if it works, why not, at least the impact is 
much smaller than renaming.


Note that the Min macro is used in several pg headers (ginblock.h, 
ginxlog.h, hash.h, simplehash.h, spgist_private.h), so you might really 
need it depending on what is being done later.


Otherwise, why not simply move llvm C++ includes *before* postgres 
includes? They should be fully independent anyway, so the order should 
not matter?


--
Fabien.

Re: Add parallelism and glibc dependent only options to reindexdb

[Michael Paquier]

On Mon, Jul 22, 2019 at 01:05:32PM -0400, Alvaro Herrera wrote:
> Fair enough.  List has gotten quite sophisticated now, so I understand
> the concern.

Just wondering something...  List cells include one pointer, one
signed integer and an OID.  The two last entries are basically 4-byte
each, hence could we reduce a bit the bloat by unifying both of them?
I understand that the distinction exists because both may not be of
the same size..

/me runs and hides
--
Michael


signature.asc
Description: PGP signature

Re: fsync error handling in pg_receivewal, pg_recvlogical

[Michael Paquier]

On Sat, Jul 27, 2019 at 01:06:06PM -0400, Sehrope Sarkuni wrote:
> While reviewing this patch I read through some of the other fsync
> callsites and noticed this typo (walkdir is in file_utils.c, not
> initdb.c) too:

Thanks, Sehrope.  Applied.
--
Michael


signature.asc
Description: PGP signature

Re: Adding column "mem_usage" to view pg_prepared_statements

[Daniel Migowski]

Hello Andres,

how do you want to generalize it? Are you thinking about a view solely for the 
display of the memory usage of different objects? Like functions or views (that 
also have a plan associated with it, when I think about it)? While being 
interesting I still believe monitoring the mem usage of prepared statements is 
a bit more important than that of other objects because of how they change 
memory consumption of the server without using any DDL or configuration options 
and I am not aware of other objects with the same properties, or are there 
some? And for the other volatile objects like tables and indexes and their 
contents PostgreSQL already has it's information functions. 

Regardless of that here is the patch for now. I didn't want to fiddle to much 
with MemoryContexts yet, so it still doesn't recurse in child contexts, but I 
will change that also when I try to build a more compact MemoryContext 
implementation and see how that works out.

Thanks for pointing out the relevant information in the statement column of the 
view.

Regards,
Daniel Migowski

-Ursprüngliche Nachricht-
Von: Andres Freund  
Gesendet: Samstag, 27. Juli 2019 21:12
An: Daniel Migowski 
Cc: pgsql-hackers@lists.postgresql.org
Betreff: Re: Adding column "mem_usage" to view pg_prepared_statements

Hi,

On 2019-07-27 18:29:23 +, Daniel Migowski wrote:
> I just implemented a small change that adds another column "mem_usage"
> to the system view "pg_prepared_statements". It returns the memory 
> usage total of CachedPlanSource.context, 
> CachedPlanSource.query_content and if available 
> CachedPlanSource.gplan.context.

FWIW, it's generally easier to comment if you actually provide the patch, even 
if it's just POC, as that gives a better handle on how much additional 
complexity it introduces.

I think this could be a useful feature. I'm not so sure we want it tied to just 
cached statements however - perhaps we ought to generalize it a bit more.


Regarding the prepared statements specific considerations: I don't think we 
ought to explicitly reference CachedPlanSource.query_content, and 
CachedPlanSource.gplan.context.

In the case of actual prepared statements (rather than oneshot plans) 
CachedPlanSource.query_context IIRC should live under CachedPlanSource.context. 
 I think there's no relevant cases where gplan.context isn't a child of 
CachedPlanSource.context either, but not quite sure.

Then we ought to just include child contexts in the memory computation (cf. 
logic in MemoryContextStatsInternal(), although you obviously wouldn't need all 
that). That way, if the cached statements has child contexts, we're going to 
stay accurate.


> Also I wonder why the "prepare test as" is part of the statement 
> column. I isn't even part of the real statement that is prepared as 
> far as I would assume. Would prefer to just have the "select *..." in 
> that column.

It's the statement that was executed. Note that you'll not see that in the case 
of protocol level prepared statements.  It will sometimes include relevant 
information, e.g. about the types specified as part of the prepare (as in 
PREPARE foo(int, float, ...) AS ...).

Greetings,

Andres Freund


prepared_statements_mem_usage.diff
Description: prepared_statements_mem_usage.diff