public inbox for [email protected]help / color / mirror / Atom feed
[PATCH 1/4] bootstrap: convert Typ to a List* 2+ messages / 2 participants [nested] [flat]
* [PATCH 1/4] bootstrap: convert Typ to a List* @ 2020-11-20 02:48 Justin Pryzby <[email protected]> 0 siblings, 0 replies; 2+ messages in thread From: Justin Pryzby @ 2020-11-20 02:48 UTC (permalink / raw) --- src/backend/bootstrap/bootstrap.c | 89 ++++++++++++++----------------- 1 file changed, 41 insertions(+), 48 deletions(-) diff --git a/src/backend/bootstrap/bootstrap.c b/src/backend/bootstrap/bootstrap.c index 6f615e6622..1b940d9d27 100644 --- a/src/backend/bootstrap/bootstrap.c +++ b/src/backend/bootstrap/bootstrap.c @@ -58,7 +58,7 @@ static void BootstrapModeMain(void); static void bootstrap_signals(void); static void ShutdownAuxiliaryProcess(int code, Datum arg); static Form_pg_attribute AllocateAttribute(void); -static void populate_typ_array(void); +static void populate_typ(void); static Oid gettype(char *type); static void cleanup(void); @@ -159,7 +159,7 @@ struct typmap FormData_pg_type am_typ; }; -static struct typmap **Typ = NULL; +static List *Typ = NIL; /* List of struct typmap* */ static struct typmap *Ap = NULL; static Datum values[MAXATTR]; /* current row's attribute values */ @@ -595,10 +595,10 @@ boot_openrel(char *relname) /* * pg_type must be filled before any OPEN command is executed, hence we - * can now populate the Typ array if we haven't yet. + * can now populate Typ if we haven't yet. */ - if (Typ == NULL) - populate_typ_array(); + if (Typ == NIL) + populate_typ(); if (boot_reldesc != NULL) closerel(NULL); @@ -688,7 +688,7 @@ DefineAttr(char *name, char *type, int attnum, int nullness) typeoid = gettype(type); - if (Typ != NULL) + if (Typ != NIL) { attrtypes[attnum]->atttypid = Ap->am_oid; attrtypes[attnum]->attlen = Ap->am_typ.typlen; @@ -866,47 +866,36 @@ cleanup(void) } /* ---------------- - * populate_typ_array + * populate_typ * - * Load the Typ array by reading pg_type. + * Load Typ by reading pg_type. * ---------------- */ static void -populate_typ_array(void) +populate_typ(void) { Relation rel; TableScanDesc scan; HeapTuple tup; - int nalloc; - int i; - - Assert(Typ == NULL); + MemoryContext old; - nalloc = 512; - Typ = (struct typmap **) - MemoryContextAlloc(TopMemoryContext, nalloc * sizeof(struct typmap *)); + Assert(Typ == NIL); rel = table_open(TypeRelationId, NoLock); scan = table_beginscan_catalog(rel, 0, NULL); - i = 0; + old = MemoryContextSwitchTo(TopMemoryContext); while ((tup = heap_getnext(scan, ForwardScanDirection)) != NULL) { Form_pg_type typForm = (Form_pg_type) GETSTRUCT(tup); + struct typmap *newtyp; - /* make sure there will be room for a trailing NULL pointer */ - if (i >= nalloc - 1) - { - nalloc *= 2; - Typ = (struct typmap **) - repalloc(Typ, nalloc * sizeof(struct typmap *)); - } - Typ[i] = (struct typmap *) - MemoryContextAlloc(TopMemoryContext, sizeof(struct typmap)); - Typ[i]->am_oid = typForm->oid; - memcpy(&(Typ[i]->am_typ), typForm, sizeof(Typ[i]->am_typ)); - i++; + newtyp = (struct typmap *) palloc(sizeof(struct typmap)); + Typ = lappend(Typ, newtyp); + + newtyp->am_oid = typForm->oid; + memcpy(&newtyp->am_typ, typForm, sizeof(newtyp->am_typ)); } - Typ[i] = NULL; /* Fill trailing NULL pointer */ + MemoryContextSwitchTo(old); table_endscan(scan); table_close(rel, NoLock); } @@ -916,25 +905,26 @@ populate_typ_array(void) * * NB: this is really ugly; it will return an integer index into TypInfo[], * and not an OID at all, until the first reference to a type not known in - * TypInfo[]. At that point it will read and cache pg_type in the Typ array, + * TypInfo[]. At that point it will read and cache pg_type in Typ, * and subsequently return a real OID (and set the global pointer Ap to * point at the found row in Typ). So caller must check whether Typ is - * still NULL to determine what the return value is! + * still NIL to determine what the return value is! * ---------------- */ static Oid gettype(char *type) { - if (Typ != NULL) + if (Typ != NIL) { - struct typmap **app; + ListCell *lc; - for (app = Typ; *app != NULL; app++) + foreach (lc, Typ) { - if (strncmp(NameStr((*app)->am_typ.typname), type, NAMEDATALEN) == 0) + struct typmap *app = lfirst(lc); + if (strncmp(NameStr(app->am_typ.typname), type, NAMEDATALEN) == 0) { - Ap = *app; - return (*app)->am_oid; + Ap = app; + return app->am_oid; } } } @@ -949,7 +939,7 @@ gettype(char *type) } /* Not in TypInfo, so we'd better be able to read pg_type now */ elog(DEBUG4, "external type: %s", type); - populate_typ_array(); + populate_typ(); return gettype(type); } elog(ERROR, "unrecognized type \"%s\"", type); @@ -977,17 +967,20 @@ boot_get_type_io_data(Oid typid, Oid *typinput, Oid *typoutput) { - if (Typ != NULL) + if (Typ != NIL) { /* We have the boot-time contents of pg_type, so use it */ - struct typmap **app; - struct typmap *ap; - - app = Typ; - while (*app && (*app)->am_oid != typid) - ++app; - ap = *app; - if (ap == NULL) + struct typmap *ap = NULL; + ListCell *lc; + + foreach (lc, Typ) + { + ap = lfirst(lc); + if (ap->am_oid == typid) + break; + } + + if (!ap || ap->am_oid != typid) elog(ERROR, "type OID %u not found in Typ list", typid); *typlen = ap->am_typ.typlen; -- 2.26.2 --------------BCA1373AE29B32B57608F4EE Content-Type: text/x-patch; charset=UTF-8; name="0002-Allow-composite-types-in-bootstrap-20210307.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="0002-Allow-composite-types-in-bootstrap-20210307.patch" ^ permalink raw reply [nested|flat] 2+ messages in thread
* Re: pg_walfile_name_offset can return inconsistent values @ 2023-11-09 19:22 Bruce Momjian <[email protected]> 0 siblings, 0 replies; 2+ messages in thread From: Bruce Momjian @ 2023-11-09 19:22 UTC (permalink / raw) To: Jehan-Guillaume de Rorthais <[email protected]>; +Cc: Kyotaro Horiguchi <[email protected]>; [email protected] On Fri, Jul 26, 2019 at 11:30:19AM +0200, Jehan-Guillaume de Rorthais wrote: > On Fri, 26 Jul 2019 17:21:20 +0900 (Tokyo Standard Time) > Kyotaro Horiguchi <[email protected]> wrote: > > > Hello. > > > > While looking [1], I noticed that pg_walfile_name_offset behaves > > somewhat oddly at segment boundary. > > > > select * from (values ('0/16ffffff'), ('0/17000000'), ('0/17000001')) as > > t(lsn), lateral pg_walfile_name_offset(lsn::pg_lsn); > > lsn | file_name | file_offset > > ------------+--------------------------+------------- > > 0/16ffffff | 000000020000000000000016 | 16777215 > > 0/17000000 | 000000020000000000000016 | 0 > > 0/17000001 | 000000020000000000000017 | 1 > > > > > > The file names are right as defined, but the return value of the > > second line wrong, or at least misleading. > > +1 > I noticed it as well and put this report on hold while working on my patch. > Thanks for reporting this! > > > It should be (16, 1000000) or (16, FFFFFF). The former is out-of-domain so we > > would have no way than choosing the latter. I'm not sure the purpose of > > the second output parameter, thus the former might be right > > decision. > > > > The function returns the following result after this patch is > > applied. > > > > select * from (values ('0/16ffffff'), ('0/17000000'), ('0/17000001')) as > > t(lsn), lateral pg_walfile_name_offset(lsn::pg_lsn); > > lsn | file_name | file_offset > > ------------+--------------------------+------------- > > 0/16ffffff | 000000020000000000000016 | 16777214 > > 0/17000000 | 000000020000000000000016 | 16777215 > > 0/17000001 | 000000020000000000000017 | 0 > > So you shift the file offset for all LSN by one byte? This could lead to > regression in various tools relying on this function. > > Moreover, it looks weird as the LSN doesn't reflect the given offset anymore > (FFFFFF <> 16777214, 000001 <> 0, etc). > > Another solution might be to return the same result when for both 0/16ffffff and > 0/17000000, but it doesn't feel right either. > > So in fact, returning 0x1000000 seems to be the cleaner result to me. I know this bug report is four years old, but it is still a pg_walfile_name_offset() bug. Here is the bug: SELECT * FROM (VALUES ('0/16ffffff'), ('0/17000000'), ('0/17000001')) AS t(lsn), LATERAL pg_walfile_name_offset(lsn::pg_lsn); lsn | file_name | file_offset ------------+--------------------------+------------- 0/16ffffff | 000000010000000000000016 | 16777215 --> 0/17000000 | 000000010000000000000016 | 0 0/17000001 | 000000010000000000000017 | 1 The bug is in the indicated line --- it shows the filename as 00016 but offset as zero, when clearly the LSN is pointing to 17/0. The bug is essentially that the code for pg_walfile_name_offset() uses the exact offset from the LSN, but uses the file name from the previous byte of the LSN. The fix involves deciding what the description or purpose of pg_walfile_name_offset() means, and adjusting it to be clearer. The current documentation says: Converts a write-ahead log location to a WAL file name and byte offset within that file. Fix #1: If we assume write-ahead log location means LSN, it is saying show the file/offset of the LSN, and that is most clearly: lsn | file_name | file_offset ------------+--------------------------+------------- 0/16ffffff | 000000010000000000000016 | 16777215 0/17000000 | 000000010000000000000017 | 0 0/17000001 | 000000010000000000000017 | 1 Fix #2: Now, there are some who have said they want the output to be the last written WAL byte (the byte before the LSN), not the current LSN, for archiving purposes. However, if we do that, we have to update the docs to clarify it. Its output would be: lsn | file_name | file_offset ------------+--------------------------+------------- 0/16ffffff | 000000010000000000000016 | 16777214 0/17000000 | 000000010000000000000016 | 16777215 0/17000001 | 000000010000000000000017 | 0 The email thread also considered having the second row offset be 16777216 (2^24), which is not a valid offset for a file if we assume a zero-based offset. Looking further, pg_walfile_name() also returns the filename for the previous byte: SELECT * FROM (values ('0/16ffffff'), ('0/17000000'), ('0/17000001')) as t(lsn), LATERAL pg_walfile_name_offset(lsn::pg_lsn), LATERAL pg_walfile_name(lsn::pg_lsn); lsn | file_name | file_offset | pg_walfile_name ------------+--------------------------+-------------+-------------------------- 0/16ffffff | 000000010000000000000016 | 16777215 | 000000010000000000000016 0/17000000 | 000000010000000000000016 | 0 | 000000010000000000000016 0/17000001 | 000000010000000000000017 | 1 | 000000010000000000000017 I have attached fix #1 as offset1.diff and fix #2 as offset2.diff. I think the most logical fix is #1, but pg_walfile_name() would need to be modified. If the previous file/byte offset are what is desired, fix #2 will need doc changes for both functions. This probably needs to be documented as a backward incompatibility for either fix. -- Bruce Momjian <[email protected]> https://momjian.us EDB https://enterprisedb.com Only you can decide what is important to you. Attachments: [text/x-diff] offset1.diff (1.1K, ../../[email protected]/2-offset1.diff) download | inline diff: diff --git a/src/backend/access/transam/xlogfuncs.c b/src/backend/access/transam/xlogfuncs.c index 45a70668b1..5039052263 100644 --- a/src/backend/access/transam/xlogfuncs.c +++ b/src/backend/access/transam/xlogfuncs.c @@ -378,6 +378,8 @@ pg_last_wal_replay_lsn(PG_FUNCTION_ARGS) * Note that a location exactly at a segment boundary is taken to be in * the previous segment. This is usually the right thing, since the * expected usage is to determine which xlog file(s) are ready to archive. + * To be consistent to filename, returns the offset one byte before the given + * location as offset. */ Datum pg_walfile_name_offset(PG_FUNCTION_ARGS) @@ -411,10 +413,13 @@ pg_walfile_name_offset(PG_FUNCTION_ARGS) resultTupleDesc = BlessTupleDesc(resultTupleDesc); + /* We assume the desired location is one-byte before the LSN. */ + locationpoint--; + /* * xlogfilename */ - XLByteToPrevSeg(locationpoint, xlogsegno, wal_segment_size); + XLByteToSeg(locationpoint, xlogsegno, wal_segment_size); XLogFileName(xlogfilename, GetWALInsertionTimeLine(), xlogsegno, wal_segment_size); [text/x-diff] offset2.diff (525B, ../../[email protected]/3-offset2.diff) download | inline diff: diff --git a/src/backend/access/transam/xlogfuncs.c b/src/backend/access/transam/xlogfuncs.c index 45a70668b1..e65502d51e 100644 --- a/src/backend/access/transam/xlogfuncs.c +++ b/src/backend/access/transam/xlogfuncs.c @@ -414,7 +414,7 @@ pg_walfile_name_offset(PG_FUNCTION_ARGS) /* * xlogfilename */ - XLByteToPrevSeg(locationpoint, xlogsegno, wal_segment_size); + XLByteToSeg(locationpoint, xlogsegno, wal_segment_size); XLogFileName(xlogfilename, GetWALInsertionTimeLine(), xlogsegno, wal_segment_size); ^ permalink raw reply [nested|flat] 2+ messages in thread
end of thread, other threads:[~2023-11-09 19:22 UTC | newest] Thread overview: 2+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2020-11-20 02:48 [PATCH 1/4] bootstrap: convert Typ to a List* Justin Pryzby <[email protected]> 2023-11-09 19:22 Re: pg_walfile_name_offset can return inconsistent values Bruce Momjian <[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