public inbox for [email protected]
help / color / mirror / Atom feed[PATCH 1/5] bootstrap: convert Typ to a List*
6+ messages / 3 participants
[nested] [flat]
* [PATCH 1/5] bootstrap: convert Typ to a List*
@ 2020-11-20 02:48 Justin Pryzby <[email protected]>
0 siblings, 0 replies; 6+ messages in thread
From: Justin Pryzby @ 2020-11-20 02:48 UTC (permalink / raw)
---
src/backend/bootstrap/bootstrap.c | 69 ++++++++++++++-----------------
1 file changed, 31 insertions(+), 38 deletions(-)
diff --git a/src/backend/bootstrap/bootstrap.c b/src/backend/bootstrap/bootstrap.c
index 6f615e6622..18eb62ca47 100644
--- a/src/backend/bootstrap/bootstrap.c
+++ b/src/backend/bootstrap/bootstrap.c
@@ -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 */
@@ -597,7 +597,7 @@ 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.
*/
- if (Typ == NULL)
+ if (Typ == NIL)
populate_typ_array();
if (boot_reldesc != 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;
@@ -877,36 +877,25 @@ populate_typ_array(void)
Relation rel;
TableScanDesc scan;
HeapTuple tup;
- int nalloc;
- int i;
-
- Assert(Typ == NULL);
- 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;
while ((tup = heap_getnext(scan, ForwardScanDirection)) != NULL)
{
Form_pg_type typForm = (Form_pg_type) GETSTRUCT(tup);
+ struct typmap *newtyp;
+ MemoryContext old;
- /* 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++;
+ old = MemoryContextSwitchTo(TopMemoryContext);
+ newtyp = (struct typmap *) palloc(sizeof(struct typmap));
+ Typ = lappend(Typ, newtyp);
+ MemoryContextSwitchTo(old);
+
+ newtyp->am_oid = typForm->oid;
+ memcpy(&newtyp->am_typ, typForm, sizeof(newtyp->am_typ));
}
- Typ[i] = NULL; /* Fill trailing NULL pointer */
table_endscan(scan);
table_close(rel, NoLock);
}
@@ -925,16 +914,17 @@ populate_typ_array(void)
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;
}
}
}
@@ -980,14 +970,17 @@ boot_get_type_io_data(Oid typid,
if (Typ != NULL)
{
/* 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
--------------614DDB87AFFED893713AC0E9
Content-Type: text/x-patch; charset=UTF-8;
name="0002-Allow-composite-types-in-bootstrap-20210304.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
filename="0002-Allow-composite-types-in-bootstrap-20210304.patch"
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Improve WALRead() to suck data directly from WAL buffers when possible
@ 2024-02-12 19:33 Jeff Davis <[email protected]>
2024-02-12 20:18 ` Re: Improve WALRead() to suck data directly from WAL buffers when possible Andres Freund <[email protected]>
2024-02-12 23:56 ` Re: Improve WALRead() to suck data directly from WAL buffers when possible Jeff Davis <[email protected]>
0 siblings, 2 replies; 6+ messages in thread
From: Jeff Davis @ 2024-02-12 19:33 UTC (permalink / raw)
To: Bharath Rupireddy <[email protected]>; Alvaro Herrera <[email protected]>; +Cc: Andres Freund <[email protected]>; Dilip Kumar <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected]; Nathan Bossart <[email protected]>; Masahiko Sawada <[email protected]>
On Wed, 2024-01-31 at 14:30 +0530, Bharath Rupireddy wrote:
> Please see the attached v22 patch set.
Committed 0001.
For 0002 & 0003, I'd like more clarity on how they will actually be
used by an extension.
For 0004, we need to resolve why callers are using XLOG_BLCKSZ and we
can fix that independently, as discussed here:
https://www.postgresql.org/message-id/[email protected]...
Regards,
Jeff Davis
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Improve WALRead() to suck data directly from WAL buffers when possible
2024-02-12 19:33 Re: Improve WALRead() to suck data directly from WAL buffers when possible Jeff Davis <[email protected]>
@ 2024-02-12 20:18 ` Andres Freund <[email protected]>
2024-02-12 20:46 ` Re: Improve WALRead() to suck data directly from WAL buffers when possible Jeff Davis <[email protected]>
1 sibling, 1 reply; 6+ messages in thread
From: Andres Freund @ 2024-02-12 20:18 UTC (permalink / raw)
To: Jeff Davis <[email protected]>; +Cc: Bharath Rupireddy <[email protected]>; Alvaro Herrera <[email protected]>; Dilip Kumar <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected]; Nathan Bossart <[email protected]>; Masahiko Sawada <[email protected]>
Hi,
On 2024-02-12 11:33:24 -0800, Jeff Davis wrote:
> On Wed, 2024-01-31 at 14:30 +0530, Bharath Rupireddy wrote:
> > Please see the attached v22 patch set.
>
> Committed 0001.
Yay, I think this is very cool. There are plenty other improvements than can
be based on this...
One thing I'm a bit confused in the code is the following:
+ /*
+ * Don't read past the available WAL data.
+ *
+ * Check using local copy of LogwrtResult. Ordinarily it's been updated by
+ * the caller when determining how far to read; but if not, it just means
+ * we'll read less data.
+ *
+ * XXX: the available WAL could be extended to the WAL insert pointer by
+ * calling WaitXLogInsertionsToFinish().
+ */
+ upto = Min(startptr + count, LogwrtResult.Write);
+ nbytes = upto - startptr;
Shouldn't it pretty much be a bug to ever encounter this? There aren't
equivalent checks in WALRead(), so any user of WALReadFromBuffers() that then
falls back to WALRead() is just going to send unwritten data.
ISTM that this should be an assertion or error.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Improve WALRead() to suck data directly from WAL buffers when possible
2024-02-12 19:33 Re: Improve WALRead() to suck data directly from WAL buffers when possible Jeff Davis <[email protected]>
2024-02-12 20:18 ` Re: Improve WALRead() to suck data directly from WAL buffers when possible Andres Freund <[email protected]>
@ 2024-02-12 20:46 ` Jeff Davis <[email protected]>
0 siblings, 0 replies; 6+ messages in thread
From: Jeff Davis @ 2024-02-12 20:46 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Bharath Rupireddy <[email protected]>; Alvaro Herrera <[email protected]>; Dilip Kumar <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected]; Nathan Bossart <[email protected]>; Masahiko Sawada <[email protected]>
On Mon, 2024-02-12 at 12:18 -0800, Andres Freund wrote:
> + upto = Min(startptr + count, LogwrtResult.Write);
> + nbytes = upto - startptr;
>
> Shouldn't it pretty much be a bug to ever encounter this?
In the current code it's impossible, though Bharath hinted at an
extension which could reach that path.
What I committed was a bit of a compromise -- earlier versions of the
patch supported reading right up to the Insert pointer (which requires
a call to WaitXLogInsertionsToFinish()). I wasn't ready to commit that
code without seeing a more about how that would be used, but I thought
it was reasonable to have some simple code in there to allow reading up
to the Write pointer.
It seems closer to the structure that we will ultimately need to
replicate unflushed data, right?
Regards,
Jeff Davis
[1]
https://www.postgresql.org/message-id/[email protected]...
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Improve WALRead() to suck data directly from WAL buffers when possible
2024-02-12 19:33 Re: Improve WALRead() to suck data directly from WAL buffers when possible Jeff Davis <[email protected]>
@ 2024-02-12 23:56 ` Jeff Davis <[email protected]>
2024-02-13 00:11 ` Re: Improve WALRead() to suck data directly from WAL buffers when possible Andres Freund <[email protected]>
1 sibling, 1 reply; 6+ messages in thread
From: Jeff Davis @ 2024-02-12 23:56 UTC (permalink / raw)
To: Bharath Rupireddy <[email protected]>; Alvaro Herrera <[email protected]>; +Cc: Andres Freund <[email protected]>; Dilip Kumar <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected]; Nathan Bossart <[email protected]>; Masahiko Sawada <[email protected]>
On Mon, 2024-02-12 at 11:33 -0800, Jeff Davis wrote:
> For 0002 & 0003, I'd like more clarity on how they will actually be
> used by an extension.
In patch 0002, I'm concerned about calling
WaitXLogInsertionsToFinish(). It loops through all the locks, but
doesn't have any early return path or advance any state.
So if it's repeatedly called with the same or similar values it seems
like it would be doing a lot of extra work.
I'm not sure of the best fix. We could add something to LogwrtResult to
track a new LSN that represents the highest known point where all
inserters are finished (in other words, the latest return value of
WaitXLogInsertionsToFinish()). That seems invasive, though.
Regards,
Jeff Davis
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Improve WALRead() to suck data directly from WAL buffers when possible
2024-02-12 19:33 Re: Improve WALRead() to suck data directly from WAL buffers when possible Jeff Davis <[email protected]>
2024-02-12 23:56 ` Re: Improve WALRead() to suck data directly from WAL buffers when possible Jeff Davis <[email protected]>
@ 2024-02-13 00:11 ` Andres Freund <[email protected]>
0 siblings, 0 replies; 6+ messages in thread
From: Andres Freund @ 2024-02-13 00:11 UTC (permalink / raw)
To: Jeff Davis <[email protected]>; +Cc: Bharath Rupireddy <[email protected]>; Alvaro Herrera <[email protected]>; Dilip Kumar <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected]; Nathan Bossart <[email protected]>; Masahiko Sawada <[email protected]>
Hi,
On 2024-02-12 15:56:19 -0800, Jeff Davis wrote:
> On Mon, 2024-02-12 at 11:33 -0800, Jeff Davis wrote:
> > For 0002 & 0003, I'd like more clarity on how they will actually be
> > used by an extension.
>
> In patch 0002, I'm concerned about calling
> WaitXLogInsertionsToFinish(). It loops through all the locks, but
> doesn't have any early return path or advance any state.
I doubt it'd be too bad - we call that at much much higher frequency during
write heavy OLTP workloads (c.f. XLogFlush()). It can be a performance issue
there, but only after increasing NUM_XLOGINSERT_LOCKS - before that the
limited number of writers is the limit. Compared to that walsender shouldn't
be a significant factor.
However, I think it's a very bad idea to call WALReadFromBuffers() from
WALReadFromBuffers(). This needs to be at the caller, not down in
WALReadFromBuffers().
I don't see why we would want to weaken the error condition in
WaitXLogInsertionsToFinish() - I suspect it'd not work correctly to wait for
insertions that aren't yet in progress and it just seems like an API misuse.
> So if it's repeatedly called with the same or similar values it seems like
> it would be doing a lot of extra work.
>
> I'm not sure of the best fix. We could add something to LogwrtResult to
> track a new LSN that represents the highest known point where all
> inserters are finished (in other words, the latest return value of
> WaitXLogInsertionsToFinish()). That seems invasive, though.
FWIW, I think LogwrtResult is an anti-pattern, perhaps introduced due to
misunderstanding how cache coherency works. It's not fundamentally faster to
access non-shared memory. It'd make far more sense to allow lock-free access
to the shared LogwrtResult and
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 6+ messages in thread
end of thread, other threads:[~2024-02-13 00:11 UTC | newest]
Thread overview: 6+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-11-20 02:48 [PATCH 1/5] bootstrap: convert Typ to a List* Justin Pryzby <[email protected]>
2024-02-12 19:33 Re: Improve WALRead() to suck data directly from WAL buffers when possible Jeff Davis <[email protected]>
2024-02-12 20:18 ` Re: Improve WALRead() to suck data directly from WAL buffers when possible Andres Freund <[email protected]>
2024-02-12 20:46 ` Re: Improve WALRead() to suck data directly from WAL buffers when possible Jeff Davis <[email protected]>
2024-02-12 23:56 ` Re: Improve WALRead() to suck data directly from WAL buffers when possible Jeff Davis <[email protected]>
2024-02-13 00:11 ` Re: Improve WALRead() to suck data directly from WAL buffers when possible Andres Freund <[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