public inbox for [email protected]help / color / mirror / Atom feed
[PATCH v12 1/4] Move callback-call from ReadPageInternal to XLogReadRecord. 8+ messages / 6 participants [nested] [flat]
* [PATCH v12 1/4] Move callback-call from ReadPageInternal to XLogReadRecord. @ 2019-09-05 11:21 Kyotaro Horiguchi <[email protected]> 0 siblings, 0 replies; 8+ messages in thread From: Kyotaro Horiguchi @ 2019-09-05 11:21 UTC (permalink / raw) The current WAL record reader reads page data using a call back function. Although it is not so problematic alone, it would be a problem if we are going to do add tasks like encryption which is performed on page data before WAL reader reads them. To avoid that the record reader facility has to have a new code path corresponds to every new callback, this patch separates page reader from WAL record reading facility by modifying the current WAL record reader to a state machine. As the first step of that change, this patch moves the page reader function out of ReadPageInternal, then the remaining tasks of the function are taken over by the new function XLogNeedData. As the result XLogPageRead directly calls the page reader callback function according to the feedback from XLogNeedData. --- src/backend/access/transam/xlog.c | 16 +- src/backend/access/transam/xlogreader.c | 272 ++++++++++-------- src/backend/access/transam/xlogutils.c | 12 +- .../replication/logical/logicalfuncs.c | 2 +- src/backend/replication/walsender.c | 10 +- src/bin/pg_rewind/parsexlog.c | 16 +- src/bin/pg_waldump/pg_waldump.c | 8 +- src/include/access/xlogreader.h | 23 +- src/include/access/xlogutils.h | 2 +- src/include/replication/logicalfuncs.h | 2 +- 10 files changed, 213 insertions(+), 150 deletions(-) diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index 5f0ee50092..4a6bd6e002 100644 --- a/src/backend/access/transam/xlog.c +++ b/src/backend/access/transam/xlog.c @@ -884,7 +884,7 @@ static bool InstallXLogFileSegment(XLogSegNo *segno, char *tmppath, static int XLogFileRead(XLogSegNo segno, int emode, TimeLineID tli, int source, bool notfoundOk); static int XLogFileReadAnyTLI(XLogSegNo segno, int emode, int source); -static int XLogPageRead(XLogReaderState *xlogreader, XLogRecPtr targetPagePtr, +static bool XLogPageRead(XLogReaderState *xlogreader, XLogRecPtr targetPagePtr, int reqLen, XLogRecPtr targetRecPtr, char *readBuf); static bool WaitForWALToBecomeAvailable(XLogRecPtr RecPtr, bool randAccess, bool fetching_ckpt, XLogRecPtr tliRecPtr); @@ -4249,7 +4249,6 @@ ReadRecord(XLogReaderState *xlogreader, XLogRecPtr RecPtr, int emode, XLogRecord *record; XLogPageReadPrivate *private = (XLogPageReadPrivate *) xlogreader->private_data; - /* Pass through parameters to XLogPageRead */ private->fetching_ckpt = fetching_ckpt; private->emode = emode; private->randAccess = (RecPtr != InvalidXLogRecPtr); @@ -11537,7 +11536,7 @@ CancelBackup(void) * XLogPageRead() to try fetching the record from another source, or to * sleep and retry. */ -static int +static bool XLogPageRead(XLogReaderState *xlogreader, XLogRecPtr targetPagePtr, int reqLen, XLogRecPtr targetRecPtr, char *readBuf) { @@ -11596,7 +11595,8 @@ retry: readLen = 0; readSource = 0; - return -1; + xlogreader->readLen = -1; + return false; } } @@ -11691,7 +11691,8 @@ retry: goto next_record_is_invalid; } - return readLen; + xlogreader->readLen = readLen; + return true; next_record_is_invalid: lastSourceFailed = true; @@ -11705,8 +11706,9 @@ next_record_is_invalid: /* In standby-mode, keep trying */ if (StandbyMode) goto retry; - else - return -1; + + xlogreader->readLen = -1; + return false; } /* diff --git a/src/backend/access/transam/xlogreader.c b/src/backend/access/transam/xlogreader.c index 67418b05f1..063223701e 100644 --- a/src/backend/access/transam/xlogreader.c +++ b/src/backend/access/transam/xlogreader.c @@ -36,8 +36,8 @@ static void report_invalid_record(XLogReaderState *state, const char *fmt,...) pg_attribute_printf(2, 3); static bool allocate_recordbuf(XLogReaderState *state, uint32 reclength); -static int ReadPageInternal(XLogReaderState *state, XLogRecPtr pageptr, - int reqLen); +static bool XLogNeedData(XLogReaderState *state, XLogRecPtr pageptr, + int reqLen, bool header_inclusive); static void XLogReaderInvalReadState(XLogReaderState *state); static bool ValidXLogRecordHeader(XLogReaderState *state, XLogRecPtr RecPtr, XLogRecPtr PrevRecPtr, XLogRecord *record, bool randAccess); @@ -245,7 +245,6 @@ XLogReadRecord(XLogReaderState *state, XLogRecPtr RecPtr, char **errormsg) uint32 targetRecOff; uint32 pageHeaderSize; bool gotheader; - int readOff; /* * randAccess indicates whether to verify the previous-record pointer of @@ -297,14 +296,20 @@ XLogReadRecord(XLogReaderState *state, XLogRecPtr RecPtr, char **errormsg) * byte to cover the whole record header, or at least the part of it that * fits on the same page. */ - readOff = ReadPageInternal(state, targetPagePtr, - Min(targetRecOff + SizeOfXLogRecord, XLOG_BLCKSZ)); - if (readOff < 0) + while (XLogNeedData(state, targetPagePtr, + Min(targetRecOff + SizeOfXLogRecord, XLOG_BLCKSZ), + targetRecOff != 0)) + { + if (!state->read_page(state, state->readPagePtr, state->readLen, + RecPtr, state->readBuf)) + break; + } + + if (!state->page_verified) goto err; /* - * ReadPageInternal always returns at least the page header, so we can - * examine it now. + * We have at least the page header, so we can examine it now. */ pageHeaderSize = XLogPageHeaderSize((XLogPageHeader) state->readBuf); if (targetRecOff == 0) @@ -330,8 +335,8 @@ XLogReadRecord(XLogReaderState *state, XLogRecPtr RecPtr, char **errormsg) goto err; } - /* ReadPageInternal has verified the page header */ - Assert(pageHeaderSize <= readOff); + /* XLogNeedData has verified the page header */ + Assert(pageHeaderSize <= state->readLen); /* * Read the record length. @@ -404,18 +409,25 @@ XLogReadRecord(XLogReaderState *state, XLogRecPtr RecPtr, char **errormsg) do { + int rest_len = total_len - gotlen; + /* Calculate pointer to beginning of next page */ targetPagePtr += XLOG_BLCKSZ; /* Wait for the next page to become available */ - readOff = ReadPageInternal(state, targetPagePtr, - Min(total_len - gotlen + SizeOfXLogShortPHD, - XLOG_BLCKSZ)); + while (XLogNeedData(state, targetPagePtr, + Min(rest_len, XLOG_BLCKSZ), + false)) + { + if (!state->read_page(state, state->readPagePtr, state->readLen, + state->ReadRecPtr, state->readBuf)) + break; + } - if (readOff < 0) + if (!state->page_verified) goto err; - Assert(SizeOfXLogShortPHD <= readOff); + Assert(SizeOfXLogShortPHD <= state->readLen); /* Check that the continuation on next page looks valid */ pageHeader = (XLogPageHeader) state->readBuf; @@ -444,21 +456,14 @@ XLogReadRecord(XLogReaderState *state, XLogRecPtr RecPtr, char **errormsg) /* Append the continuation from this page to the buffer */ pageHeaderSize = XLogPageHeaderSize(pageHeader); - if (readOff < pageHeaderSize) - readOff = ReadPageInternal(state, targetPagePtr, - pageHeaderSize); - - Assert(pageHeaderSize <= readOff); + Assert(pageHeaderSize <= state->readLen); contdata = (char *) state->readBuf + pageHeaderSize; len = XLOG_BLCKSZ - pageHeaderSize; if (pageHeader->xlp_rem_len < len) len = pageHeader->xlp_rem_len; - if (readOff < pageHeaderSize + len) - readOff = ReadPageInternal(state, targetPagePtr, - pageHeaderSize + len); - + Assert (pageHeaderSize + len <= state->readLen); memcpy(buffer, (char *) contdata, len); buffer += len; gotlen += len; @@ -488,9 +493,15 @@ XLogReadRecord(XLogReaderState *state, XLogRecPtr RecPtr, char **errormsg) else { /* Wait for the record data to become available */ - readOff = ReadPageInternal(state, targetPagePtr, - Min(targetRecOff + total_len, XLOG_BLCKSZ)); - if (readOff < 0) + while (XLogNeedData(state, targetPagePtr, + Min(targetRecOff + total_len, XLOG_BLCKSZ), true)) + { + if (!state->read_page(state, state->readPagePtr, state->readLen, + state->ReadRecPtr, state->readBuf)) + break; + } + + if (!state->page_verified) goto err; /* Record does not cross a page boundary */ @@ -533,109 +544,139 @@ err: } /* - * Read a single xlog page including at least [pageptr, reqLen] of valid data - * via the read_page() callback. + * Checks that an xlog page loaded in state->readBuf is including at least + * [pageptr, reqLen] and the page is valid. header_inclusive indicates that + * reqLen is calculated including page header length. * - * Returns -1 if the required page cannot be read for some reason; errormsg_buf - * is set in that case (unless the error occurs in the read_page callback). + * Returns false if the buffer already contains the requested data, or found + * error. state->page_verified is set to true for the former and false for the + * latter. * - * We fetch the page from a reader-local cache if we know we have the required - * data and if there hasn't been any error since caching the data. + * Otherwise returns true and requests data loaded onto state->readBuf by + * state->readPagePtr and state->readLen. The caller shall call this function + * again after filling the buffer at least with that portion of data and set + * state->readLen to the length of actually loaded data. + * + * If header_inclusive is false, corrects reqLen internally by adding the + * actual page header length and may request caller for new data. */ -static int -ReadPageInternal(XLogReaderState *state, XLogRecPtr pageptr, int reqLen) +static bool +XLogNeedData(XLogReaderState *state, XLogRecPtr pageptr, int reqLen, + bool header_inclusive) { - int readLen; uint32 targetPageOff; XLogSegNo targetSegNo; - XLogPageHeader hdr; + uint32 addLen = 0; - Assert((pageptr % XLOG_BLCKSZ) == 0); + /* Some data is loaded, but page header is not verified yet. */ + if (!state->page_verified && + !XLogRecPtrIsInvalid(state->readPagePtr) && state->readLen >= 0) + { + uint32 pageHeaderSize; - XLByteToSeg(pageptr, targetSegNo, state->segcxt.ws_segsize); - targetPageOff = XLogSegmentOffset(pageptr, state->segcxt.ws_segsize); + /* just loaded new data so needs to verify page header */ - /* check whether we have all the requested data already */ - if (targetSegNo == state->seg.ws_segno && - targetPageOff == state->segoff && reqLen <= state->readLen) - return state->readLen; + /* The caller must have loaded at least page header */ + Assert (state->readLen >= SizeOfXLogShortPHD); - /* - * Data is not in our buffer. - * - * Every time we actually read the page, even if we looked at parts of it - * before, we need to do verification as the read_page callback might now - * be rereading data from a different source. - * - * Whenever switching to a new WAL segment, we read the first page of the - * file and validate its header, even if that's not where the target - * record is. This is so that we can check the additional identification - * info that is present in the first page's "long" header. - */ - if (targetSegNo != state->seg.ws_segno && targetPageOff != 0) - { - XLogRecPtr targetSegmentPtr = pageptr - targetPageOff; + /* + * We have enough data to check the header length. Recheck the loaded + * length against the actual header length. + */ + pageHeaderSize = XLogPageHeaderSize((XLogPageHeader) state->readBuf); - readLen = state->read_page(state, targetSegmentPtr, XLOG_BLCKSZ, - state->currRecPtr, - state->readBuf); - if (readLen < 0) - goto err; + /* Request more data if we don't have the full header. */ + if (state->readLen < pageHeaderSize) + { + state->readLen = pageHeaderSize; + return true; + } + + /* Now that we know we have the full header, validate it. */ + if (!XLogReaderValidatePageHeader(state, state->readPagePtr, + (char *) state->readBuf)) + { + /* That's bad. Force reading the page again. */ + XLogReaderInvalReadState(state); - /* we can be sure to have enough WAL available, we scrolled back */ - Assert(readLen == XLOG_BLCKSZ); + return false; + } - if (!XLogReaderValidatePageHeader(state, targetSegmentPtr, - state->readBuf)) - goto err; + state->page_verified = true; + + XLByteToSeg(state->readPagePtr, state->seg.ws_segno, + state->segcxt.ws_segsize); } /* - * First, read the requested data length, but at least a short page header - * so that we can validate it. + * The loaded page may not be the one caller is supposing to read when we + * are verifying the first page of new segment. In that case, skip further + * verification and immediately load the target page. */ - readLen = state->read_page(state, pageptr, Max(reqLen, SizeOfXLogShortPHD), - state->currRecPtr, - state->readBuf); - if (readLen < 0) - goto err; - - Assert(readLen <= XLOG_BLCKSZ); + if (state->page_verified && pageptr == state->readPagePtr) + { - /* Do we have enough data to check the header length? */ - if (readLen <= SizeOfXLogShortPHD) - goto err; + /* + * calculate additional length for page header keeping the total + * length within the block size. + */ + if (!header_inclusive) + { + uint32 pageHeaderSize = + XLogPageHeaderSize((XLogPageHeader) state->readBuf); - Assert(readLen >= reqLen); + addLen = pageHeaderSize; + if (reqLen + pageHeaderSize <= XLOG_BLCKSZ) + addLen = pageHeaderSize; + else + addLen = XLOG_BLCKSZ - reqLen; - hdr = (XLogPageHeader) state->readBuf; + Assert(addLen >= 0); + } - /* still not enough */ - if (readLen < XLogPageHeaderSize(hdr)) - { - readLen = state->read_page(state, pageptr, XLogPageHeaderSize(hdr), - state->currRecPtr, - state->readBuf); - if (readLen < 0) - goto err; + /* Return if we already have it. */ + if (reqLen + addLen <= state->readLen) + return false; } + /* Data is not in our buffer, request the caller for it. */ + XLByteToSeg(pageptr, targetSegNo, state->segcxt.ws_segsize); + targetPageOff = XLogSegmentOffset(pageptr, state->segcxt.ws_segsize); + Assert((pageptr % XLOG_BLCKSZ) == 0); + /* - * Now that we know we have the full header, validate it. + * Every time we request to load new data of a page to the caller, even if + * we looked at a part of it before, we need to do verification on the next + * invocation as the caller might now be rereading data from a different + * source. */ - if (!XLogReaderValidatePageHeader(state, pageptr, (char *) hdr)) - goto err; - - /* update read state information */ - state->seg.ws_segno = targetSegNo; - state->segoff = targetPageOff; - state->readLen = readLen; + state->page_verified = false; - return readLen; + /* + * Whenever switching to a new WAL segment, we read the first page of the + * file and validate its header, even if that's not where the target + * record is. This is so that we can check the additional identification + * info that is present in the first page's "long" header. + * Don't do this if the caller requested the first page in the segment. + */ + if (targetSegNo != state->seg.ws_segno && targetPageOff != 0) + { + /* + * Then we'll see that the targetSegNo now matches the ws_segno, and + * will not come back here, but will request the actual target page. + */ + state->readPagePtr = pageptr - targetPageOff; + state->readLen = XLOG_BLCKSZ; + return true; + } -err: - XLogReaderInvalReadState(state); - return -1; + /* + * Request the caller to load the page. We need at least a short page + * header so that we can validate it. + */ + state->readPagePtr = pageptr; + state->readLen = Max(reqLen + addLen, SizeOfXLogShortPHD); + return true; } /* @@ -644,9 +685,7 @@ err: static void XLogReaderInvalReadState(XLogReaderState *state) { - state->seg.ws_segno = 0; - state->segoff = 0; - state->readLen = 0; + state->readPagePtr = InvalidXLogRecPtr; } /* @@ -924,7 +963,6 @@ XLogFindNextRecord(XLogReaderState *state, XLogRecPtr RecPtr) XLogRecPtr targetPagePtr; int targetRecOff; uint32 pageHeaderSize; - int readLen; /* * Compute targetRecOff. It should typically be equal or greater than @@ -932,7 +970,7 @@ XLogFindNextRecord(XLogReaderState *state, XLogRecPtr RecPtr) * that, except when caller has explicitly specified the offset that * falls somewhere there or when we are skipping multi-page * continuation record. It doesn't matter though because - * ReadPageInternal() is prepared to handle that and will read at + * XLogNeedData() is prepared to handle that and will read at * least short page-header worth of data */ targetRecOff = tmpRecPtr % XLOG_BLCKSZ; @@ -940,19 +978,23 @@ XLogFindNextRecord(XLogReaderState *state, XLogRecPtr RecPtr) /* scroll back to page boundary */ targetPagePtr = tmpRecPtr - targetRecOff; - /* Read the page containing the record */ - readLen = ReadPageInternal(state, targetPagePtr, targetRecOff); - if (readLen < 0) + while(XLogNeedData(state, targetPagePtr, targetRecOff, + targetRecOff != 0)) + { + if (!state->read_page(state, state->readPagePtr, state->readLen, + state->ReadRecPtr, state->readBuf)) + break; + } + + if (!state->page_verified) goto err; header = (XLogPageHeader) state->readBuf; pageHeaderSize = XLogPageHeaderSize(header); - /* make sure we have enough data for the page header */ - readLen = ReadPageInternal(state, targetPagePtr, pageHeaderSize); - if (readLen < 0) - goto err; + /* we should have read the page header */ + Assert (state->readLen >= pageHeaderSize); /* skip over potential continuation data */ if (header->xlp_info & XLP_FIRST_IS_CONTRECORD) diff --git a/src/backend/access/transam/xlogutils.c b/src/backend/access/transam/xlogutils.c index 446760ed6e..060fbc0c4c 100644 --- a/src/backend/access/transam/xlogutils.c +++ b/src/backend/access/transam/xlogutils.c @@ -680,8 +680,8 @@ XLogTruncateRelation(RelFileNode rnode, ForkNumber forkNum, void XLogReadDetermineTimeline(XLogReaderState *state, XLogRecPtr wantPage, uint32 wantLength) { - const XLogRecPtr lastReadPage = (state->seg.ws_segno * - state->segcxt.ws_segsize + state->segoff); + const XLogRecPtr lastReadPage = state->seg.ws_segno * + state->segcxt.ws_segsize + state->readLen; Assert(wantPage != InvalidXLogRecPtr && wantPage % XLOG_BLCKSZ == 0); Assert(wantLength <= XLOG_BLCKSZ); @@ -813,7 +813,7 @@ wal_segment_open(XLogSegNo nextSegNo, WALSegmentContext *segcxt, * exists for normal backends, so we have to do a check/sleep/repeat style of * loop for now. */ -int +bool read_local_xlog_page(XLogReaderState *state, XLogRecPtr targetPagePtr, int reqLen, XLogRecPtr targetRecPtr, char *cur_page) { @@ -915,7 +915,8 @@ read_local_xlog_page(XLogReaderState *state, XLogRecPtr targetPagePtr, else if (targetPagePtr + reqLen > read_upto) { /* not enough data there */ - return -1; + state->readLen = -1; + return false; } else { @@ -933,7 +934,8 @@ read_local_xlog_page(XLogReaderState *state, XLogRecPtr targetPagePtr, WALReadRaiseError(&errinfo); /* number of valid bytes in the buffer */ - return count; + state->readLen = count; + return true; } /* diff --git a/src/backend/replication/logical/logicalfuncs.c b/src/backend/replication/logical/logicalfuncs.c index aa2106b152..4b0e8ea773 100644 --- a/src/backend/replication/logical/logicalfuncs.c +++ b/src/backend/replication/logical/logicalfuncs.c @@ -106,7 +106,7 @@ check_permissions(void) (errmsg("must be superuser or replication role to use replication slots")))); } -int +bool logical_read_local_xlog_page(XLogReaderState *state, XLogRecPtr targetPagePtr, int reqLen, XLogRecPtr targetRecPtr, char *cur_page) { diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c index 8bafa65e50..554c186ae1 100644 --- a/src/backend/replication/walsender.c +++ b/src/backend/replication/walsender.c @@ -762,7 +762,7 @@ StartReplication(StartReplicationCmd *cmd) * which has to do a plain sleep/busy loop, because the walsender's latch gets * set every time WAL is flushed. */ -static int +static bool logical_read_xlog_page(XLogReaderState *state, XLogRecPtr targetPagePtr, int reqLen, XLogRecPtr targetRecPtr, char *cur_page) { @@ -782,7 +782,10 @@ logical_read_xlog_page(XLogReaderState *state, XLogRecPtr targetPagePtr, int req /* fail if not (implies we are going to shut down) */ if (flushptr < targetPagePtr + reqLen) - return -1; + { + state->readLen = -1; + return false; + } if (targetPagePtr + XLOG_BLCKSZ <= flushptr) count = XLOG_BLCKSZ; /* more than one block available */ @@ -812,7 +815,8 @@ logical_read_xlog_page(XLogReaderState *state, XLogRecPtr targetPagePtr, int req XLByteToSeg(targetPagePtr, segno, sendCxt->ws_segsize); CheckXLogRemoved(segno, sendSeg->ws_tli); - return count; + state->readLen = count; + return true; } /* diff --git a/src/bin/pg_rewind/parsexlog.c b/src/bin/pg_rewind/parsexlog.c index 1d03375a3e..795e84ff9f 100644 --- a/src/bin/pg_rewind/parsexlog.c +++ b/src/bin/pg_rewind/parsexlog.c @@ -44,7 +44,7 @@ typedef struct XLogPageReadPrivate int tliIndex; } XLogPageReadPrivate; -static int SimpleXLogPageRead(XLogReaderState *xlogreader, +static bool SimpleXLogPageRead(XLogReaderState *xlogreader, XLogRecPtr targetPagePtr, int reqLen, XLogRecPtr targetRecPtr, char *readBuf); @@ -228,7 +228,7 @@ findLastCheckpoint(const char *datadir, XLogRecPtr forkptr, int tliIndex, } /* XLogReader callback function, to read a WAL page */ -static int +static bool SimpleXLogPageRead(XLogReaderState *xlogreader, XLogRecPtr targetPagePtr, int reqLen, XLogRecPtr targetRecPtr, char *readBuf) { @@ -283,7 +283,8 @@ SimpleXLogPageRead(XLogReaderState *xlogreader, XLogRecPtr targetPagePtr, if (xlogreadfd < 0) { pg_log_error("could not open file \"%s\": %m", xlogfpath); - return -1; + xlogreader->readLen = -1; + return false; } } @@ -296,7 +297,8 @@ SimpleXLogPageRead(XLogReaderState *xlogreader, XLogRecPtr targetPagePtr, if (lseek(xlogreadfd, (off_t) targetPageOff, SEEK_SET) < 0) { pg_log_error("could not seek in file \"%s\": %m", xlogfpath); - return -1; + xlogreader->readLen = -1; + return false; } @@ -309,13 +311,15 @@ SimpleXLogPageRead(XLogReaderState *xlogreader, XLogRecPtr targetPagePtr, pg_log_error("could not read file \"%s\": read %d of %zu", xlogfpath, r, (Size) XLOG_BLCKSZ); - return -1; + xlogreader->readLen = -1; + return false; } Assert(targetSegNo == xlogreadsegno); xlogreader->seg.ws_tli = targetHistory[private->tliIndex].tli; - return XLOG_BLCKSZ; + xlogreader->readLen = XLOG_BLCKSZ; + return true; } /* diff --git a/src/bin/pg_waldump/pg_waldump.c b/src/bin/pg_waldump/pg_waldump.c index 30a5851d87..df13cf3173 100644 --- a/src/bin/pg_waldump/pg_waldump.c +++ b/src/bin/pg_waldump/pg_waldump.c @@ -324,7 +324,7 @@ WALDumpOpenSegment(XLogSegNo nextSegNo, WALSegmentContext *segcxt, /* * XLogReader read_page callback */ -static int +static bool WALDumpReadPage(XLogReaderState *state, XLogRecPtr targetPagePtr, int reqLen, XLogRecPtr targetPtr, char *readBuff) { @@ -341,7 +341,8 @@ WALDumpReadPage(XLogReaderState *state, XLogRecPtr targetPagePtr, int reqLen, else { private->endptr_reached = true; - return -1; + state->readLen = -1; + return false; } } @@ -366,7 +367,8 @@ WALDumpReadPage(XLogReaderState *state, XLogRecPtr targetPagePtr, int reqLen, (Size) errinfo.wre_req); } - return count; + state->readLen = count; + return true; } /* diff --git a/src/include/access/xlogreader.h b/src/include/access/xlogreader.h index 0193611b7f..d990432ffe 100644 --- a/src/include/access/xlogreader.h +++ b/src/include/access/xlogreader.h @@ -49,7 +49,7 @@ typedef struct WALSegmentContext typedef struct XLogReaderState XLogReaderState; /* Function type definition for the read_page callback */ -typedef int (*XLogPageReadCB) (XLogReaderState *xlogreader, +typedef bool (*XLogPageReadCB) (XLogReaderState *xlogreader, XLogRecPtr targetPagePtr, int reqLen, XLogRecPtr targetRecPtr, @@ -131,6 +131,20 @@ struct XLogReaderState XLogRecPtr ReadRecPtr; /* start of last record read */ XLogRecPtr EndRecPtr; /* end+1 of last record read */ + /* ---------------------------------------- + * Communication with page reader + * readBuf is XLOG_BLCKSZ bytes, valid up to at least readLen bytes. + * ---------------------------------------- + */ + /* variables to communicate with page reader */ + XLogRecPtr readPagePtr; /* page pointer to read */ + int32 readLen; /* bytes requested to reader, or actual bytes + * read by reader, which must be larger than + * the request, or -1 on error */ + TimeLineID readPageTLI; /* TLI for data currently in readBuf */ + char *readBuf; /* buffer to store data */ + bool page_verified; /* is the page on the buffer verified? */ + /* ---------------------------------------- * Decoded representation of current record @@ -157,13 +171,6 @@ struct XLogReaderState * ---------------------------------------- */ - /* - * Buffer for currently read page (XLOG_BLCKSZ bytes, valid up to at least - * readLen bytes) - */ - char *readBuf; - uint32 readLen; - /* last read XLOG position for data currently in readBuf */ WALSegmentContext segcxt; WALOpenSegment seg; diff --git a/src/include/access/xlogutils.h b/src/include/access/xlogutils.h index 0572b24192..0867ef0599 100644 --- a/src/include/access/xlogutils.h +++ b/src/include/access/xlogutils.h @@ -47,7 +47,7 @@ extern Buffer XLogReadBufferExtended(RelFileNode rnode, ForkNumber forknum, extern Relation CreateFakeRelcacheEntry(RelFileNode rnode); extern void FreeFakeRelcacheEntry(Relation fakerel); -extern int read_local_xlog_page(XLogReaderState *state, +extern bool read_local_xlog_page(XLogReaderState *state, XLogRecPtr targetPagePtr, int reqLen, XLogRecPtr targetRecPtr, char *cur_page); diff --git a/src/include/replication/logicalfuncs.h b/src/include/replication/logicalfuncs.h index 012096f183..54291221c3 100644 --- a/src/include/replication/logicalfuncs.h +++ b/src/include/replication/logicalfuncs.h @@ -11,7 +11,7 @@ #include "replication/logical.h" -extern int logical_read_local_xlog_page(XLogReaderState *state, +extern bool logical_read_local_xlog_page(XLogReaderState *state, XLogRecPtr targetPagePtr, int reqLen, XLogRecPtr targetRecPtr, char *cur_page); -- 2.23.0 ----Next_Part(Fri_Nov_29_17_14_21_2019_330)-- Content-Type: Text/X-Patch; charset=us-ascii Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="v12-0002-Move-page-reader-out-of-XLogReadRecord.patch" ^ permalink raw reply [nested|flat] 8+ messages in thread
* Use more CppAsString2() in pg_amcheck.c @ 2024-10-18 09:50 Michael Paquier <[email protected]> 0 siblings, 3 replies; 8+ messages in thread From: Michael Paquier @ 2024-10-18 09:50 UTC (permalink / raw) To: Postgres hackers <[email protected]> Hi all, pg_amcheck.c is one of these places where relkind and relpersistence values are hardcoded in queries based on the contents of pg_class_d.h. Similarly to other places in src/bin/, let's sprinkle some CppAsString2() and feed to the binary the values from the header. The patch attached does that. Thoughts or comments? -- Michael Attachments: [text/x-diff] 0001-Use-more-CppAsString2-in-pg_amcheck.c.patch (5.7K, ../../[email protected]/2-0001-Use-more-CppAsString2-in-pg_amcheck.c.patch) download | inline diff: From 9f56c8befb5b59b6c8013c3f7d58d62e5e42c30a Mon Sep 17 00:00:00 2001 From: Michael Paquier <[email protected]> Date: Fri, 18 Oct 2024 18:49:45 +0900 Subject: [PATCH] Use more CppAsString2() in pg_amcheck.c --- src/bin/pg_amcheck/pg_amcheck.c | 42 +++++++++++++++++++++++---------- 1 file changed, 29 insertions(+), 13 deletions(-) diff --git a/src/bin/pg_amcheck/pg_amcheck.c b/src/bin/pg_amcheck/pg_amcheck.c index 0c05cf58bc..e74a0af1f8 100644 --- a/src/bin/pg_amcheck/pg_amcheck.c +++ b/src/bin/pg_amcheck/pg_amcheck.c @@ -16,6 +16,7 @@ #include <time.h> #include "catalog/pg_am_d.h" +#include "catalog/pg_class_d.h" #include "catalog/pg_namespace_d.h" #include "common/logging.h" #include "common/username.h" @@ -857,7 +858,7 @@ prepare_heap_command(PQExpBuffer sql, RelationInfo *rel, PGconn *conn) appendPQExpBuffer(sql, "\n) v WHERE c.oid = %u " - "AND c.relpersistence != 't'", + "AND c.relpersistence != " CppAsString2(RELPERSISTENCE_TEMP), rel->reloid); } @@ -890,7 +891,7 @@ prepare_btree_command(PQExpBuffer sql, RelationInfo *rel, PGconn *conn) "\nFROM pg_catalog.pg_class c, pg_catalog.pg_index i " "WHERE c.oid = %u " "AND c.oid = i.indexrelid " - "AND c.relpersistence != 't' " + "AND c.relpersistence != " CppAsString2(RELPERSISTENCE_TEMP) " " "AND i.indisready AND i.indisvalid AND i.indislive", rel->datinfo->amcheck_schema, (opts.heapallindexed ? "true" : "false"), @@ -905,7 +906,7 @@ prepare_btree_command(PQExpBuffer sql, RelationInfo *rel, PGconn *conn) "\nFROM pg_catalog.pg_class c, pg_catalog.pg_index i " "WHERE c.oid = %u " "AND c.oid = i.indexrelid " - "AND c.relpersistence != 't' " + "AND c.relpersistence != " CppAsString2(RELPERSISTENCE_TEMP) " " "AND i.indisready AND i.indisvalid AND i.indislive", rel->datinfo->amcheck_schema, (opts.heapallindexed ? "true" : "false"), @@ -1952,7 +1953,8 @@ compile_relation_list_one_db(PGconn *conn, SimplePtrList *relations, * until firing off the amcheck command, as the state of an index may * change by then. */ - appendPQExpBufferStr(&sql, "\nWHERE c.relpersistence != 't'"); + appendPQExpBufferStr(&sql, "\nWHERE c.relpersistence != " + CppAsString2(RELPERSISTENCE_TEMP) " "); if (opts.excludetbl || opts.excludeidx || opts.excludensp) appendPQExpBufferStr(&sql, "\nAND ep.pattern_id IS NULL"); @@ -1972,15 +1974,29 @@ compile_relation_list_one_db(PGconn *conn, SimplePtrList *relations, if (opts.allrel) appendPQExpBuffer(&sql, " AND c.relam = %u " - "AND c.relkind IN ('r', 'S', 'm', 't') " + "AND c.relkind IN (" + CppAsString2(RELKIND_RELATION) ", " + CppAsString2(RELKIND_SEQUENCE) ", " + CppAsString2(RELKIND_MATVIEW) ", " + CppAsString2(RELKIND_TOASTVALUE) ") " "AND c.relnamespace != %u", HEAP_TABLE_AM_OID, PG_TOAST_NAMESPACE); else appendPQExpBuffer(&sql, " AND c.relam IN (%u, %u)" - "AND c.relkind IN ('r', 'S', 'm', 't', 'i') " - "AND ((c.relam = %u AND c.relkind IN ('r', 'S', 'm', 't')) OR " - "(c.relam = %u AND c.relkind = 'i'))", + "AND c.relkind IN (" + CppAsString2(RELKIND_RELATION) ", " + CppAsString2(RELKIND_SEQUENCE) ", " + CppAsString2(RELKIND_MATVIEW) ", " + CppAsString2(RELKIND_TOASTVALUE) ", " + CppAsString2(RELKIND_INDEX) ") " + "AND ((c.relam = %u AND c.relkind IN (" + CppAsString2(RELKIND_RELATION) ", " + CppAsString2(RELKIND_SEQUENCE) ", " + CppAsString2(RELKIND_MATVIEW) ", " + CppAsString2(RELKIND_TOASTVALUE) ")) OR " + "(c.relam = %u AND c.relkind = " + CppAsString2(RELKIND_INDEX) "))", HEAP_TABLE_AM_OID, BTREE_AM_OID, HEAP_TABLE_AM_OID, BTREE_AM_OID); @@ -2007,7 +2023,7 @@ compile_relation_list_one_db(PGconn *conn, SimplePtrList *relations, "\nAND (t.relname ~ ep.rel_regex OR ep.rel_regex IS NULL)" "\nAND ep.heap_only" "\nWHERE ep.pattern_id IS NULL" - "\nAND t.relpersistence != 't'"); + "\nAND t.relpersistence != " CppAsString2(RELPERSISTENCE_TEMP)); appendPQExpBufferStr(&sql, "\n)"); } @@ -2026,7 +2042,7 @@ compile_relation_list_one_db(PGconn *conn, SimplePtrList *relations, "ON r.oid = i.indrelid " "INNER JOIN pg_catalog.pg_class c " "ON i.indexrelid = c.oid " - "AND c.relpersistence != 't'"); + "AND c.relpersistence != " CppAsString2(RELPERSISTENCE_TEMP)); if (opts.excludeidx || opts.excludensp) appendPQExpBufferStr(&sql, "\nINNER JOIN pg_catalog.pg_namespace n " @@ -2041,7 +2057,7 @@ compile_relation_list_one_db(PGconn *conn, SimplePtrList *relations, "\nWHERE true"); appendPQExpBuffer(&sql, " AND c.relam = %u " - "AND c.relkind = 'i'", + "AND c.relkind = " CppAsString2(RELKIND_INDEX), BTREE_AM_OID); if (opts.no_toast_expansion) appendPQExpBuffer(&sql, @@ -2065,7 +2081,7 @@ compile_relation_list_one_db(PGconn *conn, SimplePtrList *relations, "ON t.oid = i.indrelid" "\nINNER JOIN pg_catalog.pg_class c " "ON i.indexrelid = c.oid " - "AND c.relpersistence != 't'"); + "AND c.relpersistence != " CppAsString2(RELPERSISTENCE_TEMP)); if (opts.excludeidx) appendPQExpBufferStr(&sql, "\nLEFT OUTER JOIN exclude_pat ep " @@ -2078,7 +2094,7 @@ compile_relation_list_one_db(PGconn *conn, SimplePtrList *relations, "\nWHERE true"); appendPQExpBuffer(&sql, " AND c.relam = %u" - " AND c.relkind = 'i')", + " AND c.relkind = " CppAsString2(RELKIND_INDEX) ")", BTREE_AM_OID); } -- 2.45.2 [application/pgp-signature] signature.asc (833B, ../../[email protected]/3-signature.asc) download ^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Use more CppAsString2() in pg_amcheck.c @ 2024-10-18 09:59 Daniel Gustafsson <[email protected]> parent: Michael Paquier <[email protected]> 2 siblings, 0 replies; 8+ messages in thread From: Daniel Gustafsson @ 2024-10-18 09:59 UTC (permalink / raw) To: Michael Paquier <[email protected]>; +Cc: Postgres hackers <[email protected]> > On 18 Oct 2024, at 11:50, Michael Paquier <[email protected]> wrote: > pg_amcheck.c is one of these places where relkind and relpersistence > values are hardcoded in queries based on the contents of pg_class_d.h. > Similarly to other places in src/bin/, let's sprinkle some > CppAsString2() and feed to the binary the values from the header. The > patch attached does that. I haven't studied the patch in detail (yet), nothing stood out from a quick skim, but +1 on the concept. -- Daniel Gustafsson ^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Use more CppAsString2() in pg_amcheck.c @ 2024-10-18 13:18 Alvaro Herrera <[email protected]> parent: Michael Paquier <[email protected]> 2 siblings, 0 replies; 8+ messages in thread From: Alvaro Herrera @ 2024-10-18 13:18 UTC (permalink / raw) To: Michael Paquier <[email protected]>; +Cc: Postgres hackers <[email protected]> On 2024-Oct-18, Michael Paquier wrote: > pg_amcheck.c is one of these places where relkind and relpersistence > values are hardcoded in queries based on the contents of pg_class_d.h. > Similarly to other places in src/bin/, let's sprinkle some > CppAsString2() and feed to the binary the values from the header. The > patch attached does that. Hmm, aren't you losing the single quotes? I think you would need to do > @@ -857,7 +858,7 @@ prepare_heap_command(PQExpBuffer sql, RelationInfo *rel, PGconn *conn) > > appendPQExpBuffer(sql, > "\n) v WHERE c.oid = %u " > - "AND c.relpersistence != 't'", > + "AND c.relpersistence != '%s'", > rel->reloid, CppAsString2(RELPERSISTENCE_TEMP)); in order for this to work properly, no? -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ "La vida es para el que se aventura" ^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Use more CppAsString2() in pg_amcheck.c @ 2024-10-18 17:22 Nathan Bossart <[email protected]> parent: Michael Paquier <[email protected]> 2 siblings, 1 reply; 8+ messages in thread From: Nathan Bossart @ 2024-10-18 17:22 UTC (permalink / raw) To: Michael Paquier <[email protected]>; +Cc: Postgres hackers <[email protected]> On Fri, Oct 18, 2024 at 06:50:50PM +0900, Michael Paquier wrote: > - appendPQExpBufferStr(&sql, "\nWHERE c.relpersistence != 't'"); > + appendPQExpBufferStr(&sql, "\nWHERE c.relpersistence != " > + CppAsString2(RELPERSISTENCE_TEMP) " "); I think these whitespace changes may cause invalid statements to be produced in some code paths. IMHO those should be reserved for a separate patch, anyway. -- nathan ^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Use more CppAsString2() in pg_amcheck.c @ 2024-10-19 01:16 Michael Paquier <[email protected]> parent: Nathan Bossart <[email protected]> 0 siblings, 1 reply; 8+ messages in thread From: Michael Paquier @ 2024-10-19 01:16 UTC (permalink / raw) To: Nathan Bossart <[email protected]>; +Cc: Postgres hackers <[email protected]> On Fri, Oct 18, 2024 at 12:22:26PM -0500, Nathan Bossart wrote: > On Fri, Oct 18, 2024 at 06:50:50PM +0900, Michael Paquier wrote: >> - appendPQExpBufferStr(&sql, "\nWHERE c.relpersistence != 't'"); >> + appendPQExpBufferStr(&sql, "\nWHERE c.relpersistence != " >> + CppAsString2(RELPERSISTENCE_TEMP) " "); > > I think these whitespace changes may cause invalid statements to be > produced in some code paths. IMHO those should be reserved for a separate > patch, anyway. I may be missing something, of course, but I don't quite see how this matters for this specific one. The possible paths after the temporary persistence check involve a qual on ep.pattern_id, then an addition based on opts.allrel. You are right that the extra whitespace after the RELPERSISTENCE_TEMP bit makes little sense. Removed that in the v2 attached. -- Michael Attachments: [text/x-diff] v2-0001-Use-more-CppAsString2-in-pg_amcheck.c.patch (5.7K, ../../[email protected]/2-v2-0001-Use-more-CppAsString2-in-pg_amcheck.c.patch) download | inline diff: From 7ef323354f1710ee755876132ae719b02c3e0468 Mon Sep 17 00:00:00 2001 From: Michael Paquier <[email protected]> Date: Sat, 19 Oct 2024 10:15:53 +0900 Subject: [PATCH v2] Use more CppAsString2() in pg_amcheck.c --- src/bin/pg_amcheck/pg_amcheck.c | 42 +++++++++++++++++++++++---------- 1 file changed, 29 insertions(+), 13 deletions(-) diff --git a/src/bin/pg_amcheck/pg_amcheck.c b/src/bin/pg_amcheck/pg_amcheck.c index 0c05cf58bc..27a7d5e925 100644 --- a/src/bin/pg_amcheck/pg_amcheck.c +++ b/src/bin/pg_amcheck/pg_amcheck.c @@ -16,6 +16,7 @@ #include <time.h> #include "catalog/pg_am_d.h" +#include "catalog/pg_class_d.h" #include "catalog/pg_namespace_d.h" #include "common/logging.h" #include "common/username.h" @@ -857,7 +858,7 @@ prepare_heap_command(PQExpBuffer sql, RelationInfo *rel, PGconn *conn) appendPQExpBuffer(sql, "\n) v WHERE c.oid = %u " - "AND c.relpersistence != 't'", + "AND c.relpersistence != " CppAsString2(RELPERSISTENCE_TEMP), rel->reloid); } @@ -890,7 +891,7 @@ prepare_btree_command(PQExpBuffer sql, RelationInfo *rel, PGconn *conn) "\nFROM pg_catalog.pg_class c, pg_catalog.pg_index i " "WHERE c.oid = %u " "AND c.oid = i.indexrelid " - "AND c.relpersistence != 't' " + "AND c.relpersistence != " CppAsString2(RELPERSISTENCE_TEMP) " " "AND i.indisready AND i.indisvalid AND i.indislive", rel->datinfo->amcheck_schema, (opts.heapallindexed ? "true" : "false"), @@ -905,7 +906,7 @@ prepare_btree_command(PQExpBuffer sql, RelationInfo *rel, PGconn *conn) "\nFROM pg_catalog.pg_class c, pg_catalog.pg_index i " "WHERE c.oid = %u " "AND c.oid = i.indexrelid " - "AND c.relpersistence != 't' " + "AND c.relpersistence != " CppAsString2(RELPERSISTENCE_TEMP) " " "AND i.indisready AND i.indisvalid AND i.indislive", rel->datinfo->amcheck_schema, (opts.heapallindexed ? "true" : "false"), @@ -1952,7 +1953,8 @@ compile_relation_list_one_db(PGconn *conn, SimplePtrList *relations, * until firing off the amcheck command, as the state of an index may * change by then. */ - appendPQExpBufferStr(&sql, "\nWHERE c.relpersistence != 't'"); + appendPQExpBufferStr(&sql, "\nWHERE c.relpersistence != " + CppAsString2(RELPERSISTENCE_TEMP)); if (opts.excludetbl || opts.excludeidx || opts.excludensp) appendPQExpBufferStr(&sql, "\nAND ep.pattern_id IS NULL"); @@ -1972,15 +1974,29 @@ compile_relation_list_one_db(PGconn *conn, SimplePtrList *relations, if (opts.allrel) appendPQExpBuffer(&sql, " AND c.relam = %u " - "AND c.relkind IN ('r', 'S', 'm', 't') " + "AND c.relkind IN (" + CppAsString2(RELKIND_RELATION) ", " + CppAsString2(RELKIND_SEQUENCE) ", " + CppAsString2(RELKIND_MATVIEW) ", " + CppAsString2(RELKIND_TOASTVALUE) ") " "AND c.relnamespace != %u", HEAP_TABLE_AM_OID, PG_TOAST_NAMESPACE); else appendPQExpBuffer(&sql, " AND c.relam IN (%u, %u)" - "AND c.relkind IN ('r', 'S', 'm', 't', 'i') " - "AND ((c.relam = %u AND c.relkind IN ('r', 'S', 'm', 't')) OR " - "(c.relam = %u AND c.relkind = 'i'))", + "AND c.relkind IN (" + CppAsString2(RELKIND_RELATION) ", " + CppAsString2(RELKIND_SEQUENCE) ", " + CppAsString2(RELKIND_MATVIEW) ", " + CppAsString2(RELKIND_TOASTVALUE) ", " + CppAsString2(RELKIND_INDEX) ") " + "AND ((c.relam = %u AND c.relkind IN (" + CppAsString2(RELKIND_RELATION) ", " + CppAsString2(RELKIND_SEQUENCE) ", " + CppAsString2(RELKIND_MATVIEW) ", " + CppAsString2(RELKIND_TOASTVALUE) ")) OR " + "(c.relam = %u AND c.relkind = " + CppAsString2(RELKIND_INDEX) "))", HEAP_TABLE_AM_OID, BTREE_AM_OID, HEAP_TABLE_AM_OID, BTREE_AM_OID); @@ -2007,7 +2023,7 @@ compile_relation_list_one_db(PGconn *conn, SimplePtrList *relations, "\nAND (t.relname ~ ep.rel_regex OR ep.rel_regex IS NULL)" "\nAND ep.heap_only" "\nWHERE ep.pattern_id IS NULL" - "\nAND t.relpersistence != 't'"); + "\nAND t.relpersistence != " CppAsString2(RELPERSISTENCE_TEMP)); appendPQExpBufferStr(&sql, "\n)"); } @@ -2026,7 +2042,7 @@ compile_relation_list_one_db(PGconn *conn, SimplePtrList *relations, "ON r.oid = i.indrelid " "INNER JOIN pg_catalog.pg_class c " "ON i.indexrelid = c.oid " - "AND c.relpersistence != 't'"); + "AND c.relpersistence != " CppAsString2(RELPERSISTENCE_TEMP)); if (opts.excludeidx || opts.excludensp) appendPQExpBufferStr(&sql, "\nINNER JOIN pg_catalog.pg_namespace n " @@ -2041,7 +2057,7 @@ compile_relation_list_one_db(PGconn *conn, SimplePtrList *relations, "\nWHERE true"); appendPQExpBuffer(&sql, " AND c.relam = %u " - "AND c.relkind = 'i'", + "AND c.relkind = " CppAsString2(RELKIND_INDEX), BTREE_AM_OID); if (opts.no_toast_expansion) appendPQExpBuffer(&sql, @@ -2065,7 +2081,7 @@ compile_relation_list_one_db(PGconn *conn, SimplePtrList *relations, "ON t.oid = i.indrelid" "\nINNER JOIN pg_catalog.pg_class c " "ON i.indexrelid = c.oid " - "AND c.relpersistence != 't'"); + "AND c.relpersistence != " CppAsString2(RELPERSISTENCE_TEMP)); if (opts.excludeidx) appendPQExpBufferStr(&sql, "\nLEFT OUTER JOIN exclude_pat ep " @@ -2078,7 +2094,7 @@ compile_relation_list_one_db(PGconn *conn, SimplePtrList *relations, "\nWHERE true"); appendPQExpBuffer(&sql, " AND c.relam = %u" - " AND c.relkind = 'i')", + " AND c.relkind = " CppAsString2(RELKIND_INDEX) ")", BTREE_AM_OID); } -- 2.45.2 [application/pgp-signature] signature.asc (833B, ../../[email protected]/3-signature.asc) download ^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Use more CppAsString2() in pg_amcheck.c @ 2024-11-25 15:25 Karina Litskevich <[email protected]> parent: Michael Paquier <[email protected]> 0 siblings, 1 reply; 8+ messages in thread From: Karina Litskevich @ 2024-11-25 15:25 UTC (permalink / raw) To: Michael Paquier <[email protected]>; +Cc: Nathan Bossart <[email protected]>; Postgres hackers <[email protected]> On Sat, Oct 19, 2024 at 4:17 AM Michael Paquier <[email protected]> wrote: > Removed that in the v2 attached. Hi Michael, The patch looks good to me. I'd like to suggest discussing a little cosmetic change in the affected lines. Compare the following. Lines 2095-2098: appendPQExpBuffer(&sql, " AND c.relam = %u" " AND c.relkind = " CppAsString2(RELKIND_INDEX) ")", BTREE_AM_OID); Lines 2058-2061: appendPQExpBuffer(&sql, " AND c.relam = %u " "AND c.relkind = " CppAsString2(RELKIND_INDEX), BTREE_AM_OID); They are not identical: space before AND vs space at the end of the previous line. I'd say that it would be better if they were identical. Personally, I prefer the one with a space before AND. Though we have two more similar cases in lines 1974-2001: if (opts.allrel) appendPQExpBuffer(&sql, " AND c.relam = %u " "AND c.relkind IN (" CppAsString2(RELKIND_RELATION) ", " CppAsString2(RELKIND_SEQUENCE) ", " CppAsString2(RELKIND_MATVIEW) ", " CppAsString2(RELKIND_TOASTVALUE) ") " "AND c.relnamespace != %u", HEAP_TABLE_AM_OID, PG_TOAST_NAMESPACE); else appendPQExpBuffer(&sql, " AND c.relam IN (%u, %u)" "AND c.relkind IN (" CppAsString2(RELKIND_RELATION) ", " CppAsString2(RELKIND_SEQUENCE) ", " CppAsString2(RELKIND_MATVIEW) ", " CppAsString2(RELKIND_TOASTVALUE) ", " CppAsString2(RELKIND_INDEX) ") " "AND ((c.relam = %u AND c.relkind IN (" CppAsString2(RELKIND_RELATION) ", " CppAsString2(RELKIND_SEQUENCE) ", " CppAsString2(RELKIND_MATVIEW) ", " CppAsString2(RELKIND_TOASTVALUE) ")) OR " "(c.relam = %u AND c.relkind = " CppAsString2(RELKIND_INDEX) "))", HEAP_TABLE_AM_OID, BTREE_AM_OID, HEAP_TABLE_AM_OID, BTREE_AM_OID); And I'm not sure how they should be handled if we choose a space before AND. Does the following still look fine? if (opts.allrel) appendPQExpBuffer(&sql, " AND c.relam = %u" " AND c.relkind IN (" CppAsString2(RELKIND_RELATION) ", " CppAsString2(RELKIND_SEQUENCE) ", " CppAsString2(RELKIND_MATVIEW) ", " CppAsString2(RELKIND_TOASTVALUE) ")" " AND c.relnamespace != %u", HEAP_TABLE_AM_OID, PG_TOAST_NAMESPACE); What do you think? Best regards, Karina Litskevich Postgres Professional: http://postgrespro.com/ ^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Use more CppAsString2() in pg_amcheck.c @ 2024-11-26 00:49 Michael Paquier <[email protected]> parent: Karina Litskevich <[email protected]> 0 siblings, 0 replies; 8+ messages in thread From: Michael Paquier @ 2024-11-26 00:49 UTC (permalink / raw) To: Karina Litskevich <[email protected]>; +Cc: Nathan Bossart <[email protected]>; Postgres hackers <[email protected]> On Mon, Nov 25, 2024 at 06:25:46PM +0300, Karina Litskevich wrote: > The patch looks good to me. Thanks for the review. > They are not identical: space before AND vs space at the end of the > previous line. I'd say that it would be better if they were > identical. Personally, I prefer the one with a space before AND. > Though we have two more similar cases in lines 1974-2001: > > And I'm not sure how they should be handled if we choose a space > before AND. Does the following still look fine? > > What do you think? Both look the same to me, TBH. :D The result in the queries generated is also the same before and after the patch. I see the inconsistency in style, but I'm not sure that it's worth bothering about that. Applied v2. -- Michael Attachments: [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 8+ messages in thread
end of thread, other threads:[~2024-11-26 00:49 UTC | newest] Thread overview: 8+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2019-09-05 11:21 [PATCH v12 1/4] Move callback-call from ReadPageInternal to XLogReadRecord. Kyotaro Horiguchi <[email protected]> 2024-10-18 09:50 Use more CppAsString2() in pg_amcheck.c Michael Paquier <[email protected]> 2024-10-18 09:59 ` Re: Use more CppAsString2() in pg_amcheck.c Daniel Gustafsson <[email protected]> 2024-10-18 13:18 ` Re: Use more CppAsString2() in pg_amcheck.c Alvaro Herrera <[email protected]> 2024-10-18 17:22 ` Re: Use more CppAsString2() in pg_amcheck.c Nathan Bossart <[email protected]> 2024-10-19 01:16 ` Re: Use more CppAsString2() in pg_amcheck.c Michael Paquier <[email protected]> 2024-11-25 15:25 ` Re: Use more CppAsString2() in pg_amcheck.c Karina Litskevich <[email protected]> 2024-11-26 00:49 ` Re: Use more CppAsString2() in pg_amcheck.c Michael Paquier <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox