Re: Race conditions with checkpointer and shutdown
On Sun, Apr 28, 2019 at 12:56 PM Tom Lane wrote: > 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. +1 I see that we removed the code that this was modelled on back in 2015, and in fact your patch even removes a dangling reference in a comment: - * This is very much like what regular backends do with ImmediateInterruptOK, > 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. Right. > 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. +1 That reminds me, we should probably also clean up at least the ereport-from-signal-handler hazard identified over in this thread: https://www.postgresql.org/message-id/CAEepm%3D10MtmKeDc1WxBM0PQM9OgtNy%2BRCeWqz40pZRRS3PNo5Q%40mail.gmail.com -- Thomas Munro https://enterprisedb.com
Re: [PATCH v5] Show detailed table persistence in \dt+
On Sun, Apr 28, 2019 at 07:26:55PM +0200, Fabien COELHO wrote:
>
> Hello David,
>
> > > Patch applies. There seems to be a compilation issue:
> > >
> > > describe.c:5974:1: error: expected declaration or statement at end of
> > > input
> > > }
> >
> > This is in brown paper bag territory. Fixed.
>
> I do not understand why you move both size and description out of the
> verbose mode, it should be there only when under verbose?
My mistake. Fixed.
> > I've sent a separate patch extracted from the one you sent which adds
> > stdin to our TAP testing infrastructure. I hope it lands so it'll be
> > simpler to add these tests in a future version of the patch.
>
> Why not. As I'm the one who wrote the modified function, probably I could
> have thought of providing an input. I'm not sure it is worth a dedicated
> submission, could go together with any commit that would use it.
My hope is that this is seen as a bug fix and gets back-patched.
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 ddc5dddccb275bffbb06d9bc2a5e60eced84a151 Mon Sep 17 00:00:00 2001
From: David Fetter
Date: Mon, 22 Apr 2019 17:50:48 -0700
Subject: [PATCH v6] 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..46b1fad52d 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))
@@ -3679,6 +3679,22 @@ listTables(const char *tabtypes, const char *pattern, bool verbose, bool showSys
if (verbose)
{
+ /*
+ * Show whether a table is permanent, temporary, or unlogged.
+ * Indexes are not, as of this writing, tables.
+ */
+ if (!showIndexes)
+ {
+ if (pset.sversion >= 90100)
+appendPQExpBuffer(,
+ ",\n CASE c.relpersistence WHEN 'p' THEN 'permanent' WHEN 't' THEN 'temporary' WHEN 'u' THEN 'unlogged' ELSE 'unknown' END as \"%s\"",
+ gettext_noop("Persistence"));
+ else if (pset.sversion >= 80400)
+appendPQExpBuffer(,
+ ",\n CASE WHEN c.relistemp THEN 'temporary' ELSE 'permanent' END as \"%s\"",
+ gettext_noop("Persistence"));
+ }
+
/*
* As of PostgreSQL 9.0, use pg_table_size() to show a more accurate
* size of a table, including FSM, VM and TOAST tables.
--2.20.1--
Re: [PATCH v4] Add \warn to psql
On Sun, Apr 28, 2019 at 08:22:09PM +0200, Fabien COELHO wrote:
>
> Hello David,
>
> About v4: applies, compiles, global & local "make check" ok. Doc gen ok.
>
> Code & help look ok.
>
> About the doc: I do not understand why the small program listing contains an
> "\echo :variable".
It no longer does.
> Also, the new entry should probably be between the \w &
> \watch entries instead of between \echo & \ef.
Moved.
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 ede29ec28e833c2c86638bbc08f3400fe6f7f3f9 Mon Sep 17 00:00:00 2001
From: David Fetter
Date: Sun, 21 Apr 2019 11:08:40 -0700
Subject: [PATCH v5] 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="v5-0001-Add-warn-to-psql.patch"
Content-Transfer-Encoding: 8bit
Content-Disposition: attachment; filename="v5-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..e474efb521 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -3220,6 +3220,25 @@ testdb= \setenv LESS -imx4F
+
+\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:
+
+
+= \warn `date`
+Sun Apr 28 21:00:10 PDT 2019
+
+If the first argument is an unquoted -n the trailing
+newline is not written.
+
+
+
+
\watch [ seconds ]
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..f6fedb45b7 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, "qecho") == 0 || strcmp(cmd, "warn") == 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, \qecho, and \warn -- echo arguments to stdout, query output, or stderr
*/
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..bc98f01be7 100644
--- a/src/bin/psql/help.c
+++ b/src/bin/psql/help.c
@@ -206,11 +206,12 @@ slashUsage(unsigned short int pager)
fprintf(output, _("Input/Output\n"));
fprintf(output, _(" \\copy ... perform SQL COPY with data stream to the client host\n"));
- fprintf(output, _(" \\echo [STRING] write string to standard output\n"));
+ fprintf(output, _(" \\echo [-n] [STRING]write string to standard output (-n for no newline)\n"));
fprintf(output, _(" \\i FILEexecute commands from file\n"));
fprintf(output, _(" \\ir FILE as \\i, but relative to location of current script\n"));
+ fprintf(output, _(" \\qecho [-n] [STRING]
Re: BUG #15708: RLS 'using' running as wrong user when called from a view
On Wed, 27 Mar 2019 at 23:46, Dean Rasheed wrote: > On second thoughts, it actually needs to be in > get_row_security_policies(), after making copies of the quals from the > policies, otherwise it would be scribbling on the copies from the > relcache. Actually that makes the code change a bit simpler too. Thanks for writing the patch! I'm sad this missed the last commit fest; I think this bug might be causing security issues in a few deployments. Could you submit the patch for the next commit fest?
Re: [PATCH v5] Show detailed table persistence in \dt+
On Sun, Apr 28, 2019 at 01:14:01PM -0400, Tom Lane wrote: > Not particularly on topic, but: including a patch version number in your > subject headings is pretty unfriendly IMO, because it breaks threading > for people whose MUAs do threading by matching up subject lines. Thanks for letting me know about those MUAs. > I don't actually see the point of the [PATCH] annotation at all, > because the thread is soon going to contain lots of messages with > the same subject line but no embedded patch. Like this one. So > it's just noise with no information content worth noticing. OK 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
Do you see any problems with this procedure for Old Master rebuild as a Slave upon switchover ?
Hi Team, Let us say we have a Master (M1) and a Slave (S1) in replication using Streaming Replication. I stopped all my writes from Application and i switched a WAL and made sure it is replicated to Slave. I have then shutdown M1. And ran a promote on S1. Now S1 is my new Master with a new timeline. Now, in order to let M1 replicate changes from S1 (Master) as a Slave, i am able to succeed with the following approach. Add recovery_target_timeline = 'latest' and then have the appropriate entries such as primary_conninfo, standby_mode in the recovery.conf and start the M1 using pg_ctl. I see that it M1 (Old Master) is able to catch up with S1 (New Master). And replication is going fine. Have you ever faced or think of a problem with this approach ? Points to note are : 1. Master was neatly SHUTDOWN after shutting down writes. So, it has not diverged. (If it is diverged, i would of course need a pg_rewind like approach). 2. It was a planned switchover. During this entire process, there are no writes to M1 (before Switchover) or S1 (after promote). 3. timeline history file is also accessible to the Old Master (M1) after S1 was promoted. No transactions, so no WALs generated, may be 1 or 2 considering timeout, etc. It looks like a clean approach, but do you think there could be a problem with this approach of rebuilding Old Master as a Slave ? Is this approach still okay ? Thanks, Avinash Vallarapu.
Re: Speed up build on Windows by generating symbol definition in batch
On Wed, Apr 10, 2019 at 02:27:26PM +0800, Peifeng Qiu wrote: > I've updated the patch according to your comments. Looks good. Thanks. I plan to push this on Saturday.
Re: "long" type is not appropriate for counting tuples
On Sun, Apr 28, 2019 at 4:25 PM Tom Lane wrote: > > ISTM that we should try to come up with a way of making code like this > > work, rather than placing the burden on new code to get it right. > > Other than "use the right datatype", I'm not sure what we can do? Ambiguity seems like the real problem here. If we could impose a general rule that you cannot use "long" (perhaps with some limited wiggle-room), then a lint-like tool could catch bugs like this. This may not be that difficult. Nobody is particularly concerned about performance on 32-bit platforms these days. > In the meantime, somebody should fix ab0dfc961b6 ... I'll leave this to Alvaro. > Hmm, why is this a problem? We should only use off_t for actual file > access operations, and we don't use files greater than 1GB. (There's a > reason for that.) The issue that was fixed by commit aa551830 showed this assumption to be kind of brittle. Admittedly this is not as clear-cut as the "long" issue, and might not be worth worrying about. I don't want to go as far as requiring explicit width integer types in all situations, since that seems totally impractical, and without any real upside. But it would be nice to identify integer types where there is a real risk of making an incorrect assumption, and then eliminate that risk once and for all. -- Peter Geoghegan
Re: "long" type is not appropriate for counting tuples
Peter Geoghegan writes: > Commit ab0dfc961b6 used a "long" variable within _bt_load() to count > the number of tuples entered into a B-Tree index as it is built. This > will not work as expected on Windows, even on 64-bit Windows, because > "long" is only 32-bits wide. Right. "long" used to be our convention years ago, but these days tuple counts should be int64 or perhaps uint64. See e.g. 23a27b039. > ISTM that we should try to come up with a way of making code like this > work, rather than placing the burden on new code to get it right. Other than "use the right datatype", I'm not sure what we can do? In the meantime, somebody should fix ab0dfc961b6 ... > Also, "off_t" is only 32-bits on Windows, which broke parallel CREATE > INDEX (issued fixed by commit aa551830). I suppose that "off_t" is > really a variant of the same problem. Hmm, why is this a problem? We should only use off_t for actual file access operations, and we don't use files greater than 1GB. (There's a reason for that.) regards, tom lane
"long" type is not appropriate for counting tuples
Commit ab0dfc961b6 used a "long" variable within _bt_load() to count the number of tuples entered into a B-Tree index as it is built. This will not work as expected on Windows, even on 64-bit Windows, because "long" is only 32-bits wide. It's far from impossible that you'd have ~2 billion index tuples when building a new index. Programmers use "long" because it is assumed to be wider than "int" (even though that isn't required by the standard, and isn't true across all of the platforms we support). The use of "long" seems inherently suspect given our constraints, except perhaps in the context of sizing work_mem-based allocations, where it is used as part of a semi-standard idiom...albeit one that only works because of the restrictions on work_mem size on Windows. ISTM that we should try to come up with a way of making code like this work, rather than placing the burden on new code to get it right. This exact issue has bitten users on a number of occasions that I can recall. There is also a hidden landmine that we know about but haven't fixed: logtape.c, which will break on Windows with very very large index builds. Also, "off_t" is only 32-bits on Windows, which broke parallel CREATE INDEX (issued fixed by commit aa551830). I suppose that "off_t" is really a variant of the same problem. -- Peter Geoghegan
Re: Data streaming between different databases
On Sun, Apr 28, 2019 at 03:59:15PM +0200, Sascha Kuhl wrote: Is it possible to differentialy synchronise two databases on the basis of equality and differences between both? Can I review this piece of code? It's rather unclear what exactly are you looking for, what do you mean by 'on the basis of equality and differences' and how it all should work. Maybe try explaining it in more detail, with some examples or something. Or if there are other products doing something like that, share a link. regards -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: [PATCH v5] Show detailed table persistence 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 } This is in brown paper bag territory. Fixed. I do not understand why you move both size and description out of the verbose mode, it should be there only when under verbose? I've sent a separate patch extracted from the one you sent which adds stdin to our TAP testing infrastructure. I hope it lands so it'll be simpler to add these tests in a future version of the patch. Why not. As I'm the one who wrote the modified function, probably I could have thought of providing an input. I'm not sure it is worth a dedicated submission, could go together with any commit that would use it. -- Fabien.
Re: jsonpath
On Thu, Apr 25, 2019 at 10:29 PM Tom Lane wrote:
> Alexander Korotkov writes:
> > I'm going to commit these adjustments if no objections.
>
> Sorry for not getting to this sooner. Looking quickly at the v2 patch,
> it seems like you didn't entirely take to heart the idea of preferring
> a useful primary error message over a boilerplate primary with errdetail.
> In particular, in places like
>
> - errmsg(ERRMSG_SINGLETON_JSON_ITEM_REQUIRED),
> - errdetail("expression should return a singleton boolean")));
> + errmsg("singleton SQL/JSON item required"),
> + errdetail("Singleton boolean result is expected.")));
>
> why bother with errdetail at all? It's not conveying any useful increment
> of information. In this example I think
>
> errmsg("expression should return a singleton boolean")
>
> is sufficient and well-phrased. Likewise, a bit further down,
>
> + errmsg("SQL/JSON member not found"),
> + errdetail("JSON object does not contain key
> \"%s\".",
>
> there is nothing being said here that wouldn't fit perfectly well into
> one errmsg.
Makes sense. Attached revision of patch gets rid of errdetail() where
it seems to be appropriate.
> > My question regarding jsonpath_yyerror() vs. bison errors is still
> > relevant. Should we bother making bison-based errdetail() a complete
> > sentence starting from uppercase character? If not, should we make
> > other yyerror() calls look the same? Or should we rather move bison
> > error from errdetail() to errmsg()?
>
> The latter I think. The core lexer just presents the yyerror message
> as primary:
>
> scanner_yyerror(const char *message, core_yyscan_t yyscanner)
> {
> ...
> ereport(ERROR,
> (errcode(ERRCODE_SYNTAX_ERROR),
> /* translator: %s is typically the translation of "syntax error" */
> errmsg("%s at end of input", _(message)),
> lexer_errposition()));
>
> and in a quick look at what jsonpath is doing, I'm not really seeing
> the point of it being different. You could do something like
>
> /* translator: %s is typically the translation of "syntax error" */
> errmsg("%s in jsonpath input", _(message))
>
> to preserve the information that this is about jsonpath, but beyond
> that I don't see that splitting off an errdetail is helping much.
I've moved error message into errmsg().
> Or, perhaps, provide an errdetail giving the full jsonpath input string?
> That might or might not be redundant with other context information,
> so I'm not sure how useful it'd be.
I'm also not sure about this. Didn't do this for now.
--
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
jsonpath-errors-improve-3.patch
Description: Binary data
Re: [PATCH v4] Add \warn to psql
Hello David, About v4: applies, compiles, global & local "make check" ok. Doc gen ok. Code & help look ok. About the doc: I do not understand why the small program listing contains an "\echo :variable". Also, the new entry should probably be between the \w & \watch entries instead of between \echo & \ef. -- Fabien.
Re: Improve search for missing parent downlinks in amcheck
On Sun, Apr 28, 2019 at 4:36 AM Peter Geoghegan wrote: > 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? I think this definitely not bug fix. Bloom filter was designed to be lossy, no way blaming it for that :) > 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. Sounds reasonable. I'll think about proposing backpatch of something like this. -- Alexander Korotkov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
Re: [PATCH v5] Show detailed table persistence in \dt+
Not particularly on topic, but: including a patch version number in your subject headings is pretty unfriendly IMO, because it breaks threading for people whose MUAs do threading by matching up subject lines. I don't actually see the point of the [PATCH] annotation at all, because the thread is soon going to contain lots of messages with the same subject line but no embedded patch. Like this one. So it's just noise with no information content worth noticing. regards, tom lane
Re: [HACKERS] Weaker shmem interlock w/o postmaster.pid
On Fri, Mar 29, 2019 at 09:53:51AM +, Daniel Gustafsson wrote: > On Saturday, March 9, 2019 8:16 AM, Noah Misch wrote: > > I tested on Red Hat and on Windows Server 2016; I won't be shocked > > if the test (not the code under test) breaks on other Windows > > configurations. > > IIRC there are Windows versions where Win32::Process::KillProcess is required > for this, but thats admittedly somewhat dated knowledge. If the buildfarm goes > red on older Windows animals it might be something to look at perhaps. Since my second attempt at committing this (commit c098509), I fixed these bugs in the new test file: - MSYS-orchestrated mingw32 (buildfarm member jacana) failed the Perl kill(9, ...) calls[1]. For HEAD and v11, using "pg_ctl kill KILL " fixed that. For v10 and v9.6, I disabled the test under msys. I can reproduce this with Perl 5.28 from msys2. Its kill(9, ...) fails for any non-msys2 process (any ordinary Windows process). [commits 947a350, 2bc0474] - The regress.dll path needed MSYS-to-w32 path translation. [commit 9daefff] - The changes to port selection in get_new_node() made Linux-specific assumptions, so Windows builds had much less protection against port conflict. [commit 4ab02e8] - Got EPIPE when writing to stdin of a child process that exited too quickly. [commit e12a472] Things have been stable on the buildfarm for the last twelve days, so I think this one is over. [1] https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=jacana=2019-04-13%2014%3A39%3A17
Re: pg_ssl
Will be doing in just a few days. I am taking _initial_ suggestions, incorporating them, then I will be setting that up. On 4/28/2019 11:25 AM, David Fetter wrote: On Sat, Apr 27, 2019 at 12:54:07PM -0400, Steve wrote: 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. Thanks for sending this along! Is there a public repo for this, in case people have patches they'd like to contribute? If not, would you be so kind as to make one? Best, David.
Data streaming between different databases
Is it possible to differentialy synchronise two databases on the basis of equality and differences between both? Can I review this piece of code?
Re: speeding up planning with partitions
On Sun, Apr 28, 2019 at 8:10 AM Tom Lane wrote:
> Amit Langote writes:
> > 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.
Just to clarify, I wasn't suggesting that we change query_planner(),
but the blocks in inheritance_planner() that perform initial planning
as if the query was SELECT and gather child target relations from that
planner run; the following consecutive blocks:
/*
* Before generating the real per-child-relation plans, do a cycle of
* planning as though the query were a SELECT.
...
*/
{
PlannerInfo *subroot;
and:
/*--
* Since only one rangetable can exist in the final plan, we need to make
* sure that it contains all the RTEs needed for any child plan.
...
child_appinfos = NIL;
old_child_rtis = NIL;
new_child_rtis = NIL;
parent_relids = bms_make_singleton(top_parentRTindex);
foreach(lc, select_appinfos)
{
AppendRelInfo *appinfo = lfirst_node(AppendRelInfo, lc);
RangeTblEntry *child_rte;
/* append_rel_list contains all append rels; ignore others */
if (!bms_is_member(appinfo->parent_relid, parent_relids))
continue;
/* remember relevant AppendRelInfos for use below */
child_appinfos = lappend(child_appinfos, appinfo);
I'm suggesting that we don't add the child relations that are dummy
due to constraint exclusion to child_appinfos. Maybe, we'll need to
combine the two blocks so that the latter can use the PlannerInfo
defined in the former to look up the child relation to check if dummy.
> > Do you want me to update my patch considering the above summary?
>
> Yes please.
I will try to get that done hopefully by tomorrow.
(On extended holidays that those of us who are in Japan have around
this time of year.)
> However, I wonder whether you're thinking differently in
> light of what you wrote in [1]:
Thanks for checking that thread.
> >>> 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.
Yeah, maybe we could away with that by telling users to define
equivalent CHECK constraints for corner cases like that although
that's not really great.
Thanks,
Amit
Re: pg_ssl
On Sat, Apr 27, 2019 at 12:54:07PM -0400, Steve wrote: > 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. Thanks for sending this along! Is there a public repo for this, in case people have patches they'd like to contribute? If not, would you be so kind as to make one? 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
[PATCH v5] Show detailed table persistence in \dt+
On Sat, Apr 27, 2019 at 10:38:50PM +0200, Fabien COELHO wrote:
>
> Hello David,
>
> Patch applies. There seems to be a compilation issue:
>
> describe.c:5974:1: error: expected declaration or statement at end of
> input
> }
This is in brown paper bag territory. Fixed.
> > I think the way forward is to test this with TAP rather than the
> > fixed-string method.
>
> Ok.
I've sent a separate patch extracted from the one you sent which adds
stdin to our TAP testing infrastructure. I hope it lands so it'll be
simpler to add these tests in a future version of the patch.
> > 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
Another brown paper bag, now fixed.
> 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…)
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 3baf2d98dd699b3b04c2d3ac038dedfc9369cef7 Mon Sep 17 00:00:00 2001
From: David Fetter
Date: Mon, 22 Apr 2019 17:50:48 -0700
Subject: [PATCH v5] 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..8b205f6e37 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,37 @@ 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"));
-
- appendPQExpBuffer(,
- ",\n pg_catalog.obj_description(c.oid, 'pg_class') as \"%s\"",
- gettext_noop("Description"));
+ if (!showIndexes)
+ {
+ if (pset.sversion >= 90100)
+appendPQExpBuffer(,
+ ",\n CASE c.relpersistence WHEN 'p' THEN 'permanent' WHEN 't' THEN 'temporary' WHEN 'u' THEN 'unlogged' ELSE 'unknown' END as \"%s\"",
+ gettext_noop("Persistence"));
+ else if (pset.sversion >= 80400)
+appendPQExpBuffer(,
+ ",\n CASE WHEN c.relistemp THEN 'temporary' ELSE 'permanent' END as \"%s\"",
+ gettext_noop("Persistence"));
+ }
}
+ /*
+ * As of PostgreSQL 9.0, use pg_table_size() to show a more accurate
+ * size of a table, including FSM, VM and TOAST 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"));
+
+ appendPQExpBuffer(,
+ ",\n pg_catalog.obj_description(c.oid, 'pg_class') as \"%s\"",
+ gettext_noop("Description"));
appendPQExpBufferStr(,
"\nFROM pg_catalog.pg_class c"
--2.20.1--
[PATCH v1] Add a way to supply stdin to TAP tests
Folks,
Our test coverage needs all the help it can get.
This patch, extracted from another by Fabian Coelho, helps move things
in that direction.
I'd like to argue that it's not a new feature, and that it should be
back-patched as far as possible.
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 08fe538383c7a3519f5bb8a0e379b023afc0bb9d Mon Sep 17 00:00:00 2001
From: David Fetter
Date: Sun, 28 Apr 2019 08:01:01 -0700
Subject: [PATCH v1] Add a way to supply stdin to TAP tests
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
by Fabian Coelho
This will help increase test coverage for anything that might need it,
and a lot of things currently need it.
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 = $?;
--2.20.1--
[PATCH v4] Add \warn to psql
On Sat, Apr 27, 2019 at 10:09:27PM +0200, Fabien COELHO wrote: > > 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) My mistake. I didn't think the order in which they were compared mattered much, but it makes sense on further reflection to keep things tidy in the code. > > > 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. Here's the alphabetical version. > > > > 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. Speaking of which, I'd like to see about getting your patch against Testlib.pm in so more tests of psql can also go in. It's not a new feature /per se/, and it doesn't break any current scripts, so I'd make the argument that it's OK for them to go in and possibly even be back-patched. 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 76ce9a85f92ec4f2283ee967b3067e3f3efda232 Mon Sep 17 00:00:00 2001 From: David Fetter Date: Sun, 21 Apr 2019 11:08:40 -0700 Subject: [PATCH v4] 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="v4-0001-Add-warn-to-psql.patch" Content-Transfer-Encoding: 8bit Content-Disposition: attachment; filename="v4-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..d4846ce729 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, "qecho") == 0 || strcmp(cmd, "warn") == 0) status = exec_command_echo(scan_state, active_branch, cmd); else if (strcmp(cmd, "elif") == 0) status =
Re: clean up docs for v12
On Sat, Apr 27, 2019 at 11:10:46AM -0400, Tom Lane wrote: > 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". Okay, sold. I have done and extra pass on v2-0002 and included again some obvious mistakes. I have noticed a couple of things on the way. -- Michael signature.asc Description: PGP signature
Re: standby recovery fails (tablespace related) (tentative patch and discussion)
On Wed, Apr 24, 2019 at 4:14 PM Kyotaro HORIGUCHI < horiguchi.kyot...@lab.ntt.co.jp> wrote: > Mmm. I posted to wrong thread. Sorry. > > At Tue, 23 Apr 2019 16:39:49 +0900 (Tokyo Standard Time), Kyotaro > HORIGUCHI wrote in < > 20190423.163949.36763221.horiguchi.kyot...@lab.ntt.co.jp> > > At Tue, 23 Apr 2019 13:31:58 +0800, Paul Guo wrote in > > > > Hi Kyotaro, ignoring the MakePGDirectory() failure will fix this > database > > > create redo error, but I suspect some other kind of redo, which > depends on > > > the files under the directory (they are not copied since the directory > is > > > not created) and also cannot be covered by the invalid page mechanism, > > > could fail. Thanks. > > > > If recovery starts from just after tablespace creation, that's > > simple. The Symlink to the removed tablespace is already removed > > in the case. Hence server innocently create files directly under > > pg_tblspc, not in the tablespace. Finally all files that were > > supposed to be created in the removed tablespace are removed > > later in recovery. > > > > If recovery starts from recalling page in a file that have been > > in the tablespace, XLogReadBufferExtended creates one (perhaps > > directly in pg_tblspc as described above) and the files are > > removed later in recoery the same way to above. This case doen't > > cause FATAL/PANIC during recovery even in master. > > > > XLogReadBufferExtended@xlogutils.c > > | * Create the target file if it doesn't already exist. This lets us > cope > > | * if the replay sequence contains writes to a relation that is later > > | * deleted. (The original coding of this routine would instead suppress > > | * the writes, but that seems like it risks losing valuable data if the > > | * filesystem loses an inode during a crash. Better to write the data > > | * until we are actually told to delete the file.) > > > > So buffered access cannot be a problem for the reason above. The > > remaining possible issue is non-buffered access to files in > > removed tablespaces. This is what I mentioned upthread: > > > > me> but I haven't checked this covers all places where need the same > > me> treatment. > > RM_DBASE_ID is fixed by the patch. > > XLOG/XACT/CLOG/MULTIXACT/RELMAP/STANDBY/COMMIT_TS/REPLORIGIN/LOGICALMSG: > - are not relevant. > > HEAP/HEAP2/BTREE/HASH/GIN/GIST/SEQ/SPGIST/BRIN/GENERIC: > - Resources works on buffer is not affected. > > SMGR: > - Both CREATE and TRUNCATE seems fine. > > TBLSPC: > - We don't nest tablespace directories. No Problem. > > I don't find a similar case. I took some time in digging into the related code. It seems that ignoring if the dst directory cannot be created directly should be fine since smgr redo code creates tablespace path finally by calling TablespaceCreateDbspace(). What's more, I found some more issues. 1) The below error message is actually misleading. 2019-04-17 14:52:14.951 CST [23030] FATAL: could not create directory "pg_tblspc/65546/PG_12_201904072/65547": No such file or directory 2019-04-17 14:52:14.951 CST [23030] CONTEXT: WAL redo at 0/3011650 for Database/CREATE: copy dir 1663/1 to 65546/65547 That should be due to dbase_desc(). It could be simply fixed following the code logic in GetDatabasePath(). 2) It seems that src directory could be missing then dbase_redo()->copydir() could error out. For example, \!rm -rf /tmp/tbspace1 \!mkdir /tmp/tbspace1 \!rm -rf /tmp/tbspace2 \!mkdir /tmp/tbspace2 create tablespace tbs1 location '/tmp/tbspace1'; create tablespace tbs2 location '/tmp/tbspace2'; create database db1 tablespace tbs1; alter database db1 set tablespace tbs2; drop tablespace tbs1; Let's say, the standby finishes all replay but redo lsn on pg_control is still the point at 'alter database', and then kill postgres, then in theory when startup, dbase_redo()->copydir() will ERROR since 'drop tablespace tbs1' has removed the directories (and symlink) of tbs1. Below simple code change could fix that. diff --git a/src/backend/commands/dbcommands.c b/src/backend/commands/dbcommands.c index 9707afabd9..7d755c759e 100644 --- a/src/backend/commands/dbcommands.c +++ b/src/backend/commands/dbcommands.c @@ -2114,6 +2114,15 @@ dbase_redo(XLogReaderState *record) */ FlushDatabaseBuffers(xlrec->src_db_id); + /* +* It is possible that the source directory is missing if +* we are re-replaying the xlog while subsequent xlogs +* drop the tablespace in previous replaying. For this +* we just skip. +*/ + if (!(stat(src_path, ) == 0 && S_ISDIR(st.st_mode))) + return; + /* * Copy this subdirectory to the new location * If we want to fix the issue by ignoring the dst path create failure, I do not think we should do that in copydir() since copydir() seems to be a common function. I'm not sure whether it is used by some extensions or not. If no maybe we should move the dst patch create logic out of copydir(). Also I'd
