Re: reindex concurrently and two toast indexes
On Tue, Feb 18, 2020 at 07:39:49AM +0100, Julien Rouhaud wrote:
> On Tue, Feb 18, 2020 at 7:19 AM Michael Paquier wrote:
> >
> > On Tue, Feb 18, 2020 at 07:06:25AM +0100, Julien Rouhaud wrote:
> > > On Tue, Feb 18, 2020 at 6:30 AM Michael Paquier
> > > wrote:
> > >> Hmm. There could be an argument here for skipping invalid toast
> > >> indexes within reindex_index(), because we are sure about having at
> > >> least one valid toast index at anytime, and these are not concerned
> > >> with CIC.
> > >
> > > Or even automatically drop any invalid index on toast relation in
> > > reindex_relation, since those can't be due to a failed CIC?
> >
> > No, I don't like much outsmarting REINDEX with more index drops than
> > it needs to do. And this would not take care of the case with REINDEX
> > INDEX done directly on a toast index.
>
> Well, we could still do both but I get the objection. Then skipping
> invalid toast indexes in reindex_relation looks like the best fix.
PFA a patch to fix the problem using this approach.
I also added isolation tester regression tests. The failure is simulated using
a pg_cancel_backend() on top of pg_stat_activity, using filters on a
specifically set application name and the query text to avoid any unwanted
interaction. I also added a 1s locking delay, to ensure that even slow/CCA
machines can consistently reproduce the failure. Maybe that's not enough, or
maybe testing this scenario is not worth the extra time.
>From 990d265e5d576b3b4133232f302d6207987f1511 Mon Sep 17 00:00:00 2001
From: Julien Rouhaud
Date: Fri, 21 Feb 2020 20:15:04 +0100
Subject: [PATCH] Don't reindex invalid indexes on TOAST tables.
Such indexes can only be duplicated leftovers of failed REINDEX CONCURRENTLY
commands. As we only allow to drop invalid indexes on TOAST tables, reindexing
those would lead to useless duplicated indexes that can't be dropped anymore.
Reported-by: Sergei Kornilov, Justin Pryzby
Author: Julien Rouhaud
Reviewed-by:
Discussion:
https://postgr.es/m/36712441546604286%40sas1-890ba5c2334a.qloud-c.yandex.net
Discussion: https://postgr.es/m/20200216190835.ga21...@telsasoft.com
Backpatch-through: 12
---
src/backend/catalog/index.c | 29 +++
.../expected/reindex-concurrently.out | 49 ++-
.../isolation/specs/reindex-concurrently.spec | 23 +
3 files changed, 100 insertions(+), 1 deletion(-)
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 8880586c37..201a3c39df 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -46,6 +46,7 @@
#include "catalog/pg_depend.h"
#include "catalog/pg_description.h"
#include "catalog/pg_inherits.h"
+#include "catalog/pg_namespace_d.h"
#include "catalog/pg_opclass.h"
#include "catalog/pg_operator.h"
#include "catalog/pg_tablespace.h"
@@ -3717,6 +3718,34 @@ reindex_relation(Oid relid, int flags, int options)
{
Oid indexOid = lfirst_oid(indexId);
+ /*
+* We skip any invalid index on a TOAST table. Those
can only be
+* a duplicate leftover of a failed REINDEX
CONCURRENTLY, and if we
+* rebuild it it won't be possible to drop it anymore.
+*/
+ if (rel->rd_rel->relnamespace == PG_TOAST_NAMESPACE)
+ {
+ HeapTuple tup;
+ boolskipit;
+
+ tup = SearchSysCache1(INDEXRELID,
ObjectIdGetDatum(indexOid));
+ if (!HeapTupleIsValid(tup))
+ elog(ERROR, "cache lookup failed for
index %u", indexOid);
+
+ skipit = ((Form_pg_index)
GETSTRUCT(tup))->indisvalid == false;
+
+ ReleaseSysCache(tup);
+
+ if (skipit)
+ {
+ ereport(NOTICE,
+(errmsg("skipping
invalid index \"%s.%s\"",
+
get_namespace_name(get_rel_namespace(indexOid)),
+
get_rel_name(indexOid;
+ continue;
+ }
+ }
+
reindex_index(indexOid, !(flags &
REINDEX_REL_CHECK_CONSTRAINTS),
persistence, options);
diff --git a/src/test/isolation/expected/reindex-concurrently.out
b/src/test/isolation/expected/reindex-concurrently.out
index 9e04169b2f..fa9039c125 100644
--- a/src/test/isolation/expected/reindex-concurrently.out
+++ b/src/test/isolation/expected/reindex-concurrently.out
@@ -1,4
Re: [HACKERS] Transactions involving multiple postgres foreign servers, take 2
On Tue, 18 Feb 2020 at 00:40, Muhammad Usama wrote: > > Hi Sawada San, > > I have a couple of comments on > "v27-0002-Support-atomic-commit-among-multiple-foreign-ser.patch" > > 1- As part of the XLogReadRecord refactoring commit the signature of > XLogReadRecord was changed, > so the function call to XLogReadRecord() needs a small adjustment. > > i.e. In function XlogReadFdwXactData(XLogRecPtr lsn, char **buf, int *len) > ... > - record = XLogReadRecord(xlogreader, lsn, ); > + XLogBeginRead(xlogreader, lsn) > + record = XLogReadRecord(xlogreader, ); > > 2- In register_fdwxact(..) function you are setting the > XACT_FLAGS_FDWNOPREPARE transaction flag > when the register request comes in for foreign server that does not support > two-phase commit regardless > of the value of 'bool modified' argument. And later in the > PreCommit_FdwXacts() you just error out when > "foreign_twophase_commit" is set to 'required' only by looking at the > XACT_FLAGS_FDWNOPREPARE flag. > which I think is not correct. > Since there is a possibility that the transaction might have only read from > the foreign servers (not capable of > handling transactions or two-phase commit) and all other servers where we > require to do atomic commit > are capable enough of doing so. > If I am not missing something obvious here, then IMHO the > XACT_FLAGS_FDWNOPREPARE flag should only > be set when the transaction management/two-phase functionality is not > available and "modified" argument is > true in register_fdwxact() > Thank you for reviewing this patch! Your comments are incorporated in the latest patch set I recently sent[1]. [1] https://www.postgresql.org/message-id/CA%2Bfd4k5ZcDvoiY_5c-mF1oDACS5nUWS7ppoiOwjCOnM%2BgrJO-Q%40mail.gmail.com Regards, -- Masahiko Sawadahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Make java client lib accept same connection strings as psql
On Fri, Feb 21, 2020 at 6:21 PM Michael Leonhard wrote: > How about making the Java client library accept the same connection > strings as psql and other command-line tools? That would make > PostgreSQL easier to use and increase its popularity. > That falls outside the scope of this list/project. The separate pgJDBC project team would need to decide to take that up. I also doubt both unsubstantiated claims you make - that it would make developers' lives materially easier and influence popularity measurably. David J.
Re: POC: rational number type (fractions)
Joe Nelson wrote: > where the denominator is made positive whenever possible (not possible > when it's -INT_MAX). (I meant INT_MIN rather than -INT_MAX.) Another more-than-one-way-to-do-it task is converting a float to a fraction. I translated John Kennedy's method [0] to C, but Github user adegert sent an alternative [1] that matches the way the CPython implementation works. 0: https://begriffs.com/pdf/dec2frac.pdf 1: https://github.com/begriffs/pg_rational/pull/13
Re: POC: rational number type (fractions)
Jeff Davis wrote: > The decision between an extension and a core type is a tricky one. To > put an extension in core, usually it's good to show either that it > satisfies something in the SQL standard, or that there is some > specific technical advantage (like integration with the syntax or the > type system). I don't see any references to "rational" in the SQL standard (fifth ed, 2016) and the only reference to "fraction" is for fractions of a second in datetime. Doesn't look like SQL includes rational numbers. Also I don't believe I'm getting extra abilities by putting this in core vs using an extension. Perhaps there's a syntax change that would make rationals more pleasant to deal with than how they in this patch, but I haven't imagined what it would be, and it's not backed by a standard. > Integrating it in core just to make it easier to use is a double-edge > sword. It does make it easier in some environments; but it also > removes pressure to make those environments offer better support for > the extension ecosystem, ultimately weakening extensions overall. Makes sense. We petitioned RDS to allow the pg_rational extension, [0] but they didn't respond. Not sure how that process is supposed to work. 0: https://github.com/begriffs/pg_rational/issues/7 > In this case I believe it could be a candidate for in-core, but it's > borderline. The reasons it makes sense to me are: > > 1. It seems that there's "only one way to do it". There may be more than one way to do it actually. For instance the choice between a fixed size type with limits on the fractions it can represent, vs one that can grow to hold any fraction. I chose the first option, to make the type the same size as float8. My reasoning was that there was would be no space overhead for choosing rational over float. Also there's the choice of whether to always store fractions in normal form (lowest terms). This patch allows fractions to be in non-normal form after arithmetic, and only normalizes as needed when an arithmetic operation would overflow. I wanted to cut down on how many times gcd is called. However this choice means that hash indices have to normalize because they hash the bit pattern, while btree indices can compare rationals without regard to normal form. This patch represents each rational as a separate numerator and denominator. I did some research today to see if there are any another ways, and looks like there's a technique from the 70s called "quote notation." [1] It appears that quote notation makes addition and subtraction faster, but that the operations can less predictable performance in the worst case scenarios with doing arbitrary precision. So there's more than one way to do it. 1: https://en.wikipedia.org/wiki/Quote_notation > 2. I don't expect this will be much of a maintenance burden. True, rational numbers aren't going to change anytime soon. > Keep in mind that if you do want this to be in core, the data format > has to be very stable to maintain pg_upgrade compatibility. The format is currently [int32 numerator][int32 denominator] packed together, where the denominator is made positive whenever possible (not possible when it's -INT_MAX). The quote notation alternative would arrange things very differently. > Patch comments: > > * Please include docs. Of course, if we determine the patch is desirable. The included tests should help demonstrate how it works for the purposes of review. > * I'm worried about the use of int32. Does that cover all of the > reasonable use cases of rational? I could imagine having two types, a rational8 for the current implementation, and an arbitrary precision rational. Perhaps... > * what's the philosophy regarding NULL and rational? Why are NULL > arguments returning non-NULL answers? The rational_intermediate(x, y) function picks a fraction between x and y, and NULL was meant as a signal that one of the sides is unbounded. So rational_intermediate(x, NULL) means find a fraction larger than x, and rational_intermediate(NULL, y) means find one smaller than y. The use case is a query for a spot "immediately after position 2:" SELECT rational_intermediate(2, min(pos)) FROM todos WHERE pos > 2; If there are already todos positioned after 2 then it'll find a spot between 2 and the min. However if there are no todos after 2 then min() will return NULL and we'll simply find a position *somewhere* after 2. > * Is rational_intermediate() well-defined, or can it just choose any > rational between the two arguments? It chooses the mediant [2] of x and y in lowest terms by walking a Stern-Brocot tree. I found that this keeps the terms of the fraction much smaller than taking the average of x and y. This was an advantage over calculating with floats because I don't know how to take the mediant of floats, and repeatedly taking the average of floats eats up precision pretty quickly. 2: https://en.wikipedia.org/wiki/Mediant_(mathematics) > * Can you discuss how
Make java client lib accept same connection strings as psql
Hi PostgreSQL Hackers, I've run into something confusing. The psql command accepts connection strings of the form: postgresql://user1:pass1@localhost:5432/db1?sslmode=require But passing this string to the java client library (with a "jdbc:" prefix) fails. See the exception and stack trace below. According to the docs https://jdbc.postgresql.org/documentation/80/connect.html , the java client library accepts connection strings with this form: postgresql://localhost:5432/db1?user=user1=pass1=true How about making the Java client library accept the same connection strings as psql and other command-line tools? That would make PostgreSQL easier to use and increase its popularity. Sincerely, Michael Exception in thread "main" org.postgresql.util.PSQLException: The connection attempt failed. at org.postgresql.core.v3.ConnectionFactoryImpl.openConnectionImpl(ConnectionFactoryImpl.java:292) at org.postgresql.core.ConnectionFactory.openConnection(ConnectionFactory.java:49) at org.postgresql.jdbc.PgConnection.(PgConnection.java:211) at org.postgresql.Driver.makeConnection(Driver.java:458) at org.postgresql.Driver.connect(Driver.java:260) at java.sql/java.sql.DriverManager.getConnection(DriverManager.java:677) at java.sql/java.sql.DriverManager.getConnection(DriverManager.java:228) at org.postgresql.ds.common.BaseDataSource.getConnection(BaseDataSource.java:98) at org.postgresql.ds.common.BaseDataSource.getConnection(BaseDataSource.java:83) at com.leonhardllc.x.db.temp.TemporaryDatabase.createTempDatabase(TemporaryDatabase.java:39) at com.leonhardllc.x.db.generated.JOOQSourceGenerator.main(JOOQSourceGenerator.java:35) Caused by: java.net.UnknownHostException: user1:pass1@localhost at java.base/java.net.AbstractPlainSocketImpl.connect(AbstractPlainSocketImpl.java:220) at java.base/java.net.SocksSocketImpl.connect(SocksSocketImpl.java:403) at java.base/java.net.Socket.connect(Socket.java:591) at org.postgresql.core.PGStream.(PGStream.java:75) at org.postgresql.core.v3.ConnectionFactoryImpl.tryConnect(ConnectionFactoryImpl.java:91) at org.postgresql.core.v3.ConnectionFactoryImpl.openConnectionImpl(ConnectionFactoryImpl.java:192) ... 10 more
Re: Parallel copy
On Fri, Feb 21, 2020 at 02:54:31PM +0200, Ants Aasma wrote: On Thu, 20 Feb 2020 at 18:43, David Fetter wrote:> On Thu, Feb 20, 2020 at 02:36:02PM +0100, Tomas Vondra wrote: > I think the wc2 is showing that maybe instead of parallelizing the > parsing, we might instead try using a different tokenizer/parser and > make the implementation more efficient instead of just throwing more > CPUs on it. That was what I had in mind. > I don't know if our code is similar to what wc does, maytbe parsing > csv is more complicated than what wc does. CSV parsing differs from wc in that there are more states in the state machine, but I don't see anything fundamentally different. The trouble with a state machine based approach is that the state transitions form a dependency chain, which means that at best the processing rate will be 4-5 cycles per byte (L1 latency to fetch the next state). I whipped together a quick prototype that uses SIMD and bitmap manipulations to do the equivalent of CopyReadLineText() in csv mode including quotes and escape handling, this runs at 0.25-0.5 cycles per byte. Interesting. How does that compare to what we currently have? regards -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: SPI Concurrency Precautions? Problems with Parallel Execution of Multiple CREATE TABLE statements
On 17/02/2020 21:24, Tom Mercha wrote:
Dear Hackers
I've been working on an extension and using SPI to execute some queries.
I am in a situation where I have the option to issue multiple queries
concurrently, ideally under same snapshot and transaction. In short, I
am achieving this by creating multiple dynamic background workers, each
one of them executing a query at the same time using
SPI_execute(sql_string, ...). To be more precise, sometimes I am also
opting to issue a 'CREATE TABLE AS ' command, an SPI utility
command.
I was however wondering whether I can indeed achieve concurrency in this
way. My initial results are not showing much difference compared to a
not concurrent implementation. If there would be a large lock somewhere
in SPI implementation obviously this can be counterintuitive. What would
be the precautions I would need to consider when working with SPI in
this manner?
Thanks,
Tom
Dear Hackers
I have run some tests to try and better highlight my issue as I am still
struggling a lot with it.
I have 4 'CREATE TABLE AS' statements of this nature: "CREATE TABLE
AS ". This means that I have different
table names for the same query.
I am spawning a number of dynamic background workers to execute each
statement. When I spawn 4 workers on a quad-core machine, the resutling
execution time per statement is {0.158s, 0.216s, 0.399s, 0.396s}.
However, when I have just one worker, the results are {0.151s, 0.141s,
0.146s, 0.136s}.
The way I am executing my statements is through SPI in each worker
(using a PG extension) as follows:
SPI_connect();
SPI_exec(queryString, 0);
SPI_finish();
In both test cases, SPI_connect/finish are executed 4 times.
What I expect is that with 4 workers, each statements will take approx
0.15s to execute since they are independent from each other. This would
result in approx a 4x speedup. Despite seeing concurrency, I am seeing
that each invdividual statement will take longer to execute. I am
struggling to understand this behavior, what this suggests to me is that
there is a lock somewhere which completely defeats my purpose.
I was wondering how I could execute my CREATE TABLE statements in a
parallel fashion given that they are independent from each other. If the
lock is the problem, what steps could I take to relax it? I would
greatly appreciate any guidance or insights on this topic.
Thanks,
Tom
Re: Do we ever edit release notes after release to add warnings about incompatibilities?
On Fri, Feb 21, 2020 at 10:15 PM Greg Stark wrote: > > So last year in 10.5, 9.6.10, 9.5.9, 9.4.19, and 9.3.24 we had a > binary ABI break that caused pglogical and other logical decoding > plugins to break: > > https://github.com/2ndQuadrant/pglogical/issues/183#issuecomment-417558313 > > This wasn't discovered until after the release so the release notes > don't highlight the risk. Someone upgrading past this release now > could diligently read all the release notes for all the versions they > care about and would never see anything warning that there was an > unintentional ABI break. > > I wonder if we shouldn't be adding a note to those release notes after > the fact (and subsequent "However if you are upgrading from a version > earliers than" notes in later releases). It would be quite a pain > I'm sure but I don't see any other way to get the information to > someone upgrading in the future. I suppose we could just add the note > to the current release notes on the theory that we only support > installing the current release. > I definitely think we should. People will be looking at the release notes for many years to come... And people will be installing the old versions. I think the right thing to do is to add them in the same place as they would've been added if we had noticed the thing at the right time. We shouldn't duplicate it across every notes since then, but it should be back-patched into those. //Magnus
Do we ever edit release notes after release to add warnings about incompatibilities?
So last year in 10.5, 9.6.10, 9.5.9, 9.4.19, and 9.3.24 we had a binary ABI break that caused pglogical and other logical decoding plugins to break: https://github.com/2ndQuadrant/pglogical/issues/183#issuecomment-417558313 This wasn't discovered until after the release so the release notes don't highlight the risk. Someone upgrading past this release now could diligently read all the release notes for all the versions they care about and would never see anything warning that there was an unintentional ABI break. I wonder if we shouldn't be adding a note to those release notes after the fact (and subsequent "However if you are upgrading from a version earliers than" notes in later releases). It would be quite a pain I'm sure but I don't see any other way to get the information to someone upgrading in the future. I suppose we could just add the note to the current release notes on the theory that we only support installing the current release. -- greg
Re: allow running parts of src/tools/msvc/ under not Windows
On 2020-02-21 21:25, Tom Lane wrote: Peter Eisentraut writes: committed crake says that this doesn't pass perlcritic. OK, fixed. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: allow running parts of src/tools/msvc/ under not Windows
Peter Eisentraut writes: > committed crake says that this doesn't pass perlcritic. regards, tom lane
Re: Memory-Bounded Hash Aggregation
Hi, On 2020-02-19 20:16:36 +0100, Tomas Vondra wrote: > 3) I wonder if we need to invent new opcodes? Wouldn't it be simpler to > just add a new flag to the agg_* structs instead? I haven't tried hacking > this, so maybe it's a silly idea. New opcodes don't really cost that much - it's a jump table based dispatch already (yes, it increases the table size slightly, but not by much). But adding branches inside opcode implementation does add cost - and we're already bottlenecked by stalls. I assume code duplication is your primary concern here? If so, I think the patch 0008 in https://postgr.es/m/20191023163849.sosqbfs5yenocez3%40alap3.anarazel.de would improve the situation. I'll try to rebase that onto master. I'd also like to apply something like 0013 from that thread, I find the whole curperagg, select_current_set, curaggcontext logic confusing as hell. I'd so far planned to put this on the backburner until this patch has been committed, to avoid breaking it. But perhaps that's not the right call? Greetings, Andres Freund
Re: allow running parts of src/tools/msvc/ under not Windows
On 2020-02-21 05:00, Michael Paquier wrote: On Thu, Feb 20, 2020 at 09:31:32AM -0500, Tom Lane wrote: Peter Eisentraut writes: The main benefit is that if you make "blind" edits in the Perl files, you can verify them easily, first by seeing that the Perl code runs, second, depending on the circumstances, by diffing the created project files. Another is that if you do some nontrivial surgery in makefiles, you can check whether the Perl code can still process them. So the benefit is mainly that you can iterate faster when working on build system related things. You still need to do a full test on Windows at the conclusion, but then hopefully with a better chance of success. I see. No objection then. None from here either, and the patch is working correctly. committed -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Removing obsolete configure checks
Peter Eisentraut writes: > On 2020-02-20 19:00, Tom Lane wrote: >> I believe that we can also get rid of these tests: >> flexible array members >> cbrt >> intptr_t >> uintptr_t >> as these features are likewise required by C99. Solution.pm thinks that >> MSVC does not have the above, but I suspect its information is out of >> date. We could soon find out from the buildfarm, of course. > The flexible array members test on Solution.pm looks correct to me > (define to empty if supported, else define to 1). Yeah, I misread it the first time. > cbrt is probably a mistake or outdated. Right; at least, Microsoft's documentation claims to have it. We'll soon find out. > The intptr_t/uintptr_t results are inconsistent: > It correctly defines intptr_t to empty, so that it will use the existing > typedef, but it does not define HAVE_INTPTR_T, but nothing uses that > anyway. But these are gone now anyway. I forgot that your pending patch would nuke those, or I wouldn't have listed them. >> Unfortunately we're not actually asking for any of those to be probed >> for --- it looks like Autoconf just up and does that of its own accord. >> So we can't get rid of the tests and save configure cycles thereby. >> But we can skip testing the HAVE_FOO_H symbols for them. We mostly >> were already, but there's one or two exceptions. > Autoconf git master seems to have modernized that a little bit. For > instance, HAVE_STDLIB_H and HAVE_STRING_H are always defined to 1, just > for backward compatibility. If we wanted to fiddle with this, I'd > consider importing the updated macro. Not sure if it's worth it though. Hmm. If I thought they'd actually put out a new release sometime soon, I'd be content to wait for that. Seems like they have forgotten the rule about "great artists ship", though. Maybe we need to just periodically grab their git master? Keeping all committers in sync would be a problem though. regards, tom lane
Re: Removing obsolete configure checks
On 2020-02-20 19:00, Tom Lane wrote: I think we can just remove these tests, and the corresponding src/port/ files where there is one: fseeko isinf memmove rint signed types utime utime.h wchar.h makes sense I believe that we can also get rid of these tests: flexible array members cbrt intptr_t uintptr_t as these features are likewise required by C99. Solution.pm thinks that MSVC does not have the above, but I suspect its information is out of date. We could soon find out from the buildfarm, of course. The flexible array members test on Solution.pm looks correct to me (define to empty if supported, else define to 1). cbrt is probably a mistake or outdated. The intptr_t/uintptr_t results are inconsistent: It correctly defines intptr_t to empty, so that it will use the existing typedef, but it does not define HAVE_INTPTR_T, but nothing uses that anyway. But these are gone now anyway. I also noted that these header checks are passing everywhere, which is unsurprising because they're required by C99 and/or POSIX: ANSI C header files inttypes.h memory.h stdlib.h string.h strings.h sys/stat.h sys/types.h unistd.h Unfortunately we're not actually asking for any of those to be probed for --- it looks like Autoconf just up and does that of its own accord. So we can't get rid of the tests and save configure cycles thereby. But we can skip testing the HAVE_FOO_H symbols for them. We mostly were already, but there's one or two exceptions. Autoconf git master seems to have modernized that a little bit. For instance, HAVE_STDLIB_H and HAVE_STRING_H are always defined to 1, just for backward compatibility. If we wanted to fiddle with this, I'd consider importing the updated macro. Not sure if it's worth it though. There are a few other tests that are getting the same results in all buildfarm configure checks, but Solution.pm is injecting different results for Windows, such as what to expand "inline" to. MSVC indeed does not appear to support plain inline. Conceivably we could hard-code that based on the WIN32 #define and remove the configure probes, but I'm inclined to think it's not worth the trouble and possible loss of flexibility. Right, better to leave it. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: [PATCH] Replica sends an incorrect epoch in its hot standby feedback to the Master
Thanks a lot for the feedback. Please let me know if you have any further comments. Meanwhile, I have also added this patch to "Commitfest 2020-03" at https://commitfest.postgresql.org/27/2464. Thanks, Eka Palamadai Amazon Web Services On 2/11/20, 11:28 PM, "Thomas Munro" wrote: On Fri, Feb 7, 2020 at 1:03 PM Palamadai, Eka wrote: > The below problem occurs in Postgres versions 11, 10, and 9.6. However, it doesn’t occur since Postgres version 12, since the commit [6] to add basic infrastructure for 64-bit transaction IDs indirectly fixed it. I'm happy that that stuff is already fixing bugs we didn't know we had, but, yeah, it looks like it really only fixed it incidentally by moving all the duplicated "assign if higher" code into a function, not through the magical power of 64 bit xids. > The replica sends an incorrect epoch in its hot standby feedback to the master in the scenario outlined below, where a checkpoint is interleaved with the execution of 2 transactions at the master. The incorrect epoch in the feedback causes the master to ignore the “oldest Xmin” X sent by the replica. If a heap page prune[1] or vacuum were executed at the master immediately thereafter, they may use a newer “oldest Xmin” Y > X, and prematurely delete a tuple T such that X < t_xmax (T) < Y, which is still in use at the replica as part of a long running read query Q. Subsequently, when the replica replays the deletion of T as part of its WAL replay, it cancels the long running query Q causing unnecessary pain to customers. Ouch. Thanks for this analysis! > The variable “ShmemVariableCache->nextXid” (or “nextXid” for short) should be monotonically increasing unless it wraps around to the next epoch. However, in the above sequence, this property is violated on the replica in the function “RecordKnownAssignedTransactionIds”[3], when the WAL replay for the insertion at step 6 is executed at the replica. I haven't tried your repro or studied this closely yet, but yes, that assignment to nextXid does indeed look pretty fishy. Other similar code elsewhere always does a check like in your patch, before clobbering nextXid.
Re: Fix compiler warnings on 64-bit Windows
On 2020-02-20 17:24, Tom Lane wrote: =?UTF-8?Q?Juan_Jos=C3=A9_Santamar=C3=ADa_Flecha?= writes: On Mon, Feb 17, 2020 at 4:52 PM Tom Lane wrote: Anyway, I'll have a go at updating gaur to use 4.5.x. There is a sane-looking stdint.h on my second-oldest dinosaur, prairiedog. Don't know about the situation on Windows, though. We might want to take a close look at NetBSD, too, based on the GCC notes. As for Windows, stdint.h was included in VS2010, and currently Postgres supports VS2013 to 2019. I've now updated gaur to gcc 4.5.4 (took a little more hair-pulling than I would have wished). I confirm that 0001-Require-stdint.h.patch works in that environment, so I think you can go ahead and push it. Done, and also the appropriately reworked Windows warnings patch from the beginning of the thread. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Yet another vectorized engine
On 12.02.2020 13:12, Hubert Zhang wrote: On Tue, Feb 11, 2020 at 1:20 AM Konstantin Knizhnik mailto:k.knizh...@postgrespro.ru>> wrote: So looks like PG-13 provides significant advantages in OLAP queries comparing with 9.6! Definitely it doesn't mean that vectorized executor is not needed for new version of Postgres. Once been ported, I expect that it should provide comparable improvement of performance. But in any case I think that vectorized executor makes sense only been combine with columnar store. Thanks for the test. +1 on vectorize should be combine with columnar store. I think when we support this extension on master, we could try the new zedstore. I'm not active on this work now, but will continue when I have time. Feel free to join bring vops's feature into this extension. Thanks Hubert Zhang I have ported vectorize_engine to the master. It takes longer than I expected: a lot of things were changed in executor. Results are the following: par.warkers PG9_6 vectorize=off PG9_6 vectorize=on master vectorize=off jit=on master vectorize=off jit=off master vectorize=on jit=ofn master vectorize=on jit=off 0 36 20 16 25.5 15 17.5 4 10 - 5 7 - - So it proves the theory that JIT provides almost the same speedup as vector executor (both eliminates interpretation overhead but in different way). I still not sure that we need vectorized executor: because with standard heap it provides almost no improvements comparing with current JIT version. But in any case I am going to test it with vertical storage (zedstore or cstore). -- Konstantin Knizhnik Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
Re: [Patch] Make pg_checksums skip foreign tablespace directories
On 2/21/20 1:36 AM, Michael Paquier wrote:
> On Thu, Feb 20, 2020 at 05:38:15PM +0100, Bernd Helmle wrote:
>> So i propose a different approach like the attached patch tries to
>> implement: instead of just blindly iterating over directory contents
>> and filter them out, reference the tablespace location and
>> TABLESPACE_VERSION_DIRECTORY directly. This is done by a new function
>> scan_tablespaces() which is specialized in just follow the
>> symlinks/junctions in pg_tblspc and call scan_directory() with just
>> what it has found there. It will also honour directories, just in case
>> an experienced DBA has copied over the tablespace into pg_tblspc
>> directly.
>
> + if (S_ISREG(st.st_mode))
> + {
> + pg_log_debug("ignoring file %s in pg_tblspc", de->d_name);
> + continue;
> + }
> We don't do that for the normal directory scan path, so it does not
> strike me as a good idea on consistency ground. As a whole, I don't
> see much point in having a separate routine which is just roughly a
> duplicate of scan_directory(), and I think that we had better just add
> the check looking for matches with TABLESPACE_VERSION_DIRECTORY
> directly when having a directory, if subdir is "pg_tblspc". That
> also makes the patch much shorter.
+1. This is roughly what pg_basebackup does and it seems simpler to me.
Regards,
--
-David
da...@pgmasters.net
Re: [Patch] Make pg_checksums skip foreign tablespace directories
Hi Michael, On 2/20/20 11:07 PM, Michael Paquier wrote: > On Thu, Feb 20, 2020 at 07:37:15AM -0600, David Steele wrote: >> But since the name includes the backend's pid you would need to get lucky >> and have a new backend with the same pid create the file after a restart. I >> tried it and the old temp file was left behind after restart and first >> connection to the database. >> >> I doubt this is a big issue in the field, but it seems like it would be nice >> to do something about it. > > The natural area to do that would be around ResetUnloggedRelations(). > Still that would require combine both operations to not do any > unnecessary lookups at the data folder paths. Yeah, that's what I was thinking as well, since there is already a directory scan there and doing the check would be very cheap. It's not obvious how to combine these in the right way without moving a lot of code around to non-obvious places. One solution might be to have each subsystem register a function that does checks/cleanup as each path/file is found in a common scan function. That's a pretty major rework though, and I doubt there would be much appetite for it to solve such a minor problem. >> I'm not excited about the amount of code duplication between these three >> tools. I know this was because of back-patching various issues in the past, >> but I really think we need to unify these data structures/functions in HEAD. > > The lists are duplicated because we have never really figured out how > to combine this code in one place. The idea was to have all the data > folder path logic and the lists within one header shared between the > frontend and backend but there was not much support for that on HEAD. Do you have the thread? I'd like to see what was proposed and what the objections were. Regards, -- -David da...@pgmasters.net
Re: False failure during repeated windows build.
On Tue, Feb 18, 2020 at 8:06 AM Kyotaro Horiguchi wrote: > > The attached fixes that. After commit 9573384 this patch no longer applies, but with a trivial rebase it fixes the issue. Regards, Juan José Santamaría Flecha
Re: Parallel copy
On Thu, 20 Feb 2020 at 18:43, David Fetter wrote:>
> On Thu, Feb 20, 2020 at 02:36:02PM +0100, Tomas Vondra wrote:
> > I think the wc2 is showing that maybe instead of parallelizing the
> > parsing, we might instead try using a different tokenizer/parser and
> > make the implementation more efficient instead of just throwing more
> > CPUs on it.
>
> That was what I had in mind.
>
> > I don't know if our code is similar to what wc does, maytbe parsing
> > csv is more complicated than what wc does.
>
> CSV parsing differs from wc in that there are more states in the state
> machine, but I don't see anything fundamentally different.
The trouble with a state machine based approach is that the state
transitions form a dependency chain, which means that at best the
processing rate will be 4-5 cycles per byte (L1 latency to fetch the
next state).
I whipped together a quick prototype that uses SIMD and bitmap
manipulations to do the equivalent of CopyReadLineText() in csv mode
including quotes and escape handling, this runs at 0.25-0.5 cycles per
byte.
Regards,
Ants Aasma
#include
#include
#include
#include
#include
#include
#include
#define likely(x) __builtin_expect((x),1)
#define unlikely(x) __builtin_expect((x),0)
/*
* Create a bitmap of matching characters in the next 64 bytes
**/
static inline uint64_t
find_chars(__m256i *data, char c)
{
const __m256i mask = _mm256_set1_epi8(c);
uint64_t result = (uint32_t) _mm256_movemask_epi8(_mm256_cmpeq_epi8(data[0], mask));
result |= ((uint64_t) _mm256_movemask_epi8(_mm256_cmpeq_epi8(data[1], mask))) << 32;
return result;
}
/*
* Creates a bitmap of unpaired escape characters
**/
static inline uint64_t
find_unpaired_escapes(uint64_t escapes)
{
// TODO: handle unpaired escape from end of last iteration
uint64_t p, e, r;
p = escapes;
e = escapes;
r = escapes;
while (e) {
p = e;
e = (e << 1) & escapes;
r ^= e;
}
return r & p;
}
/*
* Creates a bitmap mask of quoted sections given locations of
* quote chatacters.
**/
static inline uint64_t
find_quote_mask(uint64_t quote_bits, uint64_t *prev_inside_quote)
{
uint64_t mask = _mm_cvtsi128_si64(_mm_clmulepi64_si128(
_mm_set_epi64x(0ULL, quote_bits), _mm_set1_epi8(0xFF), 0));
mask ^= *prev_inside_quote;
*prev_inside_quote = ((int64_t) mask) >> 63;
return mask;
}
/*
* Parses len bytes from buf according to csv rules and writes start positions of
* records to output. Returns number of rows found.
**/
int64_t
parseIntoLines(char *buf, size_t len, size_t *output)
{
__m256i* input = (__m256i*) buf;
uint64_t prev_inside_quote = 0;
size_t pos = 0;
uint64_t numfound = 0;
*output++ = 0;
numfound++;
while (pos < len - 64) {
uint64_t quotes = find_chars(input, '"');
uint64_t escapes = find_chars(input, '\\');
uint64_t unpaired_escapes = find_unpaired_escapes(escapes);
uint64_t unescaped_quotes = quotes & ~(unpaired_escapes << 1);
uint64_t newlines = find_chars(input, '\n');
uint64_t quote_mask = find_quote_mask(unescaped_quotes, _inside_quote);
uint64_t tokenpositions = newlines & ~quote_mask;
uint64_t carriages = find_chars(input, '\r') & ~quote_mask;
if (unlikely(carriages != 0))
exit(1);
uint64_t offset = 0;
while (tokenpositions > 0) {
int numchars = __builtin_ctzll(tokenpositions);
tokenpositions >>= numchars;
tokenpositions >>= 1;
offset += numchars + 1;
*output++ = pos + offset;
numfound++;
}
pos += 64;
input += 2;
}
// TODO: handle tail
return numfound;
}
int main(int argc, char *argv[])
{
char *buf;
uint64_t *lines;
uint64_t iters = 1;
if (argc < 2)
{
printf("Usage: simdcopy csvfile [iterations]\n");
return 1;
}
if (argc > 2)
{
iters = atol(argv[2]);
}
buf = aligned_alloc(64, 1024*1024*1024);
lines = aligned_alloc(8, 128*1024*1024*sizeof(uint64_t));
if (!buf || !lines)
return 1;
FILE *f = fopen(argv[1], "r");
if (!f)
return 1;
#define READBLOCK (1024*1024)
size_t len = 0;
while (len < sizeof(buf) - READBLOCK)
{
size_t result = fread(buf + len, 1, READBLOCK, f);
if (!result)
break;
len += result;
}
fclose(f);
struct timespec start;
struct timespec end;
printf("Parsing %lu bytes, %lu times\n", len, iters);
uint64_t numfound;
clock_gettime(CLOCK_MONOTONIC, );
for (uint64_t i = 0; i < iters; i++) {
numfound = parseIntoLines(buf, len, lines);
}
clock_gettime(CLOCK_MONOTONIC, );
double delta = (end.tv_sec - start.tv_sec) + (1.e-9)*(end.tv_nsec - start.tv_nsec);
printf("Found %lu rows in %lu bytes in %f milliseconds\n", numfound, len*iters, delta*1000);
printf(" Speed: %0.3f GB/s\n", len/delta/1e9*iters);
return 0;
}
Re: Minor improvement to partition_bounds_copy()
On Thu, Feb 20, 2020 at 10:52 PM Julien Rouhaud wrote: > On Thu, Feb 20, 2020 at 09:38:26PM +0900, Amit Langote wrote: > > On Thu, Feb 20, 2020 at 8:36 PM Etsuro Fujita > > wrote: > > > partition_bounds_copy() sets the hash_part and natts variable in each > > > iteration of a loop to copy the datums in the datums array, which > > > would not be efficient. Attached is small patch for avoiding that. > > > > That looks good to me. > > Looks good to me too! Pushed. Thanks, Amit and Julien! Best regards, Etsuro Fujita
Re: backend type in log_line_prefix?
On 2020-02-13 09:56, Peter Eisentraut wrote:
Attached is a demo patch that adds a placeholder %b for log_line_prefix
(not in the default setting) that contains the backend type, the same
that you see in pg_stat_activity and in the ps status. I would have
found this occasionally useful when analyzing logs, especially if you
have a lot of background workers active. Thoughts?
After positive initial feedback, here is a more ambitious patch set. In
particular, I wanted to avoid having to specify the backend type (at
least) twice, once for the ps display and once for this new facility.
I have added a new global variable MyBackendType that uses the existing
BackendType enum that was previously only used by the stats collector.
Then the ps display, the stats collector, the log_line_prefix, and other
places can just refer to this to know "who am I". (There are more
places like that, for example in the autovacuum system, so patch 0004 in
particular could be expanded in analogous ways.)
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From 29f9c92c5cdc7eddb60f69bb984a57e45a600938 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut
Date: Thu, 20 Feb 2020 18:07:37 +0100
Subject: [PATCH v2 1/4] Refactor ps_status.c API
The init_ps_display() arguments were mostly lies by now, so to match
typical usage, just use one argument and let the caller assemble it
from multiple sources if necessary. The only user of the additional
arguments is BackendInitialize(), which was already doing string
assembly on the caller side anyway.
Remove the second argument of set_ps_display() ("force") and just
handle that in init_ps_display() internally.
BackendInitialize() also used to set the initial status as
"authentication", but that was very far from where authentication
actually happened. So now it's set to "initializing" and then
"authentication" just before the actual call to
ClientAuthentication().
---
src/backend/access/transam/xlog.c | 4 ++--
src/backend/bootstrap/bootstrap.c | 2 +-
src/backend/commands/async.c | 4 ++--
src/backend/postmaster/autovacuum.c | 6 ++---
src/backend/postmaster/bgworker.c | 2 +-
src/backend/postmaster/pgarch.c | 8 +++
src/backend/postmaster/pgstat.c | 2 +-
src/backend/postmaster/postmaster.c | 32 ---
src/backend/postmaster/syslogger.c| 2 +-
src/backend/replication/basebackup.c | 2 +-
src/backend/replication/syncrep.c | 4 ++--
src/backend/replication/walreceiver.c | 7 +++---
src/backend/replication/walsender.c | 2 +-
src/backend/storage/ipc/standby.c | 4 ++--
src/backend/storage/lmgr/lock.c | 6 ++---
src/backend/tcop/postgres.c | 16 +++---
src/backend/utils/init/postinit.c | 3 ++-
src/backend/utils/misc/ps_status.c| 31 +++---
src/include/utils/ps_status.h | 5 ++---
19 files changed, 71 insertions(+), 71 deletions(-)
diff --git a/src/backend/access/transam/xlog.c
b/src/backend/access/transam/xlog.c
index 3813eadfb4..b4455799c4 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -3639,7 +3639,7 @@ XLogFileRead(XLogSegNo segno, int emode, TimeLineID tli,
/* Report recovery progress in PS display */
snprintf(activitymsg, sizeof(activitymsg), "waiting for
%s",
xlogfname);
- set_ps_display(activitymsg, false);
+ set_ps_display(activitymsg);
restoredFromArchive = RestoreArchivedFile(path,
xlogfname,
"RECOVERYXLOG",
@@ -3682,7 +3682,7 @@ XLogFileRead(XLogSegNo segno, int emode, TimeLineID tli,
/* Report recovery progress in PS display */
snprintf(activitymsg, sizeof(activitymsg), "recovering %s",
xlogfname);
- set_ps_display(activitymsg, false);
+ set_ps_display(activitymsg);
/* Track source of data in assorted state variables */
readSource = source;
diff --git a/src/backend/bootstrap/bootstrap.c
b/src/backend/bootstrap/bootstrap.c
index bfc629c753..4212333737 100644
--- a/src/backend/bootstrap/bootstrap.c
+++ b/src/backend/bootstrap/bootstrap.c
@@ -342,7 +342,7 @@ AuxiliaryProcessMain(int argc, char *argv[])
statmsg = "??? process";
break;
}
- init_ps_display(statmsg, "", "", "");
+ init_ps_display(statmsg);
}
/* Acquire configuration parameters, unless inherited from postmaster */
diff --git a/src/backend/commands/async.c b/src/backend/commands/async.c
index 9aa2b61600..50275f94ff 100644
Re: custom postgres launcher for tests
On Fri, Feb 21, 2020 at 4:49 AM Craig Ringer wrote: > I thought I saw a related patch to this that proposed to add a pg_ctl > argument. Was that you too? I can't find it at the moment. This very simple two-line patch for src/test/perl/PostgresNode.pm code, it set `pg_ctl -p ` argument, and one-line patch for src/test/regress/pg_regress.c it spawn postgres-launcher directly. This routine usable only for tap tests with used PostgresNode::get_new_node/start/restart, and for regress tests. Perhaps the name TEST_PGLAUNCHER is more correct for this env-var. >into doing whatever they want, so it's not a security concern >I currently do this with a horrid shellscript hack that uses next-on-path >lookups and a wrapper 'postgres' executable Its not security problem, because this kit only for developer, commonly - for iteratively build and run concrete tests. For more complexy replacing need patch for pg_ctl, or postgres wrapper, or replacing postgres bin and other ways... Thanks for the response!
Re: Portal->commandTag as an enum
Thinking about this some more, would it be possible to treat these
like we do parser/kwlist.h? Something like this:
commandtag_list.h:
PG_COMMANDTAG(ALTER_ACCESS_METHOD, "ALTER ACCESS METHOD", true, false,
false, false)
...
then, just:
#define PG_COMMANDTAG(taglabel, tagname, event_trigger, table_rewrite,
display_rowcount, display_oid) label,
typedef enum CommandTag
{
#include "commandtag_list.h"
}
#undef PG_COMMANDTAG
...and then:
#define PG_COMMANDTAG(taglabel, tagname, event_trigger, table_rewrite,
display_rowcount, display_oid) \
{ tagname, event_trigger, table_rewrite, display_rowcount, display_oid },
const CommandTagBehavior tag_behavior[] =
{
#include "commandtag_list.h"
}
#undef PG_COMMANDTAG
I'm hand-waving a bit, and it doesn't have the flexibility of a .dat
file, but it's a whole lot simpler.
--
John Naylorhttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: [Patch] Make pg_checksums skip foreign tablespace directories
Thank you David for decrypting my previous mail.., and your
translation was correct.
At Fri, 21 Feb 2020 15:07:12 +0900, Michael Paquier wrote
in
> On Thu, Feb 20, 2020 at 07:37:15AM -0600, David Steele wrote:
> > But since the name includes the backend's pid you would need to get lucky
> > and have a new backend with the same pid create the file after a restart. I
> > tried it and the old temp file was left behind after restart and first
> > connection to the database.
> >
> > I doubt this is a big issue in the field, but it seems like it would be nice
> > to do something about it.
>
> The natural area to do that would be around ResetUnloggedRelations().
> Still that would require combine both operations to not do any
> unnecessary lookups at the data folder paths.
>
> > I'm not excited about the amount of code duplication between these three
> > tools. I know this was because of back-patching various issues in the past,
> > but I really think we need to unify these data structures/functions in HEAD.
>
> The lists are duplicated because we have never really figured out how
> to combine this code in one place. The idea was to have all the data
> folder path logic and the lists within one header shared between the
> frontend and backend but there was not much support for that on HEAD.
>
> >> For now, my proposal is to fix the prefix first, and then let's look
> >> at the business with tablespaces where needed.
> >
> > OK.
>
> I'll let this patch round for a couple of extra day, and revisit it at
> the beginning of next week.
Thank you for the version.
I didn't look it closer bat it looks in the direction I wanted.
At a quick look, the following section attracted my eyes.
+ if (strncmp(de->d_name,
excludeFiles[excludeIdx].name,
+
strlen(excludeFiles[excludeIdx].name)) == 0)
+ {
+ elog(DEBUG1, "file \"%s\" matching
prefix \"%s\" excluded from backup",
+de->d_name,
excludeFiles[excludeIdx].name);
+ excludeFound = true;
+ break;
+ }
+ }
+ else
+ {
+ if (strcmp(de->d_name,
excludeFiles[excludeIdx].name) == 0)
+ {
+ elog(DEBUG1, "file \"%s\" excluded from
backup", de->d_name);
+ excludeFound = true;
+ break;
+ }
The two str[n]cmps are different only in matching length. I don't
think we don't need to differentiate the two message there, so we
could reduce the code as:
| cmplen = strlen(excludeFiles[].name);
| if (!prefix_patch)
| cmplen++;
| if (strncmp(d_name, excludeFilep.name, cmplen) == 0)
| ...
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Re: Autovacuum on partitioned table
On Fri, Feb 21, 2020 at 4:47 PM Masahiko Sawada wrote: > Thank you for updating the patch. I tested v4 patch. > > After analyze or autoanalyze on partitioned table n_live_tup and > n_dead_tup are updated. However, TRUNCATE and VACUUM on the > partitioned table don't change these values until invoking analyze or > autoanalyze whereas in normal tables these values are reset or > changed. For example, with your patch: > > * Before > relname | n_live_tup | n_dead_tup | n_mod_since_analyze > -+++- > c1 | 11 | 0 | 0 > c2 | 11 | 0 | 0 > c3 | 11 | 0 | 0 > c4 | 11 | 0 | 0 > c5 | 11 | 0 | 0 > parent | 55 | 0 | 0 > (6 rows) > > * After 'TRUNCATE parent' > relname | n_live_tup | n_dead_tup | n_mod_since_analyze > -+++- > c1 | 0 | 0 | 0 > c2 | 0 | 0 | 0 > c3 | 0 | 0 | 0 > c4 | 0 | 0 | 0 > c5 | 0 | 0 | 0 > parent | 55 | 0 | 0 > (6 rows) > > * Before > relname | n_live_tup | n_dead_tup | n_mod_since_analyze > -+++- > c1 | 0 | 11 | 0 > c2 | 0 | 11 | 0 > c3 | 0 | 11 | 0 > c4 | 0 | 11 | 0 > c5 | 0 | 11 | 0 > parent | 0 | 55 | 0 > (6 rows) > > * After 'VACUUM parent' > relname | n_live_tup | n_dead_tup | n_mod_since_analyze > -+++- > c1 | 0 | 0 | 0 > c2 | 0 | 0 | 0 > c3 | 0 | 0 | 0 > c4 | 0 | 0 | 0 > c5 | 0 | 0 | 0 > parent | 0 | 55 | 0 > (6 rows) > > We can make it work correctly but I think perhaps we can skip updating > statistics values of partitioned tables other than n_mod_since_analyze > as the first step. Because if we support also n_live_tup and > n_dead_tup, user might get confused that other statistics values such > as seq_scan, seq_tup_read however are not supported. +1, that makes sense. Thanks, Amit
Re: pg_regress cleans up tablespace twice.
At Thu, 20 Feb 2020 14:23:22 +0900, Michael Paquier wrote in > On Wed, Feb 19, 2020 at 04:06:33PM -0500, Tom Lane wrote: > > I think the existing coding dates from before we had a Perl driver for > > this, or else we had it but there were other less-functional ways to > > replace "make check" on Windows. +1 for taking the code out of > > pg_regress.c --- but I'm not in a position to say whether the other > > part of your patch is sufficient. > > Removing this code from pg_regress.c makes also sense to me. Now, the > patch breaks "vcregress installcheck" as this is missing to patch > installcheck_internal() for the tablespace path creation. I would > also recommend using a full path for the directory location to avoid > any potential issues if this code is refactored or moved around, the > patch now relying on the current path used. Hmm. Right. I confused database directory and tablespace directory. Tablespace directory should be provided by the test script, even though the database directory is preexisting in installcheck. To avoid useless future failure, I would do that that for all subcommands, as regress/GNUmakefile does. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
