Re: Improve search for missing parent downlinks in amcheck
On Sat, Apr 27, 2019 at 5:13 PM Alexander Korotkov wrote: > Yes, increasing of Bloom filter size also helps. But my intention was > to make non-lossy check here. Why is that your intention? Do you want to do this as a feature for Postgres 13, or do you want to treat this as a bug that we need to backpatch a fix for? Can we avoid the problem you saw with the Bloom filter approach by using the real size of the index (i.e. smgrnblocks()/RelationGetNumberOfBlocks()) to size the Bloom filter, and/or by rethinking the work_mem cap? Maybe we can have a WARNING that advertises that work_mem is probably too low? The state->downlinkfilter Bloom filter should be small in almost all cases, so I still don't fully understand your concern. With a 100GB index, we'll have ~13 million blocks. We only need a Bloom filter that is ~250MB to have less than a 1% chance of missing inconsistencies even with such a large index. I admit that its unfriendly that users are not warned about the shortage currently, but that is something we can probably find a simple (backpatchable) fix for. -- Peter Geoghegan
Re: Race conditions with checkpointer and shutdown
I have spent a fair amount of time trying to replicate these failures
locally, with little success. I now think that the most promising theory
is Munro's idea in [1] that the walreceiver is hanging up during its
unsafe attempt to do ereport(FATAL) from inside a signal handler. It's
extremely plausible that that could result in a deadlock inside libc's
malloc/free, or some similar place. Moreover, if that's what's causing
it, then the windows for trouble are fixed by the length of time that
malloc might hold internal locks, which fits with the results I've gotten
that inserting delays in various promising-looking places doesn't do a
thing towards making this reproducible.
Even if that isn't the proximate cause of the current reports, it's
clearly trouble waiting to happen, and we should get rid of it.
Accordingly, see attached proposed patch. This just flushes the
"immediate interrupt" stuff in favor of making sure that
libpqwalreceiver.c will take care of any signals received while
waiting for input.
The existing code does not use PQsetnonblocking, which means that it's
theoretically at risk of blocking while pushing out data to the remote
server. In practice I think that risk is negligible because (IIUC) we
don't send very large amounts of data at one time. So I didn't bother to
change that. Note that for the most part, if that happened, the existing
code was at risk of slow response to SIGTERM anyway since it didn't have
Enable/DisableWalRcvImmediateExit around the places that send data.
My thought is to apply this only to HEAD for now; it's kind of a large
change to shove into the back branches to handle a failure mode that's
not been reported from the field. Maybe we could back-patch after we
have more confidence in it.
regards, tom lane
[1]
https://www.postgresql.org/message-id/CA%2BhUKG%2B%3D1G98m61VjNS-qGboJPwdZcF%2BrAPu2eC4XuWRTR3UPw%40mail.gmail.com
diff --git a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
index 7123d41..765d58d 100644
--- a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
+++ b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
@@ -99,6 +99,7 @@ static WalReceiverFunctionsType PQWalReceiverFunctions = {
/* Prototypes for private functions */
static PGresult *libpqrcv_PQexec(PGconn *streamConn, const char *query);
+static PGresult *libpqrcv_PQgetResult(PGconn *streamConn);
static char *stringlist_to_identifierstr(PGconn *conn, List *strings);
/*
@@ -196,7 +197,7 @@ libpqrcv_connect(const char *conninfo, bool logical, const char *appname,
if (rc & WL_LATCH_SET)
{
ResetLatch(MyLatch);
- CHECK_FOR_INTERRUPTS();
+ ProcessWalRcvInterrupts();
}
/* If socket is ready, advance the libpq state machine */
@@ -456,6 +457,10 @@ libpqrcv_endstreaming(WalReceiverConn *conn, TimeLineID *next_tli)
{
PGresult *res;
+ /*
+ * Send copy-end message. As in libpqrcv_PQexec, this could theoretically
+ * block, but the risk seems small.
+ */
if (PQputCopyEnd(conn->streamConn, NULL) <= 0 ||
PQflush(conn->streamConn))
ereport(ERROR,
@@ -472,7 +477,7 @@ libpqrcv_endstreaming(WalReceiverConn *conn, TimeLineID *next_tli)
* If we had not yet received CopyDone from the backend, PGRES_COPY_OUT is
* also possible in case we aborted the copy in mid-stream.
*/
- res = PQgetResult(conn->streamConn);
+ res = libpqrcv_PQgetResult(conn->streamConn);
if (PQresultStatus(res) == PGRES_TUPLES_OK)
{
/*
@@ -486,7 +491,7 @@ libpqrcv_endstreaming(WalReceiverConn *conn, TimeLineID *next_tli)
PQclear(res);
/* the result set should be followed by CommandComplete */
- res = PQgetResult(conn->streamConn);
+ res = libpqrcv_PQgetResult(conn->streamConn);
}
else if (PQresultStatus(res) == PGRES_COPY_OUT)
{
@@ -499,7 +504,7 @@ libpqrcv_endstreaming(WalReceiverConn *conn, TimeLineID *next_tli)
pchomp(PQerrorMessage(conn->streamConn);
/* CommandComplete should follow */
- res = PQgetResult(conn->streamConn);
+ res = libpqrcv_PQgetResult(conn->streamConn);
}
if (PQresultStatus(res) != PGRES_COMMAND_OK)
@@ -509,7 +514,7 @@ libpqrcv_endstreaming(WalReceiverConn *conn, TimeLineID *next_tli)
PQclear(res);
/* Verify that there are no more results */
- res = PQgetResult(conn->streamConn);
+ res = libpqrcv_PQgetResult(conn->streamConn);
if (res != NULL)
ereport(ERROR,
(errmsg("unexpected result after CommandComplete: %s",
@@ -572,12 +577,11 @@ libpqrcv_readtimelinehistoryfile(WalReceiverConn *conn,
* The function is modeled on PQexec() in libpq, but only implements
* those parts that are in use in the walreceiver api.
*
- * Queries are always executed on the connection in streamConn.
+ * May return NULL, rather than an error result, on failure.
*/
static PGresult *
libpqrcv_PQexec(PGconn *streamConn, const char *query)
{
- PGresult *result = NULL;
PGresult *lastResult =
Re: Improve search for missing parent downlinks in amcheck
On Sat, Apr 27, 2019 at 5:13 PM Alexander Korotkov wrote: > Yes, increasing of Bloom filter size also helps. But my intention was > to make non-lossy check here. I agree that that might be a good goal, but I am interested in knowing if there is something naive about how the downlinkfilter Bloom filter is sized. I think that we could probably do better than this without much work: /* * Extra readonly downlink check. * * In readonly case, we know that there cannot be a concurrent * page split or a concurrent page deletion, which gives us the * opportunity to verify that every non-ignorable page had a * downlink one level up. We must be tolerant of interrupted page * splits and page deletions, though. This is taken care of in * bt_downlink_missing_check(). */ total_pages = (int64) state->rel->rd_rel->relpages; state->downlinkfilter = bloom_create(total_pages, work_mem, seed); Maybe we could use "smgrnblocks(index->rd_smgr, MAIN_FORKNUM))" instead of relpages, for example. > It wouldn't be really wasteful, because the same children were > accessed recently. So, they are likely not yet evicted from > shared_buffers. I think we can fit both checks into one loop over > downlinks instead of two. I see your point, but if we're going to treat this as a bug then I would prefer a simple fix. > Yes, we can do more checks with bloom filter. But assuming that we > anyway iterate over children for each non-leaf page, can we do: > 1) All the checks, which bt_downlink_check() does for now > 2) Check there are no missing downlinks > 3) Check hikeys > in one pass, can't we? We can expect every high key in a page to have a copy contained within its parent, either as one of the keys, or as parent's own high key (assuming no concurrent or interrupted page splits or page deletions). This is true today, even though we truncate negative infinity items in internal pages. I think that the simple answer to your question is yes. It would be more complicated that way, and the only extra check would be the check of high keys against their parent, but overall this does seem possible. -- Peter Geoghegan
Re: Improve search for missing parent downlinks in amcheck
On Tue, Apr 16, 2019 at 10:00 PM Peter Geoghegan wrote: > > On Mon, Apr 15, 2019 at 7:30 PM Alexander Korotkov > wrote: > > Currently we amcheck supports lossy checking for missing parent > > downlinks. It collects bitmap of downlink hashes and use it to check > > subsequent tree level. We've experienced some large corrupted indexes > > which pass this check due to its looseness. > > Can you be more specific? What was the cause of the corruption? I'm > always very interested in hearing about cases that amcheck could have > detected, but didn't. AFAIR, the cause of corruption in this case was our (Postgres Pro) modification. Not something really present in PostgreSQL core. > > Was the issue that the Bloom filter was simply undersized/ineffective? Yes, increasing of Bloom filter size also helps. But my intention was to make non-lossy check here. > > > However, it seems to me we can implement this check in non-lossy > > manner without making it significantly slower. We anyway traverse > > downlinks from parent to children in order to verify that hikeys are > > corresponding to downlink keys. > > Actually, we don't check the high keys in children against the parent > (all other items are checked, though). We probably *should* do > something with the high key when verifying consistency across levels, > but currently we don't. (We only use the high key for the same-page > high key check -- more on this below.) Nice idea. > > We can also traverse from one > > downlink to subsequent using rightlinks. So, if there are some > > intermediate pages between them, they are candidates to have missing > > parent downlinks. The patch is attached. > > > > With this patch amcheck could successfully detect corruption for our > > customer, which unpatched amcheck couldn't find. > > Maybe we can be a lot less conservative about sizing the Bloom filter > instead? That would be an easier fix IMV -- we can probably change > that logic to be a lot more aggressive without anybody noticing, since > the Bloom filter is already usually tiny. We are already not very > careful about saving work within bt_index_parent_check(), but with > this patch we follow each downlink twice instead of once. That seems > wasteful. It wouldn't be really wasteful, because the same children were accessed recently. So, they are likely not yet evicted from shared_buffers. I think we can fit both checks into one loop over downlinks instead of two. > The reason I used a Bloom filter here is because I would eventually > like the missing downlink check to fingerprint entire tuples, not just > block numbers. In L terms, the check could in the future fingerprint > the separator key and the downlink at the same time, not just the > downlink. That way, we could probe the Bloom filter on the next level > down for its high key (with the right sibling pointer set to be > consistent with the parent) iff we don't see that the page split was > interrupted (i.e. iff P_INCOMPLETE_SPLIT() bit is not set). Obviously > this would be a more effective form of verification, since we would > notice high key values that don't agree with the parent's values for > the same sibling/cousin/child block. > > I didn't do it that way for v11 because of "minus infinity" items on > internal pages, which don't store the original key (the key remains > the high key of the left sibling page, but we truncate the original to > 0 attributes in _bt_pgaddtup()). I think that we should eventually > stop truncating minus infinity items, and actually store a "low key" > at P_FIRSTDATAKEY() within internal pages instead. That would be > useful for other things anyway (e.g. prefix compression). Yes, we can do more checks with bloom filter. But assuming that we anyway iterate over children for each non-leaf page, can we do: 1) All the checks, which bt_downlink_check() does for now 2) Check there are no missing downlinks 3) Check hikeys in one pass, can't we? -- Alexander Korotkov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
Re: Improve search for missing parent downlinks in amcheck
On Sat, Apr 27, 2019 at 4:57 PM Alexander Korotkov wrote: > "rootdescend" is cool type of check. Thank you for noticing, I wasn't aware > of it. > But can it detect the missing downlink in following situation? > > A > / \ > B <-> C <-> D > > Here A has downlinks to B and D, which downlink to C is missing, > while B, C and D are correctly connected with leftlinks and rightlinks. > I can see "rootdescend" calls _bt_search(), which would just step > right from C to D as if it was concurrent split. There is a comment about this scenario above bt_rootdescend() in amcheck. You're right -- this is a kind of corruption that even the new rootdescend verification option would miss. We can imagine a version of rootdescend verification that tells the core code to only move right when there was an *interrupted* page split (i.e. P_INCOMPLETE_SPLIT() flag bit is set), but that isn't what happens right now. That said, the lossy downlink check that you want to improve on *should* already catch this situation. Of course it might not because it is lossy (uses a Bloom filter), but I think that that's very unlikely. That's why I would like to understand the problem that you found with the check. -- Peter Geoghegan
Re: Improve search for missing parent downlinks in amcheck
On Tue, Apr 16, 2019 at 10:04 PM Peter Geoghegan wrote: > > On Tue, Apr 16, 2019 at 12:00 PM Peter Geoghegan wrote: > > Can you be more specific? What was the cause of the corruption? I'm > > always very interested in hearing about cases that amcheck could have > > detected, but didn't. > > FWIW, v4 indexes in Postgres 12 will support the new "rootdescend" > verification option, which isn't lossy, and would certainly have > detected your customer issue in practice. Admittedly the new check is > quite expensive, even compared to the other bt_index_parent_check() > checks, but it is nice that we now have a verification option that is > *extremely* thorough, and uses _bt_search() directly. "rootdescend" is cool type of check. Thank you for noticing, I wasn't aware of it. But can it detect the missing downlink in following situation? A / \ B <-> C <-> D Here A has downlinks to B and D, which downlink to C is missing, while B, C and D are correctly connected with leftlinks and rightlinks. I can see "rootdescend" calls _bt_search(), which would just step right from C to D as if it was concurrent split. -- Alexander Korotkov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
Re: speeding up planning with partitions
Amit Langote writes: > On 2019/04/23 7:08, Tom Lane wrote: >> [ a bunch of stuff ] > Not sure if you'll like it but maybe we could ignore even regular > inheritance child targets that are proven to be empty (is_dummy_rel()) for > a given query during the initial SELECT planning. That way, we can avoid > re-running relation_excluded_by_constraints() a second time for *all* > child target relations. My thought was to keep traditional inheritance working more or less as it has. To do what you're suggesting, we'd have to move generic constraint-exclusion logic up into the RTE expansion phase, and I don't think that's a particularly great idea. I think what we should be doing is applying partition pruning (which is a very specialized form of constraint exclusion) during RTE expansion, then applying generic constraint exclusion in relation_excluded_by_constraints, and not examining partition constraints again there if we already used them. > Do you want me to update my patch considering the above summary? Yes please. However, I wonder whether you're thinking differently in light of what you wrote in [1]: >>> Pruning in 10.2 works using internally generated partition constraints >>> (which for this purpose are same as CHECK constraints). With the new >>> pruning logic introduced in 11, planner no longer considers partition >>> constraint because it's redundant to check them in most cases, because >>> pruning would've selected the right set of partitions. Given that the new >>> pruning logic is still unable to handle the cases like above, maybe we >>> could change the planner to consider them, at least until we fix the >>> pruning logic to handle such cases. If we take that seriously then it would suggest not ignoring partition constraints in relation_excluded_by_constraints. However, I'm of the opinion that we shouldn't let temporary deficiencies in the partition-pruning logic drive what we do here. I don't think the set of cases where we could get a win by reconsidering the partition constraints is large enough to justify the cycles expended in doing so; and it'll get even smaller as pruning gets smarter. regards, tom lane [1] https://www.postgresql.org/message-id/358cd54d-c018-60f8-7d76-55780eef6...@lab.ntt.co.jp
pg_ssl
As you might know, generating SSL certificates for postgres (to be used by pgadmin, for example...) can be quite a bear; especially if you need more than one, since they are based on the username of the postgres user. I have made two command-line utilities written in python 3.6 to do just that (I, as a number of other developers do, appreciate python for its ease of code inspection...); one is called *pg_ssl_server*, and the other is called *pg_ssl_client*. Packaged together, they are referred to by the name "*pg_ssl*". They are issued under the postgres license. They have been tested out on Ubuntu 18 and python 3.6.7 with postgres 11. They were designed to be cross-platform, but they have not been tested yet on Windows, OSx, BSD, or distros other than Ubuntu. [My immediate concern is with their ability to run cross-platform; as for downlevel versions of postgres or python, that is not a priority right now. The "subprocess" module in python used by the utilities has inconsistencies working cross-platform in older versions of python; _for now_, people should just upgrade if they really need to use them...] If anyone would be interested in testing these and sending back a notice as to what problems were encountered on their platform, it would be much appreciated. The availability of these utilities will remove a rather rough spot from the administration of postgres. To keep noise on this mail thread to a minimum, please report any problems encountered directly to my address. Also, if anyone is a security fanatic and facile with python, a code review would not be a bad idea (the two utilities check in at ~1,500 lines; but since it's python, it's an easy read...) The latest version of the utility can be retrieved here: https://osfda.org/downloads/pg_ssl.zip You can also use the Contact Form at osfda.org to report issues.
Re: nRe: [PATCH v1] Show whether tables are logged in \dt+
Hello David, Patch applies. There seems to be a compilation issue: describe.c:5974:1: error: expected declaration or statement at end of input } Also there is an added indentation problem: the size & description stuff have been moved left but it should still in the verbose case, and a } is missing after them, which fixes the compilation. Make check fails because of my temp schema was numbered 4 instead of 3, and I'm "fabien" rather than "shackle". I think the way forward is to test this with TAP rather than the fixed-string method. Ok. Checks removed while I figure out a new TAP test. I only have packages down to pg 9.3, so I could not test prior 9.1. By looking at the online documentation, is seems that relistemp appears in pg 8.4, so the corresponding extraction should be guarded by this version. Before that, temporary objects existed but were identified indirectly, possibly because they were stored in a temporary schema. I suggest not to try to address cases prior 8.4. Done. After some checking, I think that there is an issue with the version numbers: - 9.1 is 90100, not 91000 - 8.4 is 80400, not 84000 Also, it seems that describes builds queries with uppercase keywords, so probably the new additions should follow that style: case -> CASE (and also when then else end as…) -- Fabien.
Re: [PATCH v1] Add \echo_stderr to psql
Hello David, About v3. Applies, compiles, global & local make check are ok. doc gen ok. I'd put the commands in alphabetical order (echo, qecho, warn) instead of e/w/q in the condition. Done. Cannot see it: + else if (strcmp(cmd, "echo") == 0 || strcmp(cmd, "warn") == 0 || strcmp(cmd, "qecho") == 0) The -n trick does not appear in the help lines, ISTM that it could fit, so maybe it could be added, possibly something like: \echo [-n] [TEXT] write string to stdout, possibly without trailing newline and same for \warn and \qecho? Makes sense, but I put it there just for \echo to keep lines short. I think that putting together the 3 echo variants help makes sense, but maybe someone will object about breaking the abc order. How might we test this portably? Hmmm... TAP tests are expected to be portable. Attached a simple POC, which could be extended to test many more things which are currently out of coverage (src/bin/psql stuff is covered around 40% only). Thanks for putting this together. I've added this test, and agree that increasing coverage is important for another patch. Yep. -- Fabien.
Re: Failure in contrib test _int on loach
Anastasia Lubennikova writes: > So it is possible, but it doesn't require any extra algorithm changes. > I didn't manage to generate dataset to reproduce grandparent split. > Though, I do agree that it's worth checking out. Do you have any ideas? Ping? This thread has gone cold, but the bug is still there, and IMV it's a beta blocker. regards, tom lane
Re: nRe: [PATCH v1] Show whether tables are logged in \dt+
On Sat, Apr 27, 2019 at 09:19:57AM +0200, Fabien COELHO wrote:
>
> Hello David,
>
> Patch v3 applies, but compiles for me with a warning because the indentation
> of the following size block has been changed:
>
> describe.c: In function ‘listTables’:
> describe.c:3705:7: warning: this ‘if’ clause does not guard...
> [-Wmisleading-indentation]
> else if (pset.sversion >= 80100)
>^~
> describe.c:3710:3: note: ...this statement, but the latter is misleadingly
> indented as if it were guarded by the ‘if’
>appendPQExpBuffer(,
>^
Fixed.
> Make check fails because of my temp schema was numbered 4 instead of 3, and
> I'm "fabien" rather than "shackle".
I think the way forward is to test this with TAP rather than the
fixed-string method.
> > > > > > Included, but they're not stable for temp tables. I'm a little
> > > > > > stumped
> > > > > > as to how to either stabilize them or test some other way.
> > > > >
> > > > > Hmmm. First there is the username which appears, so there should be a
> > > > > dedicated user for the test.
> > > > >
> > > > > I'm unsure how to work around the temporary schema number, which is
> > > > > undeterministic with parallel execution it. I'm afraid the only viable
> > > > > approach is not to show temporary tables, too bad:-(
>
> The tests have not been fixed.
>
> I think that they need a dedicated user to replace "shackle", and I'm afraid
> that there temporary test schema instability cannot be easily fixed at the
> "psql" level, but would require some kind of TAP tests instead if it is to
> be checked. In the short term, do not.
Checks removed while I figure out a new TAP test.
> I checked that the \di+ works, though. I've played with temporary views and
> \dv as well.
Great!
> I discovered that you cannot have temporary unlogged objects, nor
> temporary or unlogged materialized views. Intuitively I'd have
> thought that these features would be orthogonal, but they are not.
This seems like material for a different patch.
> Also I created an unlogged table with a SERIAL which created a
> sequence. The table is unlogged but the sequence is permanent, which
> is probably ok.
> I only have packages down to pg 9.3, so I could not test prior 9.1.
> By looking at the online documentation, is seems that relistemp
> appears in pg 8.4, so the corresponding extraction should be guarded
> by this version. Before that, temporary objects existed but were
> identified indirectly, possibly because they were stored in a
> temporary schema. I suggest not to try to address cases prior 8.4.
Done.
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
>From fd0b8215ca6bbdebc0924efaba95944731890dc9 Mon Sep 17 00:00:00 2001
From: David Fetter
Date: Mon, 22 Apr 2019 17:50:48 -0700
Subject: [PATCH v4] Show detailed table persistence in \dt+
To: hackers
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="2.20.1"
This is a multi-part message in MIME format.
--2.20.1
Content-Type: text/plain; charset=UTF-8; format=fixed
Content-Transfer-Encoding: 8bit
\d would show this for individual tables, but there wasn't an
overarching view of all tables. Now, there is.
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index ee00c5da08..4aa06a417c 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -3631,7 +3631,7 @@ listTables(const char *tabtypes, const char *pattern, bool verbose, bool showSys
PQExpBufferData buf;
PGresult *res;
printQueryOpt myopt = pset.popt;
- static const bool translate_columns[] = {false, false, true, false, false, false, false};
+ static const bool translate_columns[] = {false, false, true, false, false, false, false, false};
/* If tabtypes is empty, we default to \dtvmsE (but see also command.c) */
if (!(showTables || showIndexes || showViews || showMatViews || showSeq || showForeign))
@@ -3680,22 +3680,36 @@ listTables(const char *tabtypes, const char *pattern, bool verbose, bool showSys
if (verbose)
{
/*
- * As of PostgreSQL 9.0, use pg_table_size() to show a more accurate
- * size of a table, including FSM, VM and TOAST tables.
+ * Show whether a table is permanent, temporary, or unlogged.
+ * Indexes are not, as of this writing, tables.
*/
- if (pset.sversion >= 9)
- appendPQExpBuffer(,
- ",\n pg_catalog.pg_size_pretty(pg_catalog.pg_table_size(c.oid)) as \"%s\"",
- gettext_noop("Size"));
- else if (pset.sversion >= 80100)
- appendPQExpBuffer(,
- ",\n pg_catalog.pg_size_pretty(pg_catalog.pg_relation_size(c.oid)) as \"%s\"",
- gettext_noop("Size"));
+ if (!showIndexes)
+ {
+ if (pset.sversion >= 91000)
+appendPQExpBuffer(,
+ ",\n CASE c.relpersistence WHEN 'p' THEN 'permanent' WHEN 't' THEN 'temporary' WHEN 'u' THEN 'unlogged' ELSE 'unknown' END as \"%s\"",
+
Re: [PATCH v1] Add \echo_stderr to psql
On Sat, Apr 27, 2019 at 04:05:20PM +0200, Fabien COELHO wrote: > > Hello David, > > > Please find attached v2, name is now \warn. > > Patch applies cleanly, compiles, "make check ok", although there are no > tests. Doc gen ok. > > Code is pretty straightforward. > > I'd put the commands in alphabetical order (echo, qecho, warn) instead of > e/w/q in the condition. Done. > The -n trick does not appear in the help lines, ISTM that it could fit, so > maybe it could be added, possibly something like: > > \echo [-n] [TEXT] write string to stdout, possibly without trailing newline > > and same for \warn and \qecho? Makes sense, but I put it there just for \echo to keep lines short. > > How might we test this portably? > > Hmmm... TAP tests are expected to be portable. Attached a simple POC, which > could be extended to test many more things which are currently out of > coverage (src/bin/psql stuff is covered around 40% only). Thanks for putting this together. I've added this test, and agree that increasing coverage is important for another patch. 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 >From 2f7cbf9980c5ce1049728a0e2a3444cbd106f253 Mon Sep 17 00:00:00 2001 From: David Fetter Date: Sun, 21 Apr 2019 11:08:40 -0700 Subject: [PATCH v3] Add \warn to psql To: hackers MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="2.20.1" This is a multi-part message in MIME format. --2.20.1 Content-Type: text/plain; charset=UTF-8; format=fixed Content-Transfer-Encoding: 8bit - Does what it says on the label - Adds TAP tests for same - In passing, expose the -n option for \echo, \qecho, and \warn in \? output create mode 100644 src/bin/psql/t/001_psql.pl --2.20.1 Content-Type: text/x-patch; name="v3-0001-Add-warn-to-psql.patch" Content-Transfer-Encoding: 8bit Content-Disposition: attachment; filename="v3-0001-Add-warn-to-psql.patch" diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml index b86764003d..4edf8e67a1 100644 --- a/doc/src/sgml/ref/psql-ref.sgml +++ b/doc/src/sgml/ref/psql-ref.sgml @@ -1879,6 +1879,26 @@ Tue Oct 26 21:40:57 CEST 1999 + +\warn text [ ... ] + + +Prints the arguments to the standard error, separated by one +space and followed by a newline. This can be useful to +intersperse information in the output of scripts when not polluting +standard output is desired. For example: + + += \echo :variable += \warn `date` +Sun Apr 21 10:48:11 PDT 2019 + +If the first argument is an unquoted -n the trailing +newline is not written. + + + + \ef function_description line_number diff --git a/src/bin/psql/.gitignore b/src/bin/psql/.gitignore index c2862b12d6..d324c1c1fa 100644 --- a/src/bin/psql/.gitignore +++ b/src/bin/psql/.gitignore @@ -3,3 +3,4 @@ /sql_help.c /psql +/tmp_check diff --git a/src/bin/psql/Makefile b/src/bin/psql/Makefile index 69bb297fe7..9473ab01cb 100644 --- a/src/bin/psql/Makefile +++ b/src/bin/psql/Makefile @@ -60,8 +60,15 @@ uninstall: clean distclean: rm -f psql$(X) $(OBJS) lex.backup + rm -rf tmp_check # files removed here are supposed to be in the distribution tarball, # so do not clean them in the clean/distclean rules maintainer-clean: distclean rm -f sql_help.h sql_help.c psqlscanslash.c + +check: + $(prove_check) + +installcheck: + $(prove_installcheck) diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c index 8254d61099..9425f02005 100644 --- a/src/bin/psql/command.c +++ b/src/bin/psql/command.c @@ -319,7 +319,7 @@ exec_command(const char *cmd, status = exec_command_ef_ev(scan_state, active_branch, query_buf, true); else if (strcmp(cmd, "ev") == 0) status = exec_command_ef_ev(scan_state, active_branch, query_buf, false); - else if (strcmp(cmd, "echo") == 0 || strcmp(cmd, "qecho") == 0) + else if (strcmp(cmd, "echo") == 0 || strcmp(cmd, "warn") == 0 || strcmp(cmd, "qecho") == 0) status = exec_command_echo(scan_state, active_branch, cmd); else if (strcmp(cmd, "elif") == 0) status = exec_command_elif(scan_state, cstack, query_buf); @@ -1114,7 +1114,7 @@ exec_command_ef_ev(PsqlScanState scan_state, bool active_branch, } /* - * \echo and \qecho -- echo arguments to stdout or query output + * \echo, \warn and \qecho -- echo arguments to stdout or query output */ static backslashResult exec_command_echo(PsqlScanState scan_state, bool active_branch, const char *cmd) @@ -1129,6 +1129,8 @@ exec_command_echo(PsqlScanState scan_state, bool active_branch, const char *cmd) if (strcmp(cmd, "qecho") == 0) fout = pset.queryFout; + else if (strcmp(cmd, "warn") == 0) + fout = stderr; else fout = stdout; diff --git a/src/bin/psql/help.c b/src/bin/psql/help.c index d6d41b51d5..434388608a
Re: generate documentation keywords table automatically
Peter Eisentraut writes: > The SQL keywords table in the documentation had until now been generated > by me every year by some ad hoc scripting outside the source tree once > for each major release. This patch changes it to an automated process. Didn't test this, but +1 for the concept. Would it make more sense to have just one source file per SQL standard version, and distinguish the keyword types by labels within the file? The extreme version of that would be to format the standards-info files just like parser/kwlist.h, which perhaps would even save a bit of parsing code in the Perl script. I don't insist you have to go that far, but lists of keywords-and-categories seem to make sense. The thing in the back of my mind here is that at some point the SQL standard might have more than two keyword categories. What you've got here would take some effort to handle that, whereas it'd be an entirely trivial data change in the scheme I'm thinking of. A policy issue, independent of this mechanism, is how many different SQL spec versions we want to show in the table. HEAD currently shows just three (2011, 2008, SQL92), and it doesn't look to me like the table can accommodate more than one or at most two more columns without getting too wide for most output formats. We could buy back some space by making the "cannot be X" annotations for PG keywords more compact, but I fear that'd still not be enough for the seven spec versions you propose to show in this patch. (And, presumably, the committee's not done.) Can we pick a different table layout? regards, tom lane
Re: clean up docs for v12
Michael Paquier writes: > On Fri, Apr 26, 2019 at 09:56:47PM -0500, Justin Pryzby wrote: >> -all autovacuum actions. Minus-one (the default) disables logging >> +all autovacuum actions. -1 (the default) >> disables logging >> >> There's nothing else that says "minus-one" anywhere else on that page. I >> just >> found one in auto-explain.sgml, which I changed. > That's one of these I am not sure about. FWIW, I think we generally write this the way Justin suggests. It's more precise, at least if you're reading it in a way that makes text distinguishable from plain text: what to put into the config file is exactly "-1", and not for instance "minus-one". regards, tom lane
Re: BUG #15781: subselect on foreign table (postgres_fdw) can crash (segfault)
Etsuro Fujita writes: > On Sat, Apr 27, 2019 at 2:10 AM Tom Lane wrote: >> If we don't want to change what the core code does with fdw_exprs, >> I think the only way to fix it is to hack postgres_fdw so that it >> won't generate plans involving the problematic case. > Seems reasonable. >> See attached. > I read the patch. It looks good to me. I didn't test it, though. Thanks for looking! Have a good vacation ... regards, tom lane
Re: [PATCH v1] Add \echo_stderr to psql
Hello David,
Please find attached v2, name is now \warn.
Patch applies cleanly, compiles, "make check ok", although there are no
tests. Doc gen ok.
Code is pretty straightforward.
I'd put the commands in alphabetical order (echo, qecho, warn) instead of
e/w/q in the condition.
The -n trick does not appear in the help lines, ISTM that it could fit, so
maybe it could be added, possibly something like:
\echo [-n] [TEXT] write string to stdout, possibly without trailing newline
and same for \warn and \qecho?
How might we test this portably?
Hmmm... TAP tests are expected to be portable. Attached a simple POC,
which could be extended to test many more things which are currently out
of coverage (src/bin/psql stuff is covered around 40% only).
--
Fabien.diff --git a/src/bin/psql/.gitignore b/src/bin/psql/.gitignore
index c2862b12d6..d324c1c1fa 100644
--- a/src/bin/psql/.gitignore
+++ b/src/bin/psql/.gitignore
@@ -3,3 +3,4 @@
/sql_help.c
/psql
+/tmp_check
diff --git a/src/bin/psql/Makefile b/src/bin/psql/Makefile
index 69bb297fe7..9473ab01cb 100644
--- a/src/bin/psql/Makefile
+++ b/src/bin/psql/Makefile
@@ -60,8 +60,15 @@ uninstall:
clean distclean:
rm -f psql$(X) $(OBJS) lex.backup
+ rm -rf tmp_check
# files removed here are supposed to be in the distribution tarball,
# so do not clean them in the clean/distclean rules
maintainer-clean: distclean
rm -f sql_help.h sql_help.c psqlscanslash.c
+
+check:
+ $(prove_check)
+
+installcheck:
+ $(prove_installcheck)
diff --git a/src/bin/psql/t/001_psql.pl b/src/bin/psql/t/001_psql.pl
new file mode 100644
index 00..32dd43279b
--- /dev/null
+++ b/src/bin/psql/t/001_psql.pl
@@ -0,0 +1,32 @@
+use strict;
+use warnings;
+
+use PostgresNode;
+use TestLib;
+use Test::More;
+
+my $node = get_new_node('main');
+$node->init();
+$node->start();
+
+# invoke psql
+# - opts: space-separated options and arguments
+# - stat: expected exit status
+# - in: input stream
+# - out: list of re to check on stdout
+# - err: list of re to check on stderr
+# - name: of the test
+sub psql
+{
+ local $Test::Builder::Level = $Test::Builder::Level + 1;
+ my ($opts, $stat, $in, $out, $err, $name) = @_;
+ my @cmd = ('psql', split /\s+/, $opts);
+ $node->command_checks_all(\@cmd, $stat, $out, $err, $name, $in);
+ return;
+}
+
+psql('-c \\q', 0, '', [ qr{^$} ], [ qr{^$} ], 'psql -c');
+psql('', 0,"\\echo hello\n\\warn world\n\\q\n", [ qr{^hello$} ], [ qr{^world$} ], 'psql in/out/err');
+
+$node->stop();
+done_testing();
diff --git a/src/test/perl/TestLib.pm b/src/test/perl/TestLib.pm
index a164cdbd8c..6ad7f681ae 100644
--- a/src/test/perl/TestLib.pm
+++ b/src/test/perl/TestLib.pm
@@ -519,22 +519,24 @@ sub command_fails_like
}
# Run a command and check its status and outputs.
-# The 5 arguments are:
+# The 5 to 6 arguments are:
# - cmd: ref to list for command, options and arguments to run
# - ret: expected exit status
# - out: ref to list of re to be checked against stdout (all must match)
# - err: ref to list of re to be checked against stderr (all must match)
# - test_name: name of test
+# - in: standard input
sub command_checks_all
{
local $Test::Builder::Level = $Test::Builder::Level + 1;
- my ($cmd, $expected_ret, $out, $err, $test_name) = @_;
+ my ($cmd, $expected_ret, $out, $err, $test_name, $in) = @_;
+ $in = '' if not defined $in;
# run command
my ($stdout, $stderr);
print("# Running: " . join(" ", @{$cmd}) . "\n");
- IPC::Run::run($cmd, '>', \$stdout, '2>', \$stderr);
+ IPC::Run::run($cmd, '<', \$in, '>', \$stdout, '2>', \$stderr);
# See http://perldoc.perl.org/perlvar.html#%24CHILD_ERROR
my $ret = $?;
Re: BUG #15781: subselect on foreign table (postgres_fdw) can crash (segfault)
On Sat, Apr 27, 2019 at 2:10 AM Tom Lane wrote: > > (2019/04/26 3:24), Tom Lane wrote: > >> If we do leave it like this, then the only way for postgres_fdw to > >> avoid trouble is to not have any entries in fdw_exprs that exactly > >> match entries in fdw_scan_tlist. So that pretty much devolves back > >> to what I said before: don't ship values to the far end that are > >> just going to be fed back as-is. But now it's a correctness > >> requirement not just an optimization. > Well, the releases are coming up fast, so I spent some time on this. > If we don't want to change what the core code does with fdw_exprs, > I think the only way to fix it is to hack postgres_fdw so that it > won't generate plans involving the problematic case. Seems reasonable. > See attached. I read the patch. It looks good to me. I didn't test it, though. > We end up with slightly weird-looking plans if the troublesome Param > is actually a GROUP BY expression, but if it's not, I think things > are fine. Maybe we could do something smarter about the GROUP BY case, > but it seems weird enough to maybe not be worth additional trouble. Agreed. Thanks for working on this! Best regards, Etsuro Fujita
Re: Identity columns should own only one sequence
On 2019-04-26 15:37, Laurenz Albe wrote:
> What do you think of the patch I just posted on this thread to
> remove ownership automatically when the default is dropped, as Michael
> suggested? I think that would make things much more intuitive from
> the user's perspective.
I think that adds more nonstandard behavior on top of an already
confusing and obsolescent feature, so I can't get too excited about it.
A more forward-looking fix would be your other idea of having
getOwnedSequence() only deal with identity sequences (not serial
sequences). See attached patch for a draft.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From b0a9c3402847f846d6d133cb9eced56ea9634a30 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut
Date: Sat, 27 Apr 2019 14:12:03 +0200
Subject: [PATCH] Untangle some stuff about identity sequences
XXX draft XXX
---
src/backend/catalog/pg_depend.c | 26 +++---
src/backend/commands/tablecmds.c | 2 +-
src/backend/parser/parse_utilcmd.c | 12 +---
src/backend/rewrite/rewriteHandler.c | 2 +-
src/include/catalog/dependency.h | 2 +-
5 files changed, 27 insertions(+), 17 deletions(-)
diff --git a/src/backend/catalog/pg_depend.c b/src/backend/catalog/pg_depend.c
index f7caedcc02..63c94cfbe6 100644
--- a/src/backend/catalog/pg_depend.c
+++ b/src/backend/catalog/pg_depend.c
@@ -707,8 +707,8 @@ sequenceIsOwned(Oid seqId, char deptype, Oid *tableId,
int32 *colId)
* Collect a list of OIDs of all sequences owned by the specified relation,
* and column if specified.
*/
-List *
-getOwnedSequences(Oid relid, AttrNumber attnum)
+static List *
+getOwnedSequences_internal(Oid relid, AttrNumber attnum, char deptype)
{
List *result = NIL;
RelationdepRel;
@@ -750,7 +750,8 @@ getOwnedSequences(Oid relid, AttrNumber attnum)
(deprec->deptype == DEPENDENCY_AUTO || deprec->deptype
== DEPENDENCY_INTERNAL) &&
get_rel_relkind(deprec->objid) == RELKIND_SEQUENCE)
{
- result = lappend_oid(result, deprec->objid);
+ if (!deptype || deprec->deptype == deptype)
+ result = lappend_oid(result, deprec->objid);
}
}
@@ -761,18 +762,29 @@ getOwnedSequences(Oid relid, AttrNumber attnum)
return result;
}
+List *
+getOwnedSequences(Oid relid, AttrNumber attnum)
+{
+ return getOwnedSequences_internal(relid, attnum, 0);
+}
+
/*
- * Get owned sequence, error if not exactly one.
+ * Get owned identity sequence, error if not exactly one.
*/
Oid
-getOwnedSequence(Oid relid, AttrNumber attnum)
+getIdentitySequence(Oid relid, AttrNumber attnum, bool missing_ok)
{
- List *seqlist = getOwnedSequences(relid, attnum);
+ List *seqlist = getOwnedSequences_internal(relid, attnum,
DEPENDENCY_INTERNAL);
if (list_length(seqlist) > 1)
elog(ERROR, "more than one owned sequence found");
else if (list_length(seqlist) < 1)
- elog(ERROR, "no owned sequence found");
+ {
+ if (missing_ok)
+ return InvalidOid;
+ else
+ elog(ERROR, "no owned sequence found");
+ }
return linitial_oid(seqlist);
}
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 14fcad9034..53dc72f7d6 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -6607,7 +6607,7 @@ ATExecDropIdentity(Relation rel, const char *colName,
bool missing_ok, LOCKMODE
table_close(attrelation, RowExclusiveLock);
/* drop the internal sequence */
- seqid = getOwnedSequence(RelationGetRelid(rel), attnum);
+ seqid = getIdentitySequence(RelationGetRelid(rel), attnum, false);
deleteDependencyRecordsForClass(RelationRelationId, seqid,
RelationRelationId, DEPENDENCY_INTERNAL);
CommandCounterIncrement();
diff --git a/src/backend/parser/parse_utilcmd.c
b/src/backend/parser/parse_utilcmd.c
index 4564c0ae81..a3c5b005d5 100644
--- a/src/backend/parser/parse_utilcmd.c
+++ b/src/backend/parser/parse_utilcmd.c
@@ -1083,7 +1083,7 @@ transformTableLikeClause(CreateStmtContext *cxt,
TableLikeClause *table_like_cla
* find sequence owned by old column; extract sequence
parameters;
* build new create sequence command
*/
- seq_relid =
getOwnedSequence(RelationGetRelid(relation), attribute->attnum);
+ seq_relid =
getIdentitySequence(RelationGetRelid(relation), attribute->attnum, false);
seq_options = sequence_options(seq_relid);
generateSerialExtraStmts(cxt,
Re: Optimizer items in the release notes
On Sat, Apr 27, 2019 at 02:47:44PM +1200, David Rowley wrote: > On Sat, 27 Apr 2019 at 14:22, Bruce Momjian wrote: > > > > * They are hard to explain > > > > > > That can be true, but we generally get there if not the first time > > > then after a few iterations. Authors and committers of the > > > improvements are likely to be able to help find suitable wording. > > > > It is not the text that is hard, but hard to explain the concept in a > > way that relates to anything a normal user is familiar with. > > Yeah, that's no doubt often going to be a struggle, but we can't > expect every person who reads the release notes to understand > everything. You could probably say the same for any internal > implementation change we make though. I don't think the planner is > unique in this regard, so I don't think it needs any special I do believe the planner is unique in this regard in the sense that the changes can make 100x difference in performance, and it is often unclear from the user interface exactly what is happening. > treatment. I also don't think we need to go into great detail. The > item could be as light on detail as: > > * Improve query planner's ability to push LIMIT through Sort nodes. > > If you don't know what LIMIT is or what a Sort node is, then you're > probably not going to care about the change. They can keep reading on > the next line, but if the reader happens to have suffered some pain on > that during their migration attempt, then they might be quite happy to > see those words. If they want more details then they might be savvy > enough to hunt those down, or perhaps they'll come asking. Uh, that is not clear to me so I am not sure the average user would know about it. I would probably try to explain that one in a way that can be understood, like LIMIT in subqueries affecting the outer query sort performance. -- Bruce Momjian http://momjian.us EnterpriseDB http://enterprisedb.com + As you are, so once was I. As I am, so you will be. + + Ancient Roman grave inscription +
nRe: [PATCH v1] Show whether tables are logged in \dt+
Hello David, Patch v3 applies, but compiles for me with a warning because the indentation of the following size block has been changed: describe.c: In function ‘listTables’: describe.c:3705:7: warning: this ‘if’ clause does not guard... [-Wmisleading-indentation] else if (pset.sversion >= 80100) ^~ describe.c:3710:3: note: ...this statement, but the latter is misleadingly indented as if it were guarded by the ‘if’ appendPQExpBuffer(, ^ Make check fails because of my temp schema was numbered 4 instead of 3, and I'm "fabien" rather than "shackle". Included, but they're not stable for temp tables. I'm a little stumped as to how to either stabilize them or test some other way. Hmmm. First there is the username which appears, so there should be a dedicated user for the test. I'm unsure how to work around the temporary schema number, which is undeterministic with parallel execution it. I'm afraid the only viable approach is not to show temporary tables, too bad:-( The tests have not been fixed. I think that they need a dedicated user to replace "shackle", and I'm afraid that there temporary test schema instability cannot be easily fixed at the "psql" level, but would require some kind of TAP tests instead if it is to be checked. In the short term, do not. I checked that the \di+ works, though. I've played with temporary views and \dv as well. I discovered that you cannot have temporary unlogged objects, nor temporary or unlogged materialized views. Intuitively I'd have thought that these features would be orthogonal, but they are not. Also I created an unlogged table with a SERIAL which created a sequence. The table is unlogged but the sequence is permanent, which is probably ok. I only have packages down to pg 9.3, so I could not test prior 9.1. By looking at the online documentation, is seems that relistemp appears in pg 8.4, so the corresponding extraction should be guarded by this version. Before that, temporary objects existed but were identified indirectly, possibly because they were stored in a temporary schema. I suggest not to try to address cases prior 8.4. -- Fabien.
