Re: Online verification of checksums
Hi,
Am Sonntag, den 03.02.2019, 02:06 -0800 schrieb Andres Freund:
> Hi,
>
> On 2018-12-25 10:25:46 +0100, Fabien COELHO wrote:
> > Hallo Michael,
> >
> > > Yeah, new rebased version attached.
> >
> > Patch v8 applies cleanly, compiles, global & local make check are ok.
> >
> > A few comments:
> >
> > About added tests: the node is left running at the end of the script, which
> > is not very clean. I'd suggest to either move the added checks before
> > stopping, or to stop again at the end of the script, depending on the
> > intention.
>
> Michael?
Uh, I kinda forgot about this, I've made the tests stop the node now.
> > I'm wondering (possibly again) about the existing early exit if one block
> > cannot be read on retry: the command should count this as a kind of bad
> > block, proceed on checking other files, and obviously fail in the end, but
> > having checked everything else and generated a report. I do not think that
> > this condition warrants a full stop. ISTM that under rare race conditions
> > (eg, an unlucky concurrent "drop database" or "drop table") this could
> > happen when online, although I could not trigger one despite heavy testing,
> > so I'm possibly mistaken.
>
> This seems like a defensible judgement call either way.
Right now we have a few tests that explicitly check that
pg_verify_checksums fail on broken data ("foo" in the file). Those
would then just get skipped AFAICT, which I think is the worse behaviour
, but if everybody thinks that should be the way to go, we can
drop/adjust those tests and make pg_verify_checksums skip them.
Thoughts?
In the meanwhile, v9 is attached with the above change and rebased
(without changes) to master.
Michael
--
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax: +49 2166 9901-100
Email: michael.ba...@credativ.de
credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer
Unser Umgang mit personenbezogenen Daten unterliegt
folgenden Bestimmungen: https://www.credativ.de/datenschutzdiff --git a/doc/src/sgml/ref/pg_verify_checksums.sgml b/doc/src/sgml/ref/pg_verify_checksums.sgml
index 905b8f1222..4ad6edcde6 100644
--- a/doc/src/sgml/ref/pg_verify_checksums.sgml
+++ b/doc/src/sgml/ref/pg_verify_checksums.sgml
@@ -37,9 +37,8 @@ PostgreSQL documentation
Description
pg_verify_checksums verifies data checksums in a
- PostgreSQL cluster. The server must be shut
- down cleanly before running pg_verify_checksums.
- The exit status is zero if there are no checksum errors, otherwise nonzero.
+ PostgreSQL cluster. The exit status is zero if
+ there are no checksum errors, otherwise nonzero.
diff --git a/src/bin/pg_verify_checksums/pg_verify_checksums.c b/src/bin/pg_verify_checksums/pg_verify_checksums.c
index 511262ab5f..13fb51223a 100644
--- a/src/bin/pg_verify_checksums/pg_verify_checksums.c
+++ b/src/bin/pg_verify_checksums/pg_verify_checksums.c
@@ -1,7 +1,7 @@
/*
* pg_verify_checksums
*
- * Verifies page level checksums in an offline cluster
+ * Verifies page level checksums in a cluster
*
* Copyright (c) 2010-2019, PostgreSQL Global Development Group
*
@@ -26,10 +26,13 @@
static int64 files = 0;
static int64 blocks = 0;
static int64 badblocks = 0;
+static int64 skippedblocks = 0;
static ControlFileData *ControlFile;
+static XLogRecPtr checkpointLSN;
static char *only_relfilenode = NULL;
static bool verbose = false;
+static bool online = false;
static const char *progname;
@@ -86,10 +89,17 @@ scan_file(const char *fn, BlockNumber segmentno)
PageHeader header = (PageHeader) buf.data;
int f;
BlockNumber blockno;
+ bool block_retry = false;
f = open(fn, O_RDONLY | PG_BINARY, 0);
if (f < 0)
{
+ if (online && errno == ENOENT)
+ {
+ /* File was removed in the meantime */
+ return;
+ }
+
fprintf(stderr, _("%s: could not open file \"%s\": %s\n"),
progname, fn, strerror(errno));
exit(1);
@@ -104,26 +114,106 @@ scan_file(const char *fn, BlockNumber segmentno)
if (r == 0)
break;
+ if (r < 0)
+ {
+ fprintf(stderr, _("%s: could not read block %u in file \"%s\": %s\n"),
+ progname, blockno, fn, strerror(errno));
+ return;
+ }
if (r != BLCKSZ)
{
- fprintf(stderr, _("%s: could not read block %u in file \"%s\": read %d of %d\n"),
- progname, blockno, fn, r, BLCKSZ);
- exit(1);
+ if (block_retry)
+ {
+/* We already tried once to reread the block, bail out */
+fprintf(stderr, _("%s: could not read block %u in file \"%s\": read %d of %d\n"),
+ progname, blockno, fn, r, BLCKSZ);
+exit(1);
+ }
+
+ /*
+ * Retry the block. It's possible that we read the block while it
+ * was extended or shrinked, so it it ends up looking torn to us.
+ */
+
+ /*
+ * Seek back by the amount of bytes we read to the beginning of
+ * the failed block.
+ */
+ if (lseek(f, -r,
Re: Usage of epoch in txid_current
Hi,
On 2018-09-19 13:58:36 +1200, Thomas Munro wrote:
> +/*
> + * Advance nextFullXid to the value after a given xid. The epoch is
> inferred.
> + * If lock_free_check is true, then the caller must be sure that it's safe to
> + * read nextFullXid without holding XidGenLock (ie during recovery).
> + */
> +void
> +AdvanceNextFullTransactionIdPastXid(TransactionId xid, bool lock_free_check)
> +{
> + TransactionId current_xid;
> + uint32 epoch;
> +
> + if (lock_free_check &&
> + !TransactionIdFollowsOrEquals(xid,
> +
> XidFromFullTransactionId(ShmemVariableCache->nextFullXid)))
> + return;
> +
> + LWLockAcquire(XidGenLock, LW_EXCLUSIVE);
> + current_xid = XidFromFullTransactionId(ShmemVariableCache->nextFullXid);
> + if (TransactionIdFollowsOrEquals(xid, current_xid))
> + {
> + epoch =
> EpochFromFullTransactionId(ShmemVariableCache->nextFullXid);
> + if (xid < current_xid)
> + ++epoch; /* epoch wrapped */
> + ShmemVariableCache->nextFullXid = MakeFullTransactionId(epoch,
> xid);
> + FullTransactionIdAdvance(&ShmemVariableCache->nextFullXid);
> + }
> + LWLockRelease(XidGenLock);
> }
Is this really a good idea? Seems like it's going to perpetuate the
problematic epoch logic we had before?
> --- a/src/bin/pg_resetwal/pg_resetwal.c
> +++ b/src/bin/pg_resetwal/pg_resetwal.c
> @@ -431,11 +431,15 @@ main(int argc, char *argv[])
>* if any, includes these values.)
>*/
> if (set_xid_epoch != -1)
> - ControlFile.checkPointCopy.nextXidEpoch = set_xid_epoch;
> + ControlFile.checkPointCopy.nextFullXid =
> + MakeFullTransactionId(set_xid_epoch,
> +
> XidFromFullTransactionId(ControlFile.checkPointCopy.nextFullXid));
>
> if (set_xid != 0)
> {
> - ControlFile.checkPointCopy.nextXid = set_xid;
> + ControlFile.checkPointCopy.nextFullXid =
> +
> MakeFullTransactionId(EpochFromFullTransactionId(ControlFile.checkPointCopy.nextFullXid),
> + set_xid);
>
> /*
>* For the moment, just set oldestXid to a value that will force
> @@ -705,8 +709,7 @@ GuessControlValues(void)
> ControlFile.checkPointCopy.ThisTimeLineID = 1;
> ControlFile.checkPointCopy.PrevTimeLineID = 1;
> ControlFile.checkPointCopy.fullPageWrites = false;
> - ControlFile.checkPointCopy.nextXidEpoch = 0;
> - ControlFile.checkPointCopy.nextXid = FirstNormalTransactionId;
> + ControlFile.checkPointCopy.nextFullXid = MakeFullTransactionId(0,
> FirstNormalTransactionId);
> ControlFile.checkPointCopy.nextOid = FirstBootstrapObjectId;
> ControlFile.checkPointCopy.nextMulti = FirstMultiXactId;
> ControlFile.checkPointCopy.nextMultiOffset = 0;
> @@ -786,8 +789,8 @@ PrintControlValues(bool guessed)
> printf(_("Latest checkpoint's full_page_writes: %s\n"),
> ControlFile.checkPointCopy.fullPageWrites ? _("on") :
> _("off"));
> printf(_("Latest checkpoint's NextXID: %u:%u\n"),
> -ControlFile.checkPointCopy.nextXidEpoch,
> -ControlFile.checkPointCopy.nextXid);
> +
> EpochFromFullTransactionId(ControlFile.checkPointCopy.nextFullXid),
> +
> XidFromFullTransactionId(ControlFile.checkPointCopy.nextFullXid));
> printf(_("Latest checkpoint's NextOID: %u\n"),
> ControlFile.checkPointCopy.nextOid);
> printf(_("Latest checkpoint's NextMultiXactId: %u\n"),
> @@ -879,7 +882,7 @@ PrintNewControlValues(void)
> if (set_xid != 0)
> {
> printf(_("NextXID: %u\n"),
> -ControlFile.checkPointCopy.nextXid);
> +
> XidFromFullTransactionId(ControlFile.checkPointCopy.nextFullXid));
> printf(_("OldestXID:%u\n"),
> ControlFile.checkPointCopy.oldestXid);
> printf(_("OldestXID's DB: %u\n"),
> @@ -889,7 +892,7 @@ PrintNewControlValues(void)
> if (set_xid_epoch != -1)
> {
> printf(_("NextXID epoch:%u\n"),
> -ControlFile.checkPointCopy.nextXidEpoch);
> +
> EpochFromFullTransactionId(ControlFile.checkPointCopy.nextFullXid));
> }
I still think it's a mistake to not display the full xid here, and
rather perpetuate the epoch stuff. I'm ok with splitting that into a
separate commit, but this ought to be fixed in the same release imo.
> diff --git a/src/include/access/transam.h b/src/include/access/transam.h
> index 83ec3f19797..814becf96d7 10064
Re: WIP: Avoid creation of the free space map for small tables
On Mon, Feb 4, 2019 at 10:29 AM Amit Kapila wrote: > > On Mon, Feb 4, 2019 at 10:18 AM John Naylor > wrote: > > > > On Mon, Feb 4, 2019 at 4:17 AM Amit Kapila wrote: > > > This one seems to be FSM test portability issue (due to different page > > > contents, maybe). Looking into it, John, see if you are around and > > > have some thoughts on it. > > > > Maybe we can use the same plpgsql loop as fsm.sql that exits after 1 > > tuple has inserted into the 5th page. > > > > Yeah that can also work, but we still need to be careful about the > alignment of that one tuple, otherwise, there will could be different > free space on the fifth page. The probably easier way could be to use > an even number of integers in the table say(int, int). Anyway, for > now, I have avoided the dependency on FSM contents without losing on > coverage of test. I have pushed my latest suggestion in the previous > email. > The change seems to have worked. All the buildfarm machines that were showing the failure are passed now. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Re: Synchronous replay take III
On Mon, Feb 4, 2019 at 4:47 PM Michael Paquier wrote: > Last patch set fails to apply properly, so moved to next CF waiting on > author for a rebase. Thanks. Rebased. -- Thomas Munro http://www.enterprisedb.com 0001-Synchronous-replay-mode-for-avoiding-stale-reads-v11.patch Description: Binary data
RE: Commit Fest 2019-01 is now closed
From: Michael Paquier [mailto:mich...@paquier.xyz] > As per $subject, CF 2019-01 is now closed for business. Here is the final > score: > Committed: 58. > Moved to next CF: 113. > Withdrawn: 4. > Rejected: 3. > Returned with Feedback: 52. > Total: 230. Wow, thank you so much for your hard work. The last CF for PG 12 should be tough... Regards Takayuki Tsunakawa
Commit Fest 2019-01 is now closed
Hi all, As per $subject, CF 2019-01 is now closed for business. Here is the final score: Committed: 58. Moved to next CF: 113. Withdrawn: 4. Rejected: 3. Returned with Feedback: 52. Total: 230. I have done a pass over all the remaining entries, updating them according to their last status (hopefully!). There may be some mistakes, so please feel free to update a patch if you think that its status is not correct. This CF is no exception in the fact that many patches had a status on the CF app which was not consistent with the actual thread. So please be careful about that if you are an author or a reviewer. Thanks, -- Michael signature.asc Description: PGP signature
Re: partitioned tables referenced by FKs
On Tue, Jan 22, 2019 at 06:45:48PM -0300, Alvaro Herrera wrote: > Yes, very soon -- right now, in fact :-) This needs a rebase. Moved to next CF, waiting on author. -- Michael signature.asc Description: PGP signature
Re: Usage of epoch in txid_current
On February 4, 2019 6:43:44 AM GMT+01:00, Michael Paquier wrote: >On Sun, Feb 03, 2019 at 10:58:02PM +1100, Thomas Munro wrote: >> If there are no objections, I'm planning to do a round of testing and >> commit this shortly. > >Hm. That looks sane to me at quick glance. I am a bit on the edge >regaring the naming "FullTransactionId", which is actually a 64-bit >value with a 32-bit XID and a 32-bit epoch. Something like >TransactionIdWithEpoch or EpochTransactionId sounds a bit better to >me. My point is that "Full" is too generic for that. I'm not a fan of names with epoch in it - these are the real transaction IDs now. Conflating them with the until-now inferred epochs sounds like a bad idea to me. We IMO should just treat the new type as a 64bit uint, and the 32bit as a truncated version. Like, we could just add 64 as a prefix. Andres -- Sent from my Android device with K-9 Mail. Please excuse my brevity.
Re: [HACKERS] Can ICU be used for a database's default sort order?
On Tue, Dec 11, 2018 at 09:22:48AM +0100, Peter Eisentraut wrote: > If we have well-designed answers to these questions, I'd imagine that > the actual feature patch would be quite small. I was very surprised to > see how large this patch is and how much code is moves around without > much explanation. I don't think it's worth reviewing this patch any > further. It needs several steps back and some fundamental design and > refactoring work. Marked as returned with feedback. This thread has stalled. -- Michael signature.asc Description: PGP signature
Re: [HACKERS] SERIALIZABLE on standby servers
On Fri, Dec 28, 2018 at 02:21:44PM +1300, Thomas Munro wrote: > Just to be clear, although this patch is registered in the commitfest > and currently applies and tests pass, it is prototype/WIP code with > significant problems that remain to be resolved. I sort of wish there > were a way to indicate that in the CF but since there isn't, I'm > saying that here. What I hope to get from Kevin, Simon or other > reviewers is some feedback on the general approach and problems > discussed upthread (and other problems and better ideas I might have > missed). So it's not seriously proposed for commit in this CF. No feedback has actually come, so I have moved it to next CF. -- Michael signature.asc Description: PGP signature
Re: Flexible configuration for full-text search
On Thu, Nov 29, 2018 at 02:02:16PM +0100, Dmitry Dolgov wrote: > Maybe it would be better if you or some of your colleagues (Alexander, > Arthur?) > will post this new version, because the current one has some conflicts - so it > would be easier for a reviewers. For now I'll move it to the next CF. No updates for some time now, marked as returned with feedback. -- Michael signature.asc Description: PGP signature
Re: Statement-level rollback
On Thu, Jan 31, 2019 at 04:38:27AM -0800, Andres Freund wrote: > Is this still in development? Or should we mark this as returned with > feedback? Marked as returned with feedback, as it has already been two months. If you have an updated patch set, please feel free to resubmit. -- Michael signature.asc Description: PGP signature
Re: Conflict handling for COPY FROM
On Wed, Dec 19, 2018 at 02:48:14PM +0300, Surafel Temesgen wrote: > Thank you for informing, attach is rebased patch against current > master copy.c conflicts on HEAD, please rebase. I am moving the patch to next CF, waiting on author. -- Michael signature.asc Description: PGP signature
Re: Feature: temporary materialized views
On Tue, Jan 22, 2019 at 03:10:17AM +0100, Andreas Karlsson wrote: > On 1/21/19 3:31 AM, Andreas Karlsson wrote: > > Here is a a stab at refactoring this so the object creation does not > > happen in a callback. > > Rebased my patch on top of Andres's pluggable storage patches. Plus some > minor style changes. Taking a note to look at this refactoring bit, which is different from the temp matview part. Moved to next CF for now. -- Michael signature.asc Description: PGP signature
Re: [HACKERS] WIP: Aggregation push-down
On Tue, Jan 15, 2019 at 03:06:18PM +0100, Antonin Houska wrote: > This is the next version. A few more comments below. Latest patch set fails to apply, so moved to next CF, waiting on author. -- Michael signature.asc Description: PGP signature
Re: WIP: BRIN multi-range indexes
On Tue, Oct 02, 2018 at 11:49:05AM +0900, Michael Paquier wrote: > The latest patch set does not apply cleanly. Could you rebase it? I > have moved the patch to CF 2018-10 for now, waiting on author. It's been some time since that request, so I am marking the patch as returned with feedback. -- Michael signature.asc Description: PGP signature
Re: PATCH: logical_work_mem and logical streaming of large in-progress transactions
On Mon, Jan 14, 2019 at 07:23:31PM +0100, Tomas Vondra wrote: > Attached is an updated patch series, merging fixes and changes to TAP > tests proposed by Alexey. I've merged the fixes into the appropriate > patches, and I've kept the TAP changes / new tests as separate patches > towards the end of the series. Patch 4 of the latest set fails to apply, so I have moved the patch to next CF, waiting on author. -- Michael signature.asc Description: PGP signature
Re: Usage of epoch in txid_current
Michael Paquier writes: > Hm. That looks sane to me at quick glance. I am a bit on the edge > regaring the naming "FullTransactionId", which is actually a 64-bit > value with a 32-bit XID and a 32-bit epoch. Something like > TransactionIdWithEpoch or EpochTransactionId sounds a bit better to > me. My point is that "Full" is too generic for that. WideTransactionId, maybe? I agree that "Full" seems like a poor adjective here. regards, tom lane
Re: Synchronous replay take III
On Fri, Feb 01, 2019 at 09:34:49AM -0500, Robert Haas wrote: > Maybe I'm misunderstanding the terminology here, but if not, I find > this theory wildly implausible. *Most* people want read-your-writes > behavior. *Few* people want to wait for a dead standby. The only > application of the later is when even a tiny risk of transaction loss > is unacceptable, but the former has all kinds of clustering-related > uses. Last patch set fails to apply properly, so moved to next CF waiting on author for a rebase. -- Michael signature.asc Description: PGP signature
Re: Usage of epoch in txid_current
On Sun, Feb 03, 2019 at 10:58:02PM +1100, Thomas Munro wrote: > If there are no objections, I'm planning to do a round of testing and > commit this shortly. Hm. That looks sane to me at quick glance. I am a bit on the edge regaring the naming "FullTransactionId", which is actually a 64-bit value with a 32-bit XID and a 32-bit epoch. Something like TransactionIdWithEpoch or EpochTransactionId sounds a bit better to me. My point is that "Full" is too generic for that. Moved to next CF for now. -- Michael signature.asc Description: PGP signature
What happens if checkpoint haven't completed until the next checkpoint interval or max_wal_size?
In the name of god! Hi, What happens if checkpoint haven't completed until the next checkpoint interval or max_wal_size? thanks, regards, Mohammad Sherafat.
Re: Tid scan improvements
On Sat, 19 Jan 2019 at 17:04, Edmund Horner wrote:
> On Sat, 19 Jan 2019 at 05:35, Tom Lane wrote:
>
>> Edmund Horner writes:
>> > My patch uses the same path type and executor for all extractable
>> tidquals.
>>
>> > This worked pretty well, but I am finding it difficult to reimplement
>> it in
>> > the new tidpath.c code.
>>
>> I didn't like that approach to begin with, and would suggest that you go
>> over to using a separate path type and executor node. I don't think the
>> amount of commonality for the two cases is all that large, and doing it
>> as you had it required some ugly ad-hoc conventions about the semantics
>> of the tidquals list. Where I think this should go is that the tidquals
>> list still has OR semantics in the existing path type, but you use AND
>> semantics in the new path type, so that "ctid > ? AND ctid < ?" is just
>> represented as an implicit-AND list of two simple RestrictInfos.
>>
>
> Thanks for the advice. This approach resembles my first draft, which had
> a separate executor type. However, it did have a combined path type, with
> an enum TidPathMethod to determine how tidquals was interpreted. At this
> point, I think a different path type is clearer, though generation of both
> types can live in tidpath.c (just as indxpath.c generates different index
> path types).
>
Hi, here's a new set of patches. This one adds a new path type called
TidRangePath and a new execution node called TidRangeScan. I haven't
included any of the patches for adding pathkeys to TidPaths or
TidRangePaths.
1. v6-0001-Add-selectivity-estimate-for-CTID-system-variables.patch
2. v6-0002-Support-backward-scans-over-restricted-ranges-in-hea.patch
3. v6-0003-Support-range-quals-in-Tid-Scan.patch
4. v6-0004-TID-selectivity-reduce-the-density-of-the-last-page-.patch
Patches 1, 2, and 4 are basically unchanged from my previous post. Patch 4
is an optional tweak to the CTID selectivity estimates.
Patch 3 is a substantial rewrite from what I had before. I've checked
David's most recent review and tried to make sure the new code meets his
suggestions where applicable, although there is one spot where I left the
code as "if (tidrangequals) ..." instead of the preferred "if
(tidrangequals != NIL) ...", just for consistency with the surrounding code.
Questions --
1. Tid Range Paths are costed as random_page_cost for the first page, and
sequential page cost for the remaining pages. It made sense when there
could be multiple non-overlapping ranges. Now that there's only one range,
it might not, but it has the benefit of making Tid Range Scans a little bit
more expensive than Sequential Scans, so that they are less likely to be
picked when a Seq Scan will do just as well. Is there a better cost
formula to use?
2. Is it worth trying to get rid of some of the code duplication between
the TidPath and TidRangePath handling, such as in costsize.c or
createplan.c?
3. TidRangeRecheck (copied from TidRecheck) has an existing comment asking
whether it should actually be performing a check on the returned tuple. It
seems to me that as long as TidRangeNext doesn't return a tuple outside the
requested range, then the check shouldn't be necessary (and we'd simplify
the comment to "nothing to check"). If a range key can change at runtime,
it should never have been included in the TidRangePath. Is my
understanding correct?
4. I'm a little uncomfortable with the way heapam.c changes the scan limits
("--scan->rs_numblocks") as it progresses through the pages. I have the
executor node reset the scan limits after scanning all the tuples, which
seems to work for the tests I have, but I'm using the
heap_setscanlimits feature in a slightly different way from the only
existing use, which is for the one-off scans when building a BRIN index. I
have added some tests for cursor fetches which seems to exercise the code,
but I'd still appreciate close review of how I'm using heapam.
Edmund
v6-0001-Add-selectivity-estimate-for-CTID-system-variables.patch
Description: Binary data
v6-0003-Support-range-quals-in-Tid-Scan.patch
Description: Binary data
v6-0004-TID-selectivity-reduce-the-density-of-the-last-page-.patch
Description: Binary data
v6-0002-Support-backward-scans-over-restricted-ranges-in-hea.patch
Description: Binary data
Re: [PATCH] kNN for btree
On Fri, Jan 11, 2019 at 04:01:51PM +0300, Nikita Glukhov wrote: > All new distance functions except oiddist() are not leakproof, > so I had to relax condition in opr_sanity.sql test: This patch set needs a rebase because of conflicts caused by the recent patches for pluggable storage. -- Michael signature.asc Description: PGP signature
Re: FETCH FIRST clause WITH TIES option
On Wed, Jan 16, 2019 at 11:45:46AM +0300, Surafel Temesgen wrote: > The attached patch use your suggestion uptread This patch needs a rebase because of conflicts done recently for pluggable storage, so moved to next CF, waiting on author. -- Michael signature.asc Description: PGP signature
Re: [HACKERS] Custom compression methods
On Mon, Dec 03, 2018 at 03:43:32PM +0300, Ildus Kurbangaliev wrote: > Hi, here is a rebased version. I hope it will get some review :) This patch set is failing to apply, so moved to next CF, waiting for author. -- Michael signature.asc Description: PGP signature
Re: amcheck verification for GiST
On Thu, Jan 31, 2019 at 03:58:48PM -0800, Peter Geoghegan wrote: > I think that holding a buffer lock on an internal pages for as long as > it takes to check all of the child pages is a non-starter. If you > can't think of a way of not doing that that's race free with a > relation-level AccessShareLock, then a relation-level ShareLock (which > will block VACUUM) seems necessary. (Please be careful to update the status of the patch in the CF correctly!) This review is recent, so I have moved the patch to next CF, waiting for input from the author. -- Michael signature.asc Description: PGP signature
Re: Undo logs
On Sun, Feb 3, 2019 at 3:53 PM Andres Freund wrote: > > Hi, > > This thread is curently marked as returned with feedback, set so > 2018-12-01. Given there've been several new versions submitted since, is > that accurate? > As part of this thread we have been reviewing and fixing the comment for undo-interface patch. Now, Michael have already moved to new commitfest with status need review so I guess as of now the status is correct. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
Re: [HACKERS] logical decoding of two-phase transactions
On Fri, Jan 25, 2019 at 02:03:27PM -0300, Alvaro Herrera wrote: > Eyeballing 0001, it has a few problems. > > 1. It's under-parenthesizing the txn argument of the macros. > > 2. the "has"/"is" macro definitions don't return booleans -- see > fce4609d5e5b. > > 3. the remainder of this no longer makes sense: > > /* Do we know this is a subxact? Xid of top-level txn if so */ > - boolis_known_as_subxact; > TransactionId toplevel_xid; > > I suggest to fix the comment, and also improve the comment next to the > macro that tests this flag. > > > (4. the macro names are ugly.) This is an old thread, and the latest review is very recent. So I am moving the patch to next CF, waiting on author. -- Michael signature.asc Description: PGP signature
Re: [HACKERS] Support for Secure Transport SSL library on macOS as OpenSSL alternative
On Fri, Nov 30, 2018 at 02:00:00PM +0100, Dmitry Dolgov wrote: > Unfortunately, patch needs to be fixed - it doesn't pass "make -C ssl check" > > t/001_ssltests.pl .. 1/65 Bailout called. Further testing stopped: > system pg_ctl failed > FAILED--Further testing stopped: system pg_ctl failed > > Could you post an updated version? This has not been answered yet, and a couple of months have gone by, so I am marking the patch as returned with feedback. -- Michael signature.asc Description: PGP signature
Re: Undo logs
On Mon, Feb 4, 2019 at 3:55 PM Michael Paquier wrote: > On Sun, Feb 03, 2019 at 02:23:16AM -0800, Andres Freund wrote: > > This thread is curently marked as returned with feedback, set so > > 2018-12-01. Given there've been several new versions submitted since, is > > that accurate? > > From the latest status of this thread, there have been new patches but > no reviews on them, so moved to next CF. Thank you. New patches coming soon. -- Thomas Munro http://www.enterprisedb.com
Re: proposal: plpgsql pragma statement
On Fri, Jan 04, 2019 at 02:17:49PM +0100, Pavel Stehule wrote: > It means to write own lexer and preparse source code before I start > checking. > > I think so block level PRAGMA is significantly better solution Please note that the latest patch is failing to apply, so I have moved the patch to next CF, waiting on author. -- Michael signature.asc Description: PGP signature
Re: [HACKERS] Two pass CheckDeadlock in contentent case
On Fri, Nov 30, 2018 at 04:44:37PM +0100, Dmitry Dolgov wrote: > Thanks for the review. Just for the records, patch still has no conflicts and > pass all the tests. Yura, do you have any plans about this patch, could you > respond to the feedback? In the meantime I'm moving it to the next CF. No answers since the latest review, so I am marking the patch as returned with feedback. -- Michael signature.asc Description: PGP signature
Re: Reduce amount of WAL generated by CREATE INDEX for gist, gin and sp-gist
On Tue, Dec 18, 2018 at 10:41:48AM +0500, Andrey Lepikhov wrote: > Ok. It is used only for demonstration. The latest patch set needs a rebase, so moved to next CF, waiting on author as this got no reviews. -- Michael signature.asc Description: PGP signature
Re: ATTACH/DETACH PARTITION CONCURRENTLY
On Mon, 4 Feb 2019 at 16:45, Robert Haas wrote: > > On Sat, Feb 2, 2019 at 7:19 PM David Rowley > wrote: > > I think we do need to ensure that the PartitionDesc matches between > > worker and leader. Have a look at choose_next_subplan_for_worker() in > > nodeAppend.c. Notice that a call is made to > > ExecFindMatchingSubPlans(). > > Thanks for the tip. I see that code, but I'm not sure that I > understand why it matters here. First, if I'm not mistaken, what's > being returned by ExecFindMatchingSubPlans is a BitmapSet of subplan > indexes, not anything that returns to a PartitionDesc directly. And > second, even if it did, it looks like the computation is done > separately in every backend and not shared among backends, so even if > it were directly referring to PartitionDesc indexes, it still won't be > assuming that they're the same in every backend. Can you further > explain your thinking? In a Parallel Append, each parallel worker will call ExecInitAppend(), which calls ExecCreatePartitionPruneState(). That function makes a call to RelationGetPartitionDesc() and records the partdesc's boundinfo in context->boundinfo. This means that if we perform any pruning in the parallel worker in choose_next_subplan_for_worker() then find_matching_subplans_recurse() will use the PartitionDesc from the parallel worker to translate the partition indexes into the Append's subnodes. If the PartitionDesc from the parallel worker has an extra partition than what was there when the plan was built then the partition index to subplan index translation will be incorrect as the find_matching_subplans_recurse() will call get_matching_partitions() using the context with the PartitionDesc containing the additional partition. The return value from get_matching_partitions() is fine, it's just that the code inside the while ((i = bms_next_member(partset, i)) >= 0) loop that will do the wrong thing. It could even crash if partset has an index out of bounds of the subplan_map or subpart_map arrays. -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: WIP: Avoid creation of the free space map for small tables
On Mon, Feb 4, 2019 at 10:18 AM John Naylor wrote: > > On Mon, Feb 4, 2019 at 4:17 AM Amit Kapila wrote: > > This one seems to be FSM test portability issue (due to different page > > contents, maybe). Looking into it, John, see if you are around and > > have some thoughts on it. > > Maybe we can use the same plpgsql loop as fsm.sql that exits after 1 > tuple has inserted into the 5th page. > Yeah that can also work, but we still need to be careful about the alignment of that one tuple, otherwise, there will could be different free space on the fifth page. The probably easier way could be to use an even number of integers in the table say(int, int). Anyway, for now, I have avoided the dependency on FSM contents without losing on coverage of test. I have pushed my latest suggestion in the previous email. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Re: Ordered Partitioned Table Scans
On Thu, Jan 31, 2019 at 04:29:56PM +1300, David Rowley wrote: > I've attached a rebased patch which fixes up the recent conflicts. No > other changes were made. Moved to next CF, waiting for review. -- Michael signature.asc Description: PGP signature
RE: Protect syscache from bloating with negative cache entries
Horiguchi-san, Bruce,
Thank you for telling me your ideas behind this feature. Frankly, I don't
think I understood the proposed specification is OK, but I can't explain it
well at this instant. So, let me discuss that in a subsequent mail.
Anyway, here are my review comments on 0001:
(1)
(1)
+/* GUC variable to define the minimum age of entries that will be cosidered to
+ /* initilize catcache reference clock if haven't done yet */
cosidered -> considered
initilize -> initialize
I remember I saw some other wrong spelling and/or missing words, which I forgot
(sorry).
(2)
Only the doc prefixes "sys" to the new parameter names. Other places don't
have it. I think we should prefix sys, because relcache and plancache should
be configurable separately because of their different usage patterns/lifecycle.
(3)
The doc doesn't describe the unit of syscache_memory_target. Kilobytes?
(4)
+ hash_size = cp->cc_nbuckets * sizeof(dlist_head);
+ tupsize = sizeof(CatCTup) + MAXIMUM_ALIGNOF + dtp->t_len;
+ tupsize = sizeof(CatCTup);
GetMemoryChunkSpace() should be used to include the memory context overhead.
That's what the files in src/backend/utils/sort/ do.
(5)
+ if (entry_age > cache_prune_min_age)
">=" instead of ">"?
(6)
+ if (!ct->c_list || ct->c_list->refcount
== 0)
+ {
+ CatCacheRemoveCTup(cp, ct);
It's better to write "ct->c_list == NULL" to follow the style in this file.
"ct->refcount == 0" should also be checked prior to removing the catcache
tuple, just in case the tuple hasn't been released for a long time, which might
hardly happen.
(7)
CatalogCacheCreateEntry
+ int tupsize = 0;
if (ntp)
{
int i;
+ int tupsize;
tupsize is defined twice.
(8)
CatalogCacheCreateEntry
In the negative entry case, the memory allocated by CatCacheCopyKeys() is not
counted. I'm afraid that's not negligible.
(9)
The memory for CatCList is not taken into account for syscache_memory_target.
Regards
Takayuki Tsunakawa
Re: jsonpath
On Tue, Jan 29, 2019 at 04:51:46AM +0300, Alexander Korotkov wrote: > Oh, sorry. I've missed we have ERRCODE_TO_CATEGORY() here. Note: patch set moved to next CF, still waiting on author. -- Michael signature.asc Description: PGP signature
Re: [HACKERS] Transactions involving multiple postgres foreign servers, take 2
On Thu, Jan 31, 2019 at 11:09:09AM +0100, Masahiko Sawada wrote: > Thanks. Actually I'm updating the patch set, changing API interface as > I proposed before and improving the document and README. I'll submit > the latest patch next week. Cool, I have moved the patch to next CF. -- Michael signature.asc Description: PGP signature
Re: Undo logs
On Sun, Feb 03, 2019 at 02:23:16AM -0800, Andres Freund wrote: > This thread is curently marked as returned with feedback, set so > 2018-12-01. Given there've been several new versions submitted since, is > that accurate? From the latest status of this thread, there have been new patches but no reviews on them, so moved to next CF. -- Michael signature.asc Description: PGP signature
Re: [HACKERS] PATCH: multivariate histograms and MCV lists
On Sun, Feb 03, 2019 at 02:43:24AM -0800, Andres Freund wrote: > Are you planning to update the patch, or should the entry be marked as > RWF? Moved the patch to next CF for now, waiting on author as the last review happened not so long ago. -- Michael signature.asc Description: PGP signature
Re: has_table_privilege for a table in unprivileged schema causes an error
On Sun, Nov 18, 2018 at 06:32:42PM -0500, Tom Lane wrote: > So at this point I'm not sure what to think, other than that things > are inconsistent (and underdocumented). Nagata-san, do you have some plans to do something about the comments raised. The thread has been inactive for a couple of months now, so I am marking the patch as returned with feedback. -- Michael signature.asc Description: PGP signature
Re: Refactoring the checkpointer's fsync request queue
On Wed, Jan 30, 2019 at 09:59:38PM -0800, Shawn Debnath wrote: > I (finally) got a chance to go through these patches and they look > great. Thank you for working on this! This review is very recent, so I have moved the patch to next CF. -- Michael signature.asc Description: PGP signature
Re: WIP: Avoid creation of the free space map for small tables
On Mon, Feb 4, 2019 at 4:17 AM Amit Kapila wrote: > This one seems to be FSM test portability issue (due to different page > contents, maybe). Looking into it, John, see if you are around and > have some thoughts on it. Maybe we can use the same plpgsql loop as fsm.sql that exits after 1 tuple has inserted into the 5th page. -- John Naylorhttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: WIP: Avoid creation of the free space map for small tables
On Mon, Feb 4, 2019 at 9:24 AM Amit Kapila wrote: > On Mon, Feb 4, 2019 at 8:47 AM Amit Kapila wrote: > > One more similar failure: > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=lapwing&dt=2019-02-04%2003%3A20%3A01 > > So, basically, this is due to difference in the number of tuples that > can fit on a page. The freespace in FSM for the page is shown > different because of available space on a particular page. This can > vary due to alignment. It seems to me we can't rely on FSM contents > if there are many tuples in a relation. One idea is to get rid of > dependency on FSM contents in this test, can you think of any better > way to have consistent FSM contents across different platforms? > One more idea could be that we devise a test (say with a char/varchar) such that it always consume same space in a page irrespective of its alignment. Yet another way could be we use explain (costs off, analyze on, timing off, summary off) ..., this will ensure that we will have test coverage for function fsm_page_contents, but we don't rely on its contents. What do you think? I will go with last option to stablize the buildfarm tests unless anyone thinks otherwise or has better idea. I willprobably wait for 20 minutes or so to see if anyone has inputs. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Re: Global shared meta cache
On Mon, Nov 26, 2018 at 12:12:09PM +, Ideriha, Takeshi wrote: > On this allocation stuffs I'm trying to handle it in another thread > [1] in a broader way. Based on the latets updates of this thread, this is waiting for review, so moved to next CF. -- Michael signature.asc Description: PGP signature
Re: Protect syscache from bloating with negative cache entries
On Mon, Jan 28, 2019 at 01:31:43PM +0900, Kyotaro HORIGUCHI wrote: > I'll consider the last choice and will come up with a patch. Update is recent, so I have just moved the patch to next CF. -- Michael signature.asc Description: PGP signature
Re: WIP: Avoid creation of the free space map for small tables
On Mon, Feb 4, 2019 at 8:47 AM Amit Kapila wrote:
>
> On Mon, Feb 4, 2019 at 12:39 AM John Naylor
> wrote:
> >
> > On Sun, Feb 3, 2019 at 2:06 PM Amit Kapila wrote:
> > >
> > > On Thu, Jan 31, 2019 at 6:03 PM Amit Kapila
> > > wrote:
> > > This doesn't get applied cleanly after recent commit 0d1fe9f74e.
> > > Attached is a rebased version. I have checked once that the changes
> > > done by 0d1fe9f74e don't impact this patch. John, see if you can also
> > > once confirm whether the recent commit (0d1fe9f74e) has any impact. I
> > > am planning to push this tomorrow morning (IST) unless you or anyone
> > > see any problem with this.
> >
> > Since that commit changes RelationAddExtraBlocks(), which can be
> > induces by your pgbench adjustment upthread, I ran make check with
> > that adjustment in the pgbench dir 300 times without triggering
> > asserts.
> >
> > I also tried to follow the logic in 0d1fe9f74e, and I believe it will
> > be correct without a FSM.
> >
>
> I have just pushed it and buildfarm has shown two failures:
>
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=dromedary&dt=2019-02-04%2002%3A27%3A26
>
> ---
> /Users/buildfarm/bf-data/HEAD/pgsql.build/contrib/pageinspect/expected/page.out
> 2019-02-03 21:27:29.0 -0500
> +++
> /Users/buildfarm/bf-data/HEAD/pgsql.build/contrib/pageinspect/results/page.out
> 2019-02-03 21:41:32.0 -0500
> @@ -38,19 +38,19 @@
> SELECT * FROM fsm_page_contents(get_raw_page('test_rel_forks', 'fsm', 0));
> fsm_page_contents
> ---
> - 0: 39+
> - 1: 39+
> - 3: 39+
> - 7: 39+
> - 15: 39 +
> - 31: 39 +
> - 63: 39 +
> ..
>
> This one seems to be FSM test portability issue (due to different page
> contents, maybe). Looking into it, John, see if you are around and
> have some thoughts on it.
>
One more similar failure:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=lapwing&dt=2019-02-04%2003%3A20%3A01
So, basically, this is due to difference in the number of tuples that
can fit on a page. The freespace in FSM for the page is shown
different because of available space on a particular page. This can
vary due to alignment. It seems to me we can't rely on FSM contents
if there are many tuples in a relation. One idea is to get rid of
dependency on FSM contents in this test, can you think of any better
way to have consistent FSM contents across different platforms?
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
Re: make installcheck-world in a clean environment
04.02.2019 5:09, Michael Paquier wrote: > On Mon, Dec 03, 2018 at 11:58:13AM +0300, Alexander Lakhin wrote: >> Rebased the patch once more after d3c09b9b. > Moved to next CF, waiting on author as the patch conflicts with HEAD. > -- > Michael Hello Michael, It's very strange, I looked at http://cfbot.cputube.org/next.html from time to time (last time, two days ago) and the patch passed all checks just fine. Also I've tried to apply make-installcheck-v5.patch from https://www.postgresql.org/message-id/dabc52ea-14b6-3109-26b1-e83a9dc62...@gmail.com, it applies cleanly. Can you explain, what exactly conflicts with HEAD? Best regards, Alexander
Re: Why are we PageInit'ing buffers in RelationAddExtraBlocks()?
On Mon, Feb 4, 2019 at 1:09 PM Amit Kapila wrote: > On Sun, Feb 3, 2019 at 8:48 PM Andres Freund wrote: > > And one on eelpout, which appears to be unrelated as well: > > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=eelpout&dt=2019-02-03%2011%3A54%3A13 > None of these failures seem to be related to your commit. Yeah, that seems to be a problem with recent OpenSSL. I have some analysis over here, but I don't know what exactly is going on yet: https://www.postgresql.org/message-id/flat/CAEepm%3D2n6Nv%2B5tFfe8YnkUm1fXgvxR0Mm1FoD%2BQKG-vLNGLyKg%40mail.gmail.com -- Thomas Munro http://www.enterprisedb.com
Re: ATTACH/DETACH PARTITION CONCURRENTLY
On Sat, Feb 2, 2019 at 7:19 PM David Rowley wrote: > I think we do need to ensure that the PartitionDesc matches between > worker and leader. Have a look at choose_next_subplan_for_worker() in > nodeAppend.c. Notice that a call is made to > ExecFindMatchingSubPlans(). Thanks for the tip. I see that code, but I'm not sure that I understand why it matters here. First, if I'm not mistaken, what's being returned by ExecFindMatchingSubPlans is a BitmapSet of subplan indexes, not anything that returns to a PartitionDesc directly. And second, even if it did, it looks like the computation is done separately in every backend and not shared among backends, so even if it were directly referring to PartitionDesc indexes, it still won't be assuming that they're the same in every backend. Can you further explain your thinking? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Pluggable Storage - Andres's take
On Tue, Jan 22, 2019 at 1:43 PM Haribabu Kommi wrote: > > > OK. I will work on the doc changes. > Sorry for the delay. Attached a draft patch of doc and comments changes that I worked upon. Currently I added comments to the callbacks that are present in the TableAmRoutine structure and I copied it into the docs. I am not sure whether it is a good approach or not? I am yet to add description for the each parameter of the callbacks for easier understanding. Or, Giving description of each callbacks in the docs with division of those callbacks according to them divided in the TableAmRoutine structure? Currently following divisions are available. 1. Table scan 2. Parallel table scan 3. Index scan 4. Manipulation of physical tuples 5. Non-modifying operations on individual tuples 6. DDL 7. Planner 8. Executor Suggestions? Regards, Haribabu Kommi Fujitsu Australia 0001-Doc-and-comments-update.patch Description: Binary data
Re: WIP: Avoid creation of the free space map for small tables
On Mon, Feb 4, 2019 at 12:39 AM John Naylor wrote:
>
> On Sun, Feb 3, 2019 at 2:06 PM Amit Kapila wrote:
> >
> > On Thu, Jan 31, 2019 at 6:03 PM Amit Kapila wrote:
> > This doesn't get applied cleanly after recent commit 0d1fe9f74e.
> > Attached is a rebased version. I have checked once that the changes
> > done by 0d1fe9f74e don't impact this patch. John, see if you can also
> > once confirm whether the recent commit (0d1fe9f74e) has any impact. I
> > am planning to push this tomorrow morning (IST) unless you or anyone
> > see any problem with this.
>
> Since that commit changes RelationAddExtraBlocks(), which can be
> induces by your pgbench adjustment upthread, I ran make check with
> that adjustment in the pgbench dir 300 times without triggering
> asserts.
>
> I also tried to follow the logic in 0d1fe9f74e, and I believe it will
> be correct without a FSM.
>
I have just pushed it and buildfarm has shown two failures:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=dromedary&dt=2019-02-04%2002%3A27%3A26
---
/Users/buildfarm/bf-data/HEAD/pgsql.build/contrib/pageinspect/expected/page.out
2019-02-03 21:27:29.0 -0500
+++
/Users/buildfarm/bf-data/HEAD/pgsql.build/contrib/pageinspect/results/page.out
2019-02-03 21:41:32.0 -0500
@@ -38,19 +38,19 @@
SELECT * FROM fsm_page_contents(get_raw_page('test_rel_forks', 'fsm', 0));
fsm_page_contents
---
- 0: 39+
- 1: 39+
- 3: 39+
- 7: 39+
- 15: 39 +
- 31: 39 +
- 63: 39 +
..
This one seems to be FSM test portability issue (due to different page
contents, maybe). Looking into it, John, see if you are around and
have some thoughts on it.
2.
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=dory&dt=2019-02-04%2002%3A30%3A25
select explain_parallel_append('execute ab_q5 (33, 44, 55)');
-explain_parallel_append
- Finalize Aggregate (actual rows=1 loops=1)
- -> Gather (actual rows=3 loops=1)
- Workers Planned: 2
- Workers Launched: 2
- -> Partial Aggregate (actual rows=1 loops=3)
- -> Parallel Append (actual rows=0 loops=N)
- Subplans Removed: 8
- -> Parallel Seq Scan on ab_a1_b1 (never executed)
- Filter: ((b < 4) AND (a = ANY (ARRAY[$1, $2, $3])))
-(9 rows)
-
+ERROR: lost connection to parallel worker
+CONTEXT: PL/pgSQL function explain_parallel_append(text) line 5 at
FOR over EXECUTE statement
-- Test Parallel Append with PARAM_EXEC Params
select explain_parallel_append('select count(*) from ab where (a =
(select 1) or a = (select 3)) and b = 2');
explain_parallel_append
Failure is something like:
2019-02-03 21:44:42.456 EST [2812:327] pg_regress/partition_prune LOG:
statement: select explain_parallel_append('execute ab_q5 (1, 1, 1)');
2019-02-03 21:44:42.493 EST [2812:328] pg_regress/partition_prune LOG:
statement: select explain_parallel_append('execute ab_q5 (2, 3, 3)');
2019-02-03 21:44:42.531 EST [2812:329] pg_regress/partition_prune LOG:
statement: select explain_parallel_append('execute ab_q5 (33, 44,
55)');
2019-02-04 02:44:42.552 GMT [4172] FATAL: could not reattach to
shared memory (key=01B4, addr=0198): error
code 487
2019-02-03 21:44:42.555 EST [5116:6] LOG: background worker "parallel
worker" (PID 4172) exited with exit code 1
2019-02-03 21:44:42.560 EST [2812:330] pg_regress/partition_prune
ERROR: lost connection to parallel worker
I don't think this is related to this commit.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
Re: Synchronize with imath upstream
On Sun, Feb 03, 2019 at 10:31:26AM -0500, Tom Lane wrote:
> Noah Misch writes:
> > The -Wno-declaration-after-statement approach takes eight lines of code, and
> > the filter-out approach takes one. On the other hand, using $(filter-out)
> > changes any runs of whitespace to single spaces ("$(filter-out foo,ab
> > c)"
> > yields "a b c"). We do risk that with CPPFLAGS and LDFLAGS in a few places.
> > I don't want to proliferate that practice, because it changes semantics of
> > CFLAGS containing -DFOO="arbitrarytext".
>
> I don't particularly buy that argument, because CPPFLAGS is where any -D
> switches ought to be put. So we've already exposed ourselves to this
> risk, in the unlikely scenario where it's not hypothetical.
The $(filter-out) corruption is unlikely to matter, indeed. The question is
whether to use eight lines of code to inject -Wno-declaration-after-statement
or one line to remove -Wdeclaration-after-statement using $(filter-out). I
see negligible drawbacks on either side; both approaches are tolerable. The
above-described hypothetical problem tips the scale in favor of
-Wno-declaration-after-statement.
Re: Progress reporting for pg_verify_checksums
On Tue, Dec 25, 2018 at 11:45:09PM -0300, Alvaro Herrera wrote: > Umm, this is established coding pattern in pg_basebackup.c. > Stylistically I'd change all those cases to "fprintf(stderr, > isatty(fileno(stderr)) ? "\r" : "\n")" but leave the string alone, since > AFAIR it took some time to figure out what to do. (I'd also make the > comment one line instead of four, say "Stay on the same line if > reporting to a terminal". That makes the whole stanza two lines rather > than eight, which is the appropriate amount of space for it). There has been no input from the author for a couple of weeks now, so I have marked the patch as returned with feedback. -- Michael signature.asc Description: PGP signature
Re: Online verification of checksums
On Sun, Feb 03, 2019 at 02:06:45AM -0800, Andres Freund wrote: > On 2018-12-25 10:25:46 +0100, Fabien COELHO wrote: >> About added tests: the node is left running at the end of the script, which >> is not very clean. I'd suggest to either move the added checks before >> stopping, or to stop again at the end of the script, depending on the >> intention. > > Michael? Unlikely P., and most likely B. I have marked the patch as returned with feedback as it has been a couple of weeks already. -- Michael signature.asc Description: PGP signature
RE: pg_upgrade: Pass -j down to vacuumdb
Hi, On February 1, 2019 8:14 PM +, Jesper Pedersen wrote: > On 2/1/19 4:58 AM, Alvaro Herrera wrote: >> On 2019-Feb-01, Jamison, Kirk wrote: >>> I'm not sure if misunderstood the purpose of $VACUUMDB_OPTS. I >>> thought what the other developers suggested is to utilize it so that >>> --jobs for vacuumdb would be optional and just based from user-specified >>> option $VACUUMDB_OPTS. >>> By which it would not utilize the amount of jobs used for pg_upgrade. >>> To address your need of needing a parallel, the script would just >>> then suggest if the user wants a --jobs option. The if/else for >>> number of jobs is not needed in that case, maybe only for ifndef WIN32 else >>> case. >> >> No Kirk, you got it right -- (some of) those ifdefs are not needed, as >> adding the --jobs in the command line is not needed. I do think some >> extra whitespace in the format string is needed. > The point of the patch is to highlight to the user that even though he/she > does "pg_upgrade -j 8" the "-j 8" option isn't passed down to vacuumdb. > Default value is 1, so in that case the echo command doesn't highlight that > fact, otherwise it is. The user can then cancel the script and use the > suggested command line. > The script then executes the vaccumdb command with the $VACUUMDB_OPTS > argument as noted in the documentation. > Sample script attached as well. Sorry I think I might have understood now where you are coming from, where your script clarifies that it's not necessarily passed down. If committers can let it pass, maybe it's necessary to highlight in the script, Something like: "If you would like default statistics as quickly as possible (e.g. run in parallel)..." The difference is still perhaps your use of --jobs option as somehow "explicitly" embedded in the code, compared to the suggestion of making it optional by using the $VACUUMDB_OPTS variable, so that jobs need not to be explicitly written, unless the user wants to. (which I also think it's a better improvement because it's optional) Some quick code check. sorry, don't have much time to write a patch for it today. Perhaps you could write it similar to what you did in the --analyze-in-stages part. Maybe something like (not sure if correct): + fprintf(script, "echo %s\"%s/vacuumdb\" %s $VACUUMDB_OPTS --all %s%s\n", ECHO_QUOTE, + new_cluster.bindir, user_specification.data, + /* Did we copy the free space files? */ + (GET_MAJOR_VERSION(old_cluster.major_version) >= 804) ? + "--analyze-only" : "--analyze", ECHO_QUOTE); I'll continue to review though from time to time. Regards, Kirk Jamison
Re: Using POPCNT and other advanced bit manipulation instructions
On Thu, Jan 31, 2019 at 07:45:02PM -0300, Alvaro Herrera wrote: > Other than those minor changes, I think we should just get this pushed > and see what the buildfarm thinks. In the words of a famous PG hacker: > if a platform ain't in the buildfarm, we don't support it. Moved to next CF, waiting on author. I think that this needs more reviews. -- Michael signature.asc Description: PGP signature
Re: log bind parameter values on error
On Mon, Jan 28, 2019 at 12:15:51AM +, Alexey Bashtanov wrote: > I'm sorry for the delay, feel free to move it to next commitfest if > needed. Done. -- Michael signature.asc Description: PGP signature
Re: possible deadlock: different lock ordering for heap pages
On Fri, Feb 1, 2019 at 10:50 AM Amit Kapila wrote: > > On Mon, Jan 21, 2019 at 10:39 PM Nishant, Fnu wrote: > > > > Thanks Amit for your review. > > > > On 1/20/19, 6:55 AM, "Amit Kapila" wrote: > > > I think you need to change below code as well > >Assert(buffer2 == InvalidBuffer || buffer1 <= buffer2); > > > > Done. Updated the patch. > > > > Attached is an updated patch. I have edited comments and commit > message in the patch. I would like to backpatch this till 9.4 unless > anyone thinks otherwise. > Pushed this patch and backpatched till 9.4. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Re: shared-memory based stats collector
On Tue, Jan 22, 2019 at 03:48:02PM +0900, Kyotaro HORIGUCHI wrote: > Fixed doubious memory context usage. That's quite something that we have here for 0005: 84 files changed, 6588 insertions(+), 7501 deletions(-) Moved to next CF for now. -- Michael signature.asc Description: PGP signature
Re: Log a sample of transactions
On Sun, Feb 03, 2019 at 12:23:00PM +0100, Adrien Nayrat wrote: > I did not find any test for log_min_duration that could help me. LCOV indicate > there is no tests on this part (look check_log_duration()): > https://coverage.postgresql.org/src/backend/tcop/postgres.c.gcov.html These would take time to execute, even if you need to measure one second, and may be hard to make stable on slow machines. Moved to next CF. -- Michael signature.asc Description: PGP signature
Re: Timeout parameters
On Mon, Jan 28, 2019 at 04:51:11AM +, Nagaura, Ryohei wrote: > Sorry for my late. Moved to next CF per the latest updates: there is a patch with no reviews for it. -- Michael signature.asc Description: PGP signature
Re: proposal: new polymorphic types - commontype and commontypearray
On Wed, Jan 30, 2019 at 05:08:03PM +0100, Pavel Stehule wrote: > maybe "supertype". It is one char shorter .. somewhere is term > "supperclass, ..." > > In Czech language this term is short, "nadtyp", but probably it is not > acceptable :) Moved to next CF. -- Michael signature.asc Description: PGP signature
Re: explain plans with information about (modified) gucs
On Tue, Jan 22, 2019 at 02:29:06AM +0100, Tomas Vondra wrote: > Yes, that's an omission in the docs. Will fix. Could you fix your patch then? I am moving it to next CF, waiting on author. -- Michael signature.asc Description: PGP signature
Re: [HACKERS] [PATCH v2] Add and report the new "session_read_only" GUC pseudo-variable.
On Fri, Nov 30, 2018 at 05:35:21PM +0100, Dmitry Dolgov wrote: > Apparently due the minor conflict, mentioned above, the patch was in "Waiting > on author" state, which probably is not exactly correct. I'm moving it to the > next CF, but still, it would be nice if you post an updated version without > conflicts. And nothing has happened since, so I have marked the entry as returned with feedback. -- Michael signature.asc Description: PGP signature
Re: pgsql: Avoid creation of the free space map for small heap relations.
On Mon, Jan 28, 2019 at 07:20:29AM +0100, John Naylor wrote: > That particular test could be removed -- it's just verifying behavior > that's already been there for years and is a direct consquence of > normal truncation combined with the addressing scheme of the FSM > logic. Moved to next CF, please feel free to continue this work, this is interesting stuff. -- Michael signature.asc Description: PGP signature
Re: [HACKERS] proposal - Default namespaces for XPath expressions (PostgreSQL 11)
On Thu, Jan 31, 2019 at 02:54:49PM +0100, Pavel Stehule wrote: > Unfortunately, the development of libxml2 is frozen. > > I have to accept it. And marked as rejected, based on the last consensus. -- Michael signature.asc Description: PGP signature
Re: idle-in-transaction timeout error does not give a hint
On Tue, Dec 04, 2018 at 04:07:34AM +, Ideriha, Takeshi wrote: > Sure. I didn't have a strong opinion about it, so it's ok. From what I can see this is waiting input from a native English speaker, so for now I have moved this entry to next CF. -- Michael signature.asc Description: PGP signature
Re: GiST VACUUM
On Fri, Jan 04, 2019 at 06:26:18PM +0500, Andrey Borodin wrote: > Heikki, how do you think, is implementing our own radix tree for > this is viable solution? > I've written working implementation with 4-level statically typed > tree. If we follow this route, probably, there must be tests for > this data structure. (Note that the latest patch does not apply.) Heikki, are you planning to answer to the questions and/or review the patch? I have moved it to next CF for now. -- Michael signature.asc Description: PGP signature
Re: Why are we PageInit'ing buffers in RelationAddExtraBlocks()?
On Sun, Feb 3, 2019 at 8:48 PM Andres Freund wrote: > > Hi, > > On 2019-02-01 07:14:11 -0800, Andres Freund wrote: > > Here's a version of the patch implementing this approach. I assume this > > solves the FreeBSD issue, but I'm running tests in a loop on Thomas' > > machine. > > I pushed this, and the buildfarm so far is showing more love. > > There's a failure on jacana, but it looks like it's a machine issue: > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=jacana&dt=2019-02-03%2013%3A17%3A57 > Feb 03 08:36:16 rm: cannot remove > `/home/pgrunner/bf/root/HEAD/pgsql.build/tmp_install/home/pgrunner/bf/root/HEAD/inst/bin/postgres.exe': > Permission denied > > And one on eelpout, which appears to be unrelated as well: > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=eelpout&dt=2019-02-03%2011%3A54%3A13 > > # Failed test 'intermediate client certificate is missing: matches' > # at t/001_ssltests.pl line 385. > # 'psql: server closed the connection unexpectedly > # This probably means the server terminated abnormally > # before or while processing the request. > # could not send startup packet: Connection reset by peer > # ' > # doesn't match '(?^:SSL error)' > # Looks like you failed 1 test of 71. > [12:15:36] t/001_ssltests.pl .. > Dubious, test returned 1 (wstat 256, 0x100) > None of these failures seem to be related to your commit. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Re: make installcheck-world in a clean environment
On Mon, Dec 03, 2018 at 11:58:13AM +0300, Alexander Lakhin wrote: > Rebased the patch once more after d3c09b9b. The patch is ready for committer, so it has not attracted much attention. Perhaps Teodor or Alexander could look at it? I have moved the patch to next CF with the same status. -- Michael signature.asc Description: PGP signature
Re: make installcheck-world in a clean environment
On Mon, Dec 03, 2018 at 11:58:13AM +0300, Alexander Lakhin wrote: > Rebased the patch once more after d3c09b9b. Moved to next CF, waiting on author as the patch conflicts with HEAD. -- Michael signature.asc Description: PGP signature
Re: [PATCH] pg_hba.conf : new auth option : clientcert=verify-full
On Thu, Dec 27, 2018 at 12:14:03PM +0100, Magnus Hagander wrote: > I definitely am. In fact, I was ages ago (was planning for early December, > but hey, see wher that let me), so my apologies for failing at that. But it > definitely remains on my list of things to get to! So, Magnus, where are we on this? I have moved the patch to next CF, waiting on author as the latest patch does not apply. Could it be rebased? -- Michael signature.asc Description: PGP signature
Re: SQL/JSON: documentation
On Mon, Dec 03, 2018 at 07:23:09PM +0300, Liudmila Mantrova wrote: > Unfortunately, I couldn't find much time for this activity, but as far as I > understand, thread [1] only requires jsonpath documentation right now. So I > extracted the relevant parts from this patch, reworked path expression > description, and moved it to func.sgml as Peter suggested (attached). Nikita > is going to add this patch to the jsonpath thread together with the updated > code once it's ready. For now the entry is marked as returned with feedback. -- Michael signature.asc Description: PGP signature
Re: INSTALL file
On Mon, Jan 28, 2019 at 10:21:03PM +0100, Peter Eisentraut wrote: > I'm not in favor of listing all these versions here. It's one more > thing to keep updated. The version requirements are not outrageous, so > we can assume that someone with a reasonably up-to-date development > machine has appropriate versions. +1. I have just looked at the patch, and my take is that listing all the ways to build Postgres directly in the README is just a duplication of what we already have in the documentation. So I would just rip out all that and keep a simple link to the documentation. > I'm also not convinced that this list is actually complete. From an > "empty" start you'd typically need at least zlib and readline on top of > those. Yep. For now I have moved the patch to next CF, waiting on author. -- Michael signature.asc Description: PGP signature
Re: [PATCH] Improvements to "Getting started" tutorial for Google Code-in task
On Sat, Jan 05, 2019 at 04:21:23PM +0100, Peter Eisentraut wrote: > On 04/01/2019 00:05, Alvaro Herrera wrote: >> Besides that, I have a hard time considering this patch committable. >> There are some good additions, but they are mixed with some wording >> changes that seem to be there just because the author doesn't like the >> original, not because they're actual improvements. > > I agree. I don't find any of the changes to be improvements. I have marked the patch as returned with feedback, as it has been waiting input from the author for four weeks now. -- Michael signature.asc Description: PGP signature
Re: Proposal for Signal Detection Refactoring
On Wed, Jan 23, 2019 at 11:55:09AM +0100, Chris Travers wrote: > attached is a new signal handing patch. Path is corrected and moved. The > documentation is sightly streamlined in some places and expanded in others. This has not been reviewed, so moved to next CF. -- Michael signature.asc Description: PGP signature
Re: psql display of foreign keys
On Wed, Jan 02, 2019 at 09:38:40PM +0100, Peter Eisentraut wrote: > I'm setting this to "Waiting on Author", awaiting a new version based on > pg_partition_root() once that one is done. pg_partition_root() has not made it to the finish line yet, still it would have been nice to see a rebase, and the patch has been waiting for input for 4 weeks now. So I am marking it as returned with feedback. -- Michael signature.asc Description: PGP signature
Re: [proposal] Add an option for returning SQLSTATE in psql error message
On Mon, Jan 07, 2019 at 10:44:24PM +0100, didier wrote: > On Sat, Jan 5, 2019 at 6:30 PM Tom Lane wrote: >> Peter Eisentraut writes: >>> Why are you not including a test for \set VERBOSITY verbose? >> >> Stability of the output would be a problem ... >> >> Yes it could moreover the functionality wasn't tested before. > Should I add one ? Unpredictible outputs mean more maintenance and more alternate outputs. I have moved the patch to next CF, still ready for committer. -- Michael signature.asc Description: PGP signature
Re: DECLARE STATEMENT Syntax support
On Thu, Dec 27, 2018 at 04:46:42AM +, Kuroda, Hayato wrote: > Nobody give comments, but I revised my patches. There are many patches in the bucket. I can see that you are reviewing a bit other's patches, though those are really lower complexity. > In this update, I combined files, fixed some explanations, > and gave new number. > > Your comments or suggestions are very needed. The patch needs a rebase and has not been reviewed, so I have moved it to next CF, waiting on author. -- Michael signature.asc Description: PGP signature
Re: pg_dump multi VALUES INSERT
On Sun, Feb 03, 2019 at 01:21:45PM +0300, Surafel Temesgen wrote: > at least for processing user argument i think it is better to use strtol or > other > function that have better error handling. i can make a patch that change > usage > of atoi for user argument processing after getting feedback from here or i > will do > simultaneously Moved the patch to next CF for now, the discussion is going on. -- Michael signature.asc Description: PGP signature
Re: libpq host/hostaddr/conninfo inconsistencies
On Fri, Nov 30, 2018 at 01:08:51PM +0100, Dmitry Dolgov wrote: > Why then not split the original proposal into two patches, one to improve the > documentation, and another to make it more user friendly? Moved to next CF for now. From what I can see the latest patch manipulates the same areas of the documentation, so keeping things grouped would reduce the global amount of diffs. -- Michael signature.asc Description: PGP signature
Re: ToDo: show size of partitioned table
On Wed, Dec 19, 2018 at 07:38:16AM +0100, Pavel Stehule wrote: > I am sending updated patch. Moved to next CF. -- Michael signature.asc Description: PGP signature
Re: Psql patch to show access methods info
On Mon, Dec 10, 2018 at 07:38:39PM +0300, s.cherkas...@postgrespro.ru wrote: > Here are some fixes. But I'm not sure that the renaming of columns for the > '\dAp' command is sufficiently laconic and informative. If you have any > suggestions on how to improve them, I will be very grateful. I have not put much thougts into that to be honest. For now I have moved the patch to next CF. -- Michael signature.asc Description: PGP signature
Re: Libpq support to connect to standby server as priority
On Mon, Jan 21, 2019 at 06:48:14AM +, Tsunakawa, Takayuki wrote: > That would be fine. But as I mentioned in another mail, I think > "get read-only session" and "connect to standby" differ. So I find > it better to separate parameters for those request; > target_session_attr and target_server_type. We've had plenty of discussions about this patch, and nothing really got out of the crowd. For now I am marking the patch as returned with feedback as it has been marked as waiting on author for two weeks now. It may be worth continuing the discussion, still we need to come up with an agreement first. -- Michael signature.asc Description: PGP signature
Re: current_logfiles not following group access and instead follows log_file_mode permissions
On Fri, Feb 1, 2019 at 7:22 PM Michael Paquier wrote: > On Fri, Jan 18, 2019 at 09:50:40AM -0500, Stephen Frost wrote: > > Yes, we should update the documentation in this regard, though it's > > really an independent thing as that documentation should have been > > updated in the original group-access patch, so I'll see about fixing > > it and back-patching it. > > Stephen, could you apply Hari's patch then? I am not sure what the > consensus is, but documenting the restriction is the minimum we can > do. > > -The default permissions are 0600, meaning only the > -server owner can read or write the log files. The other commonly > -useful setting is 0640, allowing members of the > owner's > -group to read the files. Note however that to make use of such a > -setting, you'll need to alter to > -store the files somewhere outside the cluster data directory. In > -any case, it's unwise to make the log files world-readable, since > -they might contain sensitive data. > +The default permissions are either 0600, meaning > only the > +server owner can read or write the log files or > 0640, that > +allows any user in the same group can read the log files, based on > the new > +cluster created with --allow-group-access option of > initdb > +command. Note however that to make use of any setting other than > default, > +you'll need to alter to store the > files > +somewhere outside the cluster data directory. > > I would formulate that differently, by just adding an extra paragraph > to mention that using 0640 is recommended to be > compatible with initdb's --allow-group-access instead of sticking it > on the middle of the existing paragraph. > Thanks for the review. I changed the log_file_mode doc patch as per your comment. How about the attached? And regarding current_logfiles permissions, I feel this file should have permissions of data directory files as it is present in the data directory whether it stores the information of log file, until this file is completely removed with another approach to store the log file details. I am not sure whether this has been already discussed or not? How about using shared memory to store the log file names? So that we don't need of this file? Regards, Haribabu Kommi Fujitsu Australia 0001-log_file_mode-recommended-value-update.patch Description: Binary data
Re: initdb --allow-group-access behaviour in windows
On Mon, Feb 04, 2019 at 10:27:05AM +1100, Haribabu Kommi wrote: > +1 to the above changes. Thanks for working on it. Okay, done. -- Michael signature.asc Description: PGP signature
RE: reloption to prevent VACUUM from truncating empty pages at the end of relation
From: Michael Paquier [mailto:mich...@paquier.xyz] > On Fri, Feb 01, 2019 at 09:11:50AM +0100, Laurenz Albe wrote: > > Perhaps "vacuum_shrink_enabled" would be even better. > > Naming it just vacuum_truncate and autovacuum_truncate (with aliases for > toast and such), looks more natural to me. "shrink" is not a term used > in the code at all to describe this phase of vacuuming, and this option > talks mainly to people who are experts in PostgreSQL internals in my opinion. FYI, it seems that the user sees "shrink" rather than "truncate" in the documentation as below, although these are about VACUUM FULL. https://www.postgresql.org/docs/devel/sql-vacuum.html would like the table to physically shrink to occupy less disk space https://www.postgresql.org/docs/devel/routine-vacuuming.html shrink a table back to its minimum size and return the disk space to the operating system, Anyway, I don't have any favor about naming this, and I hope native English speakers will choose the best name. I won't object to whatever name any committer chooses. Regards Takayuki Tsunakawa
Re: Ryu floating point output patch
Andrew Gierth writes: > [ ryu11.patch ] I can confirm this compiles and passes core regression tests on my HPPA dinosaur. regards, tom lane
Re: Changing SQL Inlining Behaviour (or...?)
I wrote: > I've posted some preliminary design ideas at > https://www.postgresql.org/message-id/15193.1548028...@sss.pgh.pa.us > and > https://www.postgresql.org/message-id/15289.1548028...@sss.pgh.pa.us > While there's a nontrivial amount of work needed to make that happen, > I think it's doable, and it would lead to a significantly better > solution than proceeding along the inlining path could do. My > current feeling, therefore, is that we should reject this patch > (or at least stick it in the deep freeze) and go work on that plan. Now that the first of those threads has reached a feature-complete state, I feel fairly comfortable in saying that we should drop the idea of messing with the inlining heuristics (at least for the particular end goal stated in this thread). So I'm going to go close this CF entry as returned-with-feedback. regards, tom lane
Re: Tid scan improvements
Hi, my apologies for the delay. I've finished rebasing and rewriting it for Tom's changes to tidpath.c and his recommendations for tid range scans, but I then found a bug with cursor interaction. Specifically, FETCH LAST scans through the whole range, and then proceeds to scan backwards to get the last row. It worked in both my very first draft, and in the most recent draft before the changes to tidpath, but I haven't got it working yet for the new version. I'm hoping to get that fixed in the next 24 hours, and I'll then post the new patch. Edmund On Sun, 3 Feb 2019 at 23:34, Andres Freund wrote: > Hi, > > On 2019-01-19 17:04:13 +1300, Edmund Horner wrote: > > On Sat, 19 Jan 2019 at 05:35, Tom Lane wrote: > > > > > Edmund Horner writes: > > > > My patch uses the same path type and executor for all extractable > > > tidquals. > > > > > > > This worked pretty well, but I am finding it difficult to > reimplement it > > > in > > > > the new tidpath.c code. > > > > > > I didn't like that approach to begin with, and would suggest that you > go > > > over to using a separate path type and executor node. I don't think > the > > > amount of commonality for the two cases is all that large, and doing it > > > as you had it required some ugly ad-hoc conventions about the semantics > > > of the tidquals list. Where I think this should go is that the > tidquals > > > list still has OR semantics in the existing path type, but you use AND > > > semantics in the new path type, so that "ctid > ? AND ctid < ?" is just > > > represented as an implicit-AND list of two simple RestrictInfos. > > > > > > > Thanks for the advice. This approach resembles my first draft, which > had a > > separate executor type. However, it did have a combined path type, with > an > > enum TidPathMethod to determine how tidquals was interpreted. At this > > point, I think a different path type is clearer, though generation of > both > > types can live in tidpath.c (just as indxpath.c generates different index > > path types). > > > > > > > Now admittedly, this wouldn't give us an efficient way to execute > > > queries with conditions like "WHERE ctid = X OR (ctid > Y AND ctid < > Z)", > > > but I find myself quite unable to get excited about supporting that. > > > I see no reason for the new code to worry about any cases more complex > > > than one or two TID inequalities at top level of the restriction list. > > > > > > > I'm a bit sad to see support for multiple ranges go, though I never saw > > such queries as ever being particularly common. (And there was always a > > nagging feeling that tidpath.c was beginning to perform feats of boolean > > acrobatics out of proportion to its importance. Perhaps in some distant > > future, TID quals will become another way of supplying TIDs to a bitmap > > heap scan, which would enable complicated boolean queries using both > > indexes and TID scans. But that's just musing, not a proposal.) > > > > > In the query information given to the path generator, there is no > existing > > > > RestrictInfo relating to the whole expression "ctid > ? AND ctid < > ?". I > > > > am still learning about RestrictInfos, but my understanding is it > doesn't > > > > make sense to have a RestrictInfo for an AND clause, anyway; you're > > > > supposed to have them for the sub-expressions of it. > > > > > > FWIW, the actual data structure for cases like that is that there's > > > a RestrictInfo for the whole clause ctid = X OR (ctid > Y AND ctid < > Z), > > > and if you look into its "orclause" field, you will find RestrictInfos > > > attached to the primitive clauses ctid = X, ctid > Y, ctid < Z. (The > > > old code in tidpath.c didn't know that, because it'd never been > rewritten > > > since RestrictInfos were invented.) However, I think this new code > should > > > not worry about OR cases at all, but just pull out top-level TID > > > comparison clauses. > > > > > > > Thanks for the explanation. > > > > > And it doesn't seem a good idea to try to create new RestrictInfos in > the > > > > path generation just to pass the tidquals back to plan creation. > > > > > > No, you should avoid that. There are places that assume there's only > > > one RestrictInfo for any given original clause (or sub-clause). > > > The commitfest has ended, and you've not updated the patch to address > the feedback yet. Are you planning to do so soon? Otherwise I think we > ought to mark the patch as returned with feedback? > > Greetings, > > Andres Freund >
Re: initdb --allow-group-access behaviour in windows
On Sun, Feb 3, 2019 at 10:34 AM Michael Paquier wrote: > On Sat, Feb 02, 2019 at 08:50:14AM +0200, David Steele wrote: > > How about: > > > > +files created by initdb. This option is > ignored > > +on Windows, which does not support > > +POSIX-style group permissions. > > Fine for me. Anybody else has an opinion to offer? > +1 to the above changes. Thanks for working on it. Regards, Haribabu Kommi Fujitsu Australia
Re: New vacuum option to do only freezing
On Sat, Feb 2, 2019 at 7:00 PM Alvaro Herrera wrote:
>
> On 2019-Feb-01, Bossart, Nathan wrote:
>
> > IMHO we could document this feature at a slightly higher level without
> > leaving out any really important user-facing behavior. Here's a quick
> > attempt to show what I am thinking:
> >
> > With this option, VACUUM skips all index cleanup behavior and
> > only marks tuples as "dead" without reclaiming the storage.
> > While this can help reclaim transaction IDs faster to avoid
> > transaction ID wraparound (see Section 24.1.5), it will not
> > reduce bloat.
>
> Hmm ... don't we compact out the storage for dead tuples? If we do (and
> I think we should) then this wording is not entirely correct.
Yeah, we remove tuple and leave the dead ItemId. So we actually
reclaim the almost tuple storage.
>
> > Note that this option is ignored for tables
> > that have no indexes. Also, this option cannot be used in
> > conjunction with the FULL option, since FULL requires
> > rewriting the table.
>
> I would remove the "Also," in there, since it seems to me to give the
> wrong impression about those two things being similar, but they're not:
> one is convenient behavior, the other is a limitation.
Agreed.
>
> > + /* Notify user that DISABLE_INDEX_CLEANUP option is ignored */
> > + if (!vacrelstats->hasindex && (options &
> > VACOPT_DISABLE_INDEX_CLEANUP))
> > + ereport(NOTICE,
> > + (errmsg("DISABLE_INDEX_CLEANUP is ignored
> > because table \"%s\" does not have index",
> > +
> > RelationGetRelationName(onerel;
> >
> > It might make more sense to emit this in lazy_scan_heap() where we
> > determine the value of skip_index_vacuum.
>
> Why do we need a NOTICE here? I think this case should be silent.
>
Okay, removed it.
On Fri, Feb 1, 2019 at 11:43 PM Bossart, Nathan wrote:
>
> + if (skip_index_vacuum)
> + ereport(elevel,
> + (errmsg("\"%s\": marked %.0f row
> versions as dead in %u pages",
> +
> RelationGetRelationName(onerel),
> + tups_vacuumed,
> vacuumed_pages)));
>
> IIUC tups_vacuumed will include tuples removed during HOT pruning, so
> it could be inaccurate to say all of these tuples have only been
> marked "dead." Perhaps we could keep a separate count of tuples
> removed via HOT pruning in case we are using DISABLE_INDEX_CLEANUP.
> There might be similar problems with the values stored in vacrelstats
> that are used at the end of heap_vacuum_rel() (e.g. tuples_deleted).
Yeah, tups_vacuumed include such tuples so the message is inaccurate.
So I think that we should not change the message but we can report the
dead item pointers we left in errdetail. That's more accurate and
would help user.
postgres(1:1130)=# vacuum (verbose, disable_index_cleanup) test;
INFO: vacuuming "public.test"
INFO: "test": removed 49 row versions in 1 pages
INFO: "test": found 49 removable, 51 nonremovable row versions in 1
out of 1 pages
DETAIL: 0 dead row versions cannot be removed yet, oldest xmin: 509
There were 0 unused item pointers.
Skipped 0 pages due to buffer pins, 0 frozen pages.
0 pages are entirely empty.
49 tuples are left as dead.
CPU: user: 0.00 s, system: 0.00 s, elapsed: 0.00 s.
VACUUM
Attached the updated patch and the patch for vacuumdb.
Regards,
--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center
From e309d7829572648157a5d7a8363e876aaef675ef Mon Sep 17 00:00:00 2001
From: Masahiko Sawada
Date: Mon, 21 Jan 2019 19:07:44 +0900
Subject: [PATCH v6 1/2] Add DISABLE_INDEX_CLEANUP option to VACUUM command
---
doc/src/sgml/ref/vacuum.sgml | 21 ++-
src/backend/access/heap/vacuumlazy.c | 68
src/backend/commands/vacuum.c| 8 -
src/backend/parser/gram.y| 2 ++
src/include/nodes/parsenodes.h | 4 ++-
src/test/regress/expected/vacuum.out | 3 ++
src/test/regress/sql/vacuum.sql | 3 ++
7 files changed, 92 insertions(+), 17 deletions(-)
diff --git a/doc/src/sgml/ref/vacuum.sgml b/doc/src/sgml/ref/vacuum.sgml
index fd911f5..f5cde2b 100644
--- a/doc/src/sgml/ref/vacuum.sgml
+++ b/doc/src/sgml/ref/vacuum.sgml
@@ -31,6 +31,7 @@ VACUUM [ FULL ] [ FREEZE ] [ VERBOSE ] [ ANALYZE ] [ table_and_columns is:
@@ -161,7 +162,25 @@ VACUUM [ FULL ] [ FREEZE ] [ VERBOSE ] [ ANALYZE ] [ ) but not sufficient for avoiding
+ index bloat. This option is ignored if the table doesn't have index.
+ This cannot be used in conjunction with FULL
+ option.
+
+
+
+
+
SKIP_LOCKED
diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index 37aa484..9585beb 100644
--- a/s
Re: bug tracking system
On Thu, Jan 31, 2019 at 08:04:39AM -0300, Alvaro Herrera wrote: > Here's a bug tracking system that Nathan set up many years ago and > apparently has kept going unattended. It seems to me that it's > something that we could base a semi-official bug tracking system on. > > https://granicus.if.org/pgbugs/ > > There are clearly things that need fixed, such as failure to > base64-decode the body of messages, etc This is fixed now. I re-marked as stale bugs that haven't had any email for 180 days. I have also done some basic triage on some of the remaining bugs. There are a lot of bug reports that never get a response. Take bug 15513 (https://granicus.if.org/pgbugs/15513 or http://www.postgresql.org/message-id/%3c15513-94a76c56fa3ee...@postgresql.org%3E) for example. I think it's either a documentation bug (assuming incomplete documentation is considered a bug), or a feature request, or possibly a bug. I don't know enough about either the pgrowlocks extension or the internals of locking to tell the difference. So, on to how I have updated the status marked for bugs. Where a bug hadn't had any new emails for the last 180 days, I unconditionally marked the bug as stale. Where I could find a message by grepping the git logs that indicated a particular bug number was fixed, I marked the bug as fixed. If the bug appeared to be a genuine bug, but in another bit of software (usually pgadmin), I marked it as Not Our Bug. If the bug report didn't actually seem to be a bug, but either garbage (e.g. 15314), or user error, I marked them as Not a Bug. If the bug seemed to be either an actual bug or something that would have actual work done, I marked these as open. It's entirely probable that some or most of these are actually fixed. There were a number of cases where committers emailed in a "will fix" type message, but I don't have any indication that this was actually done. A few other bugs I marked as Unreproduceable or just Closed if that seemed appropriate. In general, I have tried not to make an independent judgment of the status of a bug, unless it was blindingly obvious what the status should be. It would be primarily useful if there were an easily machine readable way to associate a commit with a "this commit is intended to fix bug number X". I think the easiest way to do that is with syntax in the commit message. People generally already do this with syntax like: Author: Dmitry Dolgov Reviewed-by: Tom Lane, Arthur Zakirov Discussion: https://postg... I have used the regex /Fixes bug #([0-9]+)/ to automatically look for commits purporting to fix bugs by number. I have thought about automatically marking as Open any bug report that gets a reply email, but that's probably too broad. A lot of those replies are variations on "this is not a bug", so Open wouldn't really make sense. In any case, the system is functional again, and I have done some work in categorizing bugs. The full text search function works as well, which I have found useful on occasion. > ... and we probably want to > piggyback on our archives rather than having its own copy of the emails. I sort of do both. The pgbugs list is processed on my server via procmail and a perl script, so I have a copy of the emails, but the links for each email point back to the archives, rather than my copy. -- nw
Re: WIP: Avoid creation of the free space map for small tables
On Sun, Feb 3, 2019 at 2:06 PM Amit Kapila wrote: > > On Thu, Jan 31, 2019 at 6:03 PM Amit Kapila wrote: > This doesn't get applied cleanly after recent commit 0d1fe9f74e. > Attached is a rebased version. I have checked once that the changes > done by 0d1fe9f74e don't impact this patch. John, see if you can also > once confirm whether the recent commit (0d1fe9f74e) has any impact. I > am planning to push this tomorrow morning (IST) unless you or anyone > see any problem with this. Since that commit changes RelationAddExtraBlocks(), which can be induces by your pgbench adjustment upthread, I ran make check with that adjustment in the pgbench dir 300 times without triggering asserts. I also tried to follow the logic in 0d1fe9f74e, and I believe it will be correct without a FSM. [1] https://www.postgresql.org/message-id/CAA4eK1KRByXY03qR2JvUjUxKBzpBnCSO5H19oAC%3D_v4r5dzTwQ%40mail.gmail.com -- John Naylorhttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Synchronize with imath upstream
On Sun, Feb 03, 2019 at 07:07:36AM +, Andrew Gierth wrote:
> > "Noah" == Noah Misch writes:
>
> >> I found it much simpler to strip out -Wdeclaration-after-statement
> >> instead:
> >>
> >> $(RYU_OBJS): override CFLAGS := $(filter-out
> -Wdeclaration-after-statement,$(CFLAGS))
>
> Noah> The -Wno-declaration-after-statement approach takes eight lines
> Noah> of code, and the filter-out approach takes one. On the other
> Noah> hand, using $(filter-out) changes any runs of whitespace to
> Noah> single spaces ("$(filter-out foo,a b c)" yields "a b c"). We do
> Noah> risk that with CPPFLAGS and LDFLAGS in a few places. I don't want
> Noah> to proliferate that practice, because it changes semantics of
> Noah> CFLAGS containing -DFOO="arbitrary text".
>
> In that case I propose that we avoid the whole issue by removing
> -Wdeclaration-after-statement entirely.
>
> So far, expressed opinions have run about 4:2 in favour of allowing
> mixed declarations and code.
+1 for this.
Best,
David.
--
David Fetter http://fetter.org/
Phone: +1 415 235 3778
Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
Re: Early WIP/PoC for inlining CTEs
Vik Fearing writes: > On 28/01/2019 23:05, Tom Lane wrote: >> Peter Eisentraut writes: >>> Or put it at the end? >>> WITH ctename AS ( query ) MATERIALIZED >> Yeah, I thought about that too, but it doesn't seem like an improvement. >> If the query is very long (which isn't unlikely) I think people would >> prefer to see the option(s) up front. > On the other hand, the end is where the other options go (that we > haven't implemented yet). See . Yeah, I noticed that too while working on the latest patch revision. ISTM that's actually an argument for *not* putting PG-specific syntax there. We'd increase the risk of conflicting with future spec additions, assuming that they continue to add stuff at the end rather than just after AS. regards, tom lane
Re: Synchronize with imath upstream
Noah Misch writes:
> The -Wno-declaration-after-statement approach takes eight lines of code, and
> the filter-out approach takes one. On the other hand, using $(filter-out)
> changes any runs of whitespace to single spaces ("$(filter-out foo,ab c)"
> yields "a b c"). We do risk that with CPPFLAGS and LDFLAGS in a few places.
> I don't want to proliferate that practice, because it changes semantics of
> CFLAGS containing -DFOO="arbitrarytext".
I don't particularly buy that argument, because CPPFLAGS is where any -D
switches ought to be put. So we've already exposed ourselves to this
risk, in the unlikely scenario where it's not hypothetical.
regards, tom lane
Re: Why are we PageInit'ing buffers in RelationAddExtraBlocks()?
Hi, On 2019-02-01 07:14:11 -0800, Andres Freund wrote: > Here's a version of the patch implementing this approach. I assume this > solves the FreeBSD issue, but I'm running tests in a loop on Thomas' > machine. I pushed this, and the buildfarm so far is showing more love. There's a failure on jacana, but it looks like it's a machine issue: https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=jacana&dt=2019-02-03%2013%3A17%3A57 Feb 03 08:36:16 rm: cannot remove `/home/pgrunner/bf/root/HEAD/pgsql.build/tmp_install/home/pgrunner/bf/root/HEAD/inst/bin/postgres.exe': Permission denied And one on eelpout, which appears to be unrelated as well: https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=eelpout&dt=2019-02-03%2011%3A54%3A13 # Failed test 'intermediate client certificate is missing: matches' # at t/001_ssltests.pl line 385. # 'psql: server closed the connection unexpectedly # This probably means the server terminated abnormally # before or while processing the request. # could not send startup packet: Connection reset by peer # ' # doesn't match '(?^:SSL error)' # Looks like you failed 1 test of 71. [12:15:36] t/001_ssltests.pl .. Dubious, test returned 1 (wstat 256, 0x100) Greetings, Andres Freund
