public inbox for [email protected]help / color / mirror / Atom feed
[PATCH v5 1/2] Let ALTER TABLE exec routines deal with the relation 11+ messages / 8 participants [nested] [flat]
* [PATCH v5 1/2] Let ALTER TABLE exec routines deal with the relation @ 2020-07-14 00:15 Alvaro Herrera <[email protected]> 0 siblings, 0 replies; 11+ messages in thread From: Alvaro Herrera @ 2020-07-14 00:15 UTC (permalink / raw) This means that ATExecCmd relies on AlteredRelationInfo->rel instead of keeping the relation as a local variable; this is useful if the subcommand needs to modify the relation internally. For example, a subcommand that internally commits its transaction needs this. --- src/backend/commands/tablecmds.c | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 993da56d43..a8528a3423 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -156,6 +156,8 @@ typedef struct AlteredTableInfo Oid relid; /* Relation to work on */ char relkind; /* Its relkind */ TupleDesc oldDesc; /* Pre-modification tuple descriptor */ + /* Transiently set during Phase 2, normally set to NULL */ + Relation rel; /* Information saved by Phase 1 for Phase 2: */ List *subcmds[AT_NUM_PASSES]; /* Lists of AlterTableCmd */ /* Information saved by Phases 1/2 for Phase 3: */ @@ -353,7 +355,7 @@ static void ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd, AlterTableUtilityContext *context); static void ATRewriteCatalogs(List **wqueue, LOCKMODE lockmode, AlterTableUtilityContext *context); -static void ATExecCmd(List **wqueue, AlteredTableInfo *tab, Relation rel, +static void ATExecCmd(List **wqueue, AlteredTableInfo *tab, AlterTableCmd *cmd, LOCKMODE lockmode, int cur_pass, AlterTableUtilityContext *context); static AlterTableCmd *ATParseTransformCmd(List **wqueue, AlteredTableInfo *tab, @@ -4405,7 +4407,6 @@ ATRewriteCatalogs(List **wqueue, LOCKMODE lockmode, { AlteredTableInfo *tab = (AlteredTableInfo *) lfirst(ltab); List *subcmds = tab->subcmds[pass]; - Relation rel; ListCell *lcmd; if (subcmds == NIL) @@ -4414,10 +4415,10 @@ ATRewriteCatalogs(List **wqueue, LOCKMODE lockmode, /* * Appropriate lock was obtained by phase 1, needn't get it again */ - rel = relation_open(tab->relid, NoLock); + tab->rel = relation_open(tab->relid, NoLock); foreach(lcmd, subcmds) - ATExecCmd(wqueue, tab, rel, + ATExecCmd(wqueue, tab, castNode(AlterTableCmd, lfirst(lcmd)), lockmode, pass, context); @@ -4429,7 +4430,11 @@ ATRewriteCatalogs(List **wqueue, LOCKMODE lockmode, if (pass == AT_PASS_ALTER_TYPE) ATPostAlterTypeCleanup(wqueue, tab, lockmode); - relation_close(rel, NoLock); + if (tab->rel) + { + relation_close(tab->rel, NoLock); + tab->rel = NULL; + } } } @@ -4455,11 +4460,12 @@ ATRewriteCatalogs(List **wqueue, LOCKMODE lockmode, * ATExecCmd: dispatch a subcommand to appropriate execution routine */ static void -ATExecCmd(List **wqueue, AlteredTableInfo *tab, Relation rel, +ATExecCmd(List **wqueue, AlteredTableInfo *tab, AlterTableCmd *cmd, LOCKMODE lockmode, int cur_pass, AlterTableUtilityContext *context) { ObjectAddress address = InvalidObjectAddress; + Relation rel = tab->rel; switch (cmd->subtype) { @@ -5562,6 +5568,7 @@ ATGetQueueEntry(List **wqueue, Relation rel) */ tab = (AlteredTableInfo *) palloc0(sizeof(AlteredTableInfo)); tab->relid = relid; + tab->rel = NULL; /* set later */ tab->relkind = rel->rd_rel->relkind; tab->oldDesc = CreateTupleDescCopyConstr(RelationGetDescr(rel)); tab->newrelpersistence = RELPERSISTENCE_PERMANENT; -- 2.20.1 --/04w6evG8XlLl3ft Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v5-0002-ALTER-TABLE-.-DETACH-CONCURRENTLY.patch" ^ permalink raw reply [nested|flat] 11+ messages in thread
* post-freeze damage control @ 2024-04-08 19:47 Robert Haas <[email protected]> 0 siblings, 3 replies; 11+ messages in thread From: Robert Haas @ 2024-04-08 19:47 UTC (permalink / raw) To: pgsql-hackers On Mon, Apr 8, 2024 at 10:42 AM Heikki Linnakangas <[email protected]> wrote: > Can you elaborate, which patches you think were not ready? Let's make > sure to capture any concrete concerns in the Open Items list. Hi, I'm moving this topic to a new thread for better visibility and less admixture of concerns. I'd like to invite everyone here present to opine on which patches we ought to be worried about. Here are a few picks from me to start things off. My intention here is to convey "I find these scary" rather than "these commits were irresponsible," so I respectfully ask that you don't take the choice to list your patch here as an attack, or the choice not to list your patch here as an endorsement. I'm very happy if I'm wrong and these patches are not actually scary. And likewise let's view any allegations of scariness by others as a genuine attempt to figure out what might be broken, rather than as an attempt to get anyone into trouble or whatever. - As I wrote about separately, I'm concerned that the failover slots stuff may not be in as good a shape as it needs to be for people to get good use out of it. http://postgr.es/m/CA+TgmoaA4oufUBR5B-4o83rnwGZ3zAA5UvwxDX=NjCm1TVgRsQ@mail.gmail.com - I also wrote separately about the flurry of recent table AM changes. http://postgr.es/m/CA+TgmoZpWB50GnJZYF1x8PenTqXDTFBH_Euu6ybGfzEy34o+5Q@mail.gmail.com - The streaming read API stuff was all committed very last minute. I think this should have been committed much sooner. It's probably not going to break the world; it's more likely to have performance consequences. But if it had gone in sooner, we'd have had more time to figure that out. - The heap pruning refactoring work, on the other hand, seems to be at very significant risk of breaking the world. I don't doubt the authors, but this is some very critical stuff and a lot of it happened very quickly right at the end. - Incremental backup could have all sorts of terrible data-corrupting bugs. 55a5ee30cd65886ff0a2e7ffef4ec2816fbec273 was a full brown-paper bag level fix, so the chances of there being further problems are probably high. - I'm slightly worried about the TID store work (ee1b30f12, 30e144287, 667e65aac35), perhaps for no reason. Actually, the results seem really impressive, and a very quick look at some of the commits seemed kind of reassuring. But, like the changes to pruning and freezing, this is making some really fundamental changes to critical code. In this case, it's code that has evolved very little over the years and seems to have now evolved quite a lot. - I couldn't understand why the "Operate XLogCtl->log{Write,Flush}Result with atomics" code was correct when I read it. That's not to say I know it to be incorrect. But, a spinlock protecting two variables together guarantees more than atomic access to each of those variables separately. There's probably a lot more to worry about; there have been so incredibly many changes recently. But this is as far as my speculation extends, as of now. Comments welcome. Additional concerns also welcome, as noted above. And again, no offense is intended to anyone. -- Robert Haas EDB: http://www.enterprisedb.com ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: post-freeze damage control @ 2024-04-08 21:35 Thomas Munro <[email protected]> parent: Robert Haas <[email protected]> 2 siblings, 1 reply; 11+ messages in thread From: Thomas Munro @ 2024-04-08 21:35 UTC (permalink / raw) To: Robert Haas <[email protected]>; +Cc: pgsql-hackers On Tue, Apr 9, 2024 at 7:47 AM Robert Haas <[email protected]> wrote: > - The streaming read API stuff was all committed very last minute. I > think this should have been committed much sooner. It's probably not > going to break the world; it's more likely to have performance > consequences. But if it had gone in sooner, we'd have had more time to > figure that out. OK, let me give an update on this work stream (pun intended). One reason for the delay in committing was precisely that we were fretting about regressions risks. We tried pretty hard to identify and grind down every regression we could find, and cases with outstanding not-fully-understood or examined problems in that area have been booted into the next cycle for more work: streaming bitmap heapscan, several streaming vacuum patches, and more, basically things that seem to have more complex interactions with other machinery. The only three places using streaming I/O that went in were: 041b9680: Use streaming I/O in ANALYZE. b7b0f3f2: Use streaming I/O in sequential scans. 3a352df0: Use streaming I/O in pg_prewarm. The first is a good first exercise in streaming random blocks; hopefully no one would be too upset about an unexpected small regression in ANALYZE, but as it happens it goes faster hot and cold according to all reports. The second is a good first exercise in streaming sequential blocks, and it ranges from faster to no regression, according to testing and reports. The third is less important, but it also goes faster. Of those, streaming seq scan is clearly the most relevant to real workloads that someone might be upset about, and I made a couple of choices that you might say had damage control in mind: * A conservative choice not to get into the business of the issuing new hints to the kernel for random jumps in cold scans, even though we think we probably should for better performance: more research needed precisely to avoid unexpected interactions (cf the booted bitmap heapscan where that sort of thing seems to be afoot). * A GUC to turn off I/O combining if it somehow upsets your storage in ways we didn't foresee (io_combine_limit=1). For fully cached hot scans, it does seem to be quite sensitive to tiny changes in a hot code path that I and others spent a lot of time optimising and testing during the CF. Perhaps it is possible that someone else's microarchitecture or compiler could show a regression that I don't see, and I will certainly look into it with vim and vigour if so. In that case we could consider a tiny micro-optimisation that I've shared already (it seemed a little novel so I'd rather propose it in the new cycle if I can), or, if it comes to it based on evidence and inability to address a problem quickly, reverting just b7b0f3f2 which itself is a very small patch. An aspect you didn't mention is correctness. I don't actually know how to prove that buffer manager protocols are correct beyond thinking and torture testing, ie what kind of new test harness machinery could be used to cross-check more things about buffer pool state explicitly, and that is a weakness I'm planning to look into. I realise that "these are the good ones, you should see all the stuff we decided not to commit!" is not an argument, I'm just laying out how I see the patches that went in and why I thought they were good. It's almost an architectural change, but done in tiny footsteps. I appreciate that people would have liked to see those particular tiny footsteps in some of the other fine months available for patching the tree, and some of the earlier underpinning patches that were part of the same patch series did go in around New Year, but clearly my "commit spreading" didn't go as well as planned after that (not helped by Jan/Feb summer vacation season down here). Mr Paquier this year announced his personal code freeze a few weeks back on social media, which seemed like an interesting idea I might adopt. Perhaps that is what some other people are doing without saying so, and perhaps the time they are using for that is the end of the calendar year. I might still be naturally inclined to crunch-like behaviour, but it wouldn't be at the same time as everyone else, except all the people who follow the same advice. ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: post-freeze damage control @ 2024-04-08 22:58 Michael Paquier <[email protected]> parent: Thomas Munro <[email protected]> 0 siblings, 0 replies; 11+ messages in thread From: Michael Paquier @ 2024-04-08 22:58 UTC (permalink / raw) To: Thomas Munro <[email protected]>; +Cc: Robert Haas <[email protected]>; pgsql-hackers On Tue, Apr 09, 2024 at 09:35:00AM +1200, Thomas Munro wrote: > Mr Paquier this year announced his personal code freeze a few weeks > back on social media, which seemed like an interesting idea I might > adopt. Perhaps that is what some other people are doing without > saying so, and perhaps the time they are using for that is the end of > the calendar year. I might still be naturally inclined to crunch-like > behaviour, but it wouldn't be at the same time as everyone else, > except all the people who follow the same advice. That's more linked to the fact that I was going silent without a laptop for a few weeks before the end of the release cycle, and a way to say to not count on me, while I was trying to keep my room clean to avoid noise for others who would rush patches. It is a vacation period for schools in Japan as the fiscal year finishes at the end of March, while the rest of the world still studies/works, so that makes trips much easier with areas being less busy when going abroad. If you want to limit commit activity during this period, the answer is simple then: require that all the committers live in Japan. Jokes apart, I really try to split commit effort across the year and not rush things at the last minute. If something's not agreed upon and commit-ready by the 15th of March, the chances that I would apply it within the release cycle are really slim. That's a kind of personal policy I have in place for a few years now. -- Michael Attachments: [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: post-freeze damage control @ 2024-04-08 23:16 Tomas Vondra <[email protected]> parent: Robert Haas <[email protected]> 2 siblings, 0 replies; 11+ messages in thread From: Tomas Vondra @ 2024-04-08 23:16 UTC (permalink / raw) To: Robert Haas <[email protected]>; pgsql-hackers On 4/8/24 21:47, Robert Haas wrote: > On Mon, Apr 8, 2024 at 10:42 AM Heikki Linnakangas <[email protected]> wrote: >> Can you elaborate, which patches you think were not ready? Let's make >> sure to capture any concrete concerns in the Open Items list. > > ... > > - Incremental backup could have all sorts of terrible data-corrupting > bugs. 55a5ee30cd65886ff0a2e7ffef4ec2816fbec273 was a full brown-paper > bag level fix, so the chances of there being further problems are > probably high. > I don't feel too particularly worried about this. Yes, backups are super important because it's often the only thing you have left when things go wrong, and the incremental aspect is all new. The code I've seen while doing the CoW-related patches seemed very precise and careful, and the one bug we found & fixed does not make it bad. Sure, I can't rule there being more bugs, but I've been doing some pretty extensive stress testing of this (doing incremental backups + combinebackup, and comparing the results against the source, and that sort of stuff). And so far only that single bug this way. I'm still doing this randomized stress testing, with more and more complex workloads etc. and I'll let keep doing that for a while. Maybe I'm a bit too happy-go-lucky, but IMO the risk here is limited. regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: post-freeze damage control @ 2024-04-09 02:12 Tom Lane <[email protected]> parent: Robert Haas <[email protected]> 2 siblings, 2 replies; 11+ messages in thread From: Tom Lane @ 2024-04-09 02:12 UTC (permalink / raw) To: Robert Haas <[email protected]>; +Cc: pgsql-hackers Robert Haas <[email protected]> writes: > On Mon, Apr 8, 2024 at 10:42 AM Heikki Linnakangas <[email protected]> wrote: >> Can you elaborate, which patches you think were not ready? Let's make >> sure to capture any concrete concerns in the Open Items list. > Hi, > I'm moving this topic to a new thread for better visibility and less > admixture of concerns. I'd like to invite everyone here present to > opine on which patches we ought to be worried about. Here are a few > picks from me to start things off. My intention here is to convey "I > find these scary" rather than "these commits were irresponsible," so I > respectfully ask that you don't take the choice to list your patch > here as an attack, or the choice not to list your patch here as an > endorsement. I have another one that I'm not terribly happy about: Author: Alexander Korotkov <[email protected]> Branch: master [72bd38cc9] 2024-04-08 01:27:52 +0300 Transform OR clauses to ANY expression I don't know that I'd call it scary exactly, but I do think it was premature. A week ago there was no consensus that it was ready to commit, but Alexander pushed it (or half of it, anyway) despite that. A few concrete concerns: * Yet another planner GUC. Do we really need or want that? * What the medical community would call off-label usage of query jumbling. I'm not sure this is even correct as-used, and for sure it's using that code for something never intended. Nor is the added code adequately (as in, at all) documented. * Patch refuses to group anything but Consts into the SAOP transformation. I realize that if you want to produce an array Const you need Const inputs, but I wonder why it wasn't considered to produce an ARRAY[] construct if there are available clauses with pseudo-constant (eg Param) comparison values. * I really, really dislike jamming this logic into prepqual.c, where it has no business being. I note that it was shoved into process_duplicate_ors without even the courtesy of expanding the header comment: * process_duplicate_ors * Given a list of exprs which are ORed together, try to apply * the inverse OR distributive law. Another reason to think this wasn't a very well chosen place is that the file's list of #include's went from 4 entries to 11. Somebody should have twigged to the idea that this was off-topic for prepqual.c. * OrClauseGroupKey is not a Node type, so why does it have a NodeTag? I wonder what value will appear in that field, and what will happen if the struct is passed to any code that expects real Nodes. I could probably find some other nits if I spent more time on it, but I think these are sufficient to show that this was not commit-ready. regards, tom lane ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: post-freeze damage control @ 2024-04-09 05:37 Andrei Lepikhov <[email protected]> parent: Tom Lane <[email protected]> 1 sibling, 1 reply; 11+ messages in thread From: Andrei Lepikhov @ 2024-04-09 05:37 UTC (permalink / raw) To: Tom Lane <[email protected]>; Robert Haas <[email protected]>; +Cc: pgsql-hackers On 9/4/2024 09:12, Tom Lane wrote: > I have another one that I'm not terribly happy about: > > Author: Alexander Korotkov <[email protected]> > Branch: master [72bd38cc9] 2024-04-08 01:27:52 +0300 > > Transform OR clauses to ANY expression Because I'm primary author of the idea, let me answer. > > I don't know that I'd call it scary exactly, but I do think it > was premature. A week ago there was no consensus that it was > ready to commit, but Alexander pushed it (or half of it, anyway) > despite that. A few concrete concerns: > > * Yet another planner GUC. Do we really need or want that? It is the most interesting question here. Looking around planner features designed but not applied for the same reason because they can produce suboptimal plans in corner cases, I think about inventing flag-type parameters and hiding some features that work better for different load types under such flagged parameters. > > * What the medical community would call off-label usage of > query jumbling. I'm not sure this is even correct as-used, > and for sure it's using that code for something never intended. > Nor is the added code adequately (as in, at all) documented. I agree with documentation and disagree with critics on the expression jumbling. It was introduced in the core. Why don't we allow it to be used to speed up machinery with some hashing? > > * Patch refuses to group anything but Consts into the SAOP > transformation. I realize that if you want to produce an > array Const you need Const inputs, but I wonder why it > wasn't considered to produce an ARRAY[] construct if there > are available clauses with pseudo-constant (eg Param) > comparison values. Good point. I think, we can consider that in the future. > > * I really, really dislike jamming this logic into prepqual.c, > where it has no business being. I note that it was shoved > into process_duplicate_ors without even the courtesy of > expanding the header comment: Yeah, I preferred to do it in parse_expr.c with the assumption of some 'minimal' or 'canonical' tree form. You can see this code in the previous version. I think we don't have any bugs here, but we have different opinions on how it should work. > > * process_duplicate_ors > * Given a list of exprs which are ORed together, try to apply > * the inverse OR distributive law. > > Another reason to think this wasn't a very well chosen place is > that the file's list of #include's went from 4 entries to 11. > Somebody should have twigged to the idea that this was off-topic > for prepqual.c. > > * OrClauseGroupKey is not a Node type, so why does it have > a NodeTag? I wonder what value will appear in that field, > and what will happen if the struct is passed to any code > that expects real Nodes. It's a hack authored by Alexander. I guess He can provide additional reasons in support of that. > > I could probably find some other nits if I spent more time > on it, but I think these are sufficient to show that this > was not commit-ready. It's up to you. On the one hand, I don't see any bugs or strong performance issues, and all the issues can be resolved further; on the other hand, I've got your good review and some ideas to work out. -- regards, Andrei Lepikhov Postgres Professional ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: post-freeze damage control @ 2024-04-09 05:55 Tom Lane <[email protected]> parent: Andrei Lepikhov <[email protected]> 0 siblings, 0 replies; 11+ messages in thread From: Tom Lane @ 2024-04-09 05:55 UTC (permalink / raw) To: Andrei Lepikhov <[email protected]>; +Cc: Robert Haas <[email protected]>; pgsql-hackers Andrei Lepikhov <[email protected]> writes: > On 9/4/2024 09:12, Tom Lane wrote: >> I have another one that I'm not terribly happy about: >> Author: Alexander Korotkov <[email protected]> >> Branch: master [72bd38cc9] 2024-04-08 01:27:52 +0300 >> Transform OR clauses to ANY expression >> * What the medical community would call off-label usage of >> query jumbling. I'm not sure this is even correct as-used, >> and for sure it's using that code for something never intended. >> Nor is the added code adequately (as in, at all) documented. > I agree with documentation and disagree with critics on the expression > jumbling. It was introduced in the core. Why don't we allow it to be > used to speed up machinery with some hashing? I would back up from that a good deal: why do we need to hash here in the first place? There's no evidence I'm aware of that it's needful from a performance standpoint. >> * I really, really dislike jamming this logic into prepqual.c, >> where it has no business being. I note that it was shoved >> into process_duplicate_ors without even the courtesy of >> expanding the header comment: > Yeah, I preferred to do it in parse_expr.c with the assumption of some > 'minimal' or 'canonical' tree form. That seems quite the wrong direction to me. AFAICS, the argument for making this transformation depends on being able to convert to an indexscan condition, so I would try to apply it even later, when we have a set of restriction conditions to apply to a particular baserel. (This would weaken the argument that we need hashing rather than naive equal() tests even further, I think.) Applying the transform to join quals seems unlikely to be a win. regards, tom lane ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: post-freeze damage control @ 2024-04-09 19:47 Alexander Korotkov <[email protected]> parent: Tom Lane <[email protected]> 1 sibling, 1 reply; 11+ messages in thread From: Alexander Korotkov @ 2024-04-09 19:47 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Robert Haas <[email protected]>; pgsql-hackers On Tue, Apr 9, 2024 at 5:12 AM Tom Lane <[email protected]> wrote: > * OrClauseGroupKey is not a Node type, so why does it have > a NodeTag? I wonder what value will appear in that field, > and what will happen if the struct is passed to any code > that expects real Nodes. I used that to put both not-subject-of-transform nodes together with hash entries into the same list. This is used to save the order of clauses. I think this is an important property, and I have already expressed it in [1]. That could be achieved without adding NodeTag to hash entries, but that would require a level of indirection. It's not passed to code that expects real Nodes, it doesn't go to anything except lists. Links. 1. https://www.postgresql.org/message-id/CAPpHfdutHt31sdt2rfU%3D4fsDMWxf6tvtnHARgCzLY2Tf21%2Bfgw%40mail... ------ Regards, Alexander Korotkov ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: post-freeze damage control @ 2024-04-09 20:37 Tom Lane <[email protected]> parent: Alexander Korotkov <[email protected]> 0 siblings, 1 reply; 11+ messages in thread From: Tom Lane @ 2024-04-09 20:37 UTC (permalink / raw) To: Alexander Korotkov <[email protected]>; +Cc: Robert Haas <[email protected]>; pgsql-hackers Alexander Korotkov <[email protected]> writes: > On Tue, Apr 9, 2024 at 5:12 AM Tom Lane <[email protected]> wrote: >> * OrClauseGroupKey is not a Node type, so why does it have >> a NodeTag? I wonder what value will appear in that field, >> and what will happen if the struct is passed to any code >> that expects real Nodes. > I used that to put both not-subject-of-transform nodes together with > hash entries into the same list. This is used to save the order of > clauses. I think this is an important property, and I have already > expressed it in [1]. What exactly is the point of having a NodeTag in the struct though? If you don't need it to be a valid Node, that seems pointless and confusing. We certainly have plenty of other lists that contain plain structs without tags, so I don't buy that the List infrastructure is making you do that. regards, tom lane ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: post-freeze damage control @ 2024-04-09 20:48 Alexander Korotkov <[email protected]> parent: Tom Lane <[email protected]> 0 siblings, 0 replies; 11+ messages in thread From: Alexander Korotkov @ 2024-04-09 20:48 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Robert Haas <[email protected]>; pgsql-hackers On Tue, Apr 9, 2024 at 11:37 PM Tom Lane <[email protected]> wrote: > Alexander Korotkov <[email protected]> writes: > > On Tue, Apr 9, 2024 at 5:12 AM Tom Lane <[email protected]> wrote: > >> * OrClauseGroupKey is not a Node type, so why does it have > >> a NodeTag? I wonder what value will appear in that field, > >> and what will happen if the struct is passed to any code > >> that expects real Nodes. > > > I used that to put both not-subject-of-transform nodes together with > > hash entries into the same list. This is used to save the order of > > clauses. I think this is an important property, and I have already > > expressed it in [1]. > > What exactly is the point of having a NodeTag in the struct though? > If you don't need it to be a valid Node, that seems pointless and > confusing. We certainly have plenty of other lists that contain > plain structs without tags, so I don't buy that the List > infrastructure is making you do that. This code mixes Expr's and hash entries in the single list. The point of having a NodeTag in the struct is the ability to distinguish them later. ------ Regards, Alexander Korotkov ^ permalink raw reply [nested|flat] 11+ messages in thread
end of thread, other threads:[~2024-04-09 20:48 UTC | newest] Thread overview: 11+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2020-07-14 00:15 [PATCH v5 1/2] Let ALTER TABLE exec routines deal with the relation Alvaro Herrera <[email protected]> 2024-04-08 19:47 post-freeze damage control Robert Haas <[email protected]> 2024-04-08 21:35 ` Re: post-freeze damage control Thomas Munro <[email protected]> 2024-04-08 22:58 ` Re: post-freeze damage control Michael Paquier <[email protected]> 2024-04-08 23:16 ` Re: post-freeze damage control Tomas Vondra <[email protected]> 2024-04-09 02:12 ` Re: post-freeze damage control Tom Lane <[email protected]> 2024-04-09 05:37 ` Re: post-freeze damage control Andrei Lepikhov <[email protected]> 2024-04-09 05:55 ` Re: post-freeze damage control Tom Lane <[email protected]> 2024-04-09 19:47 ` Re: post-freeze damage control Alexander Korotkov <[email protected]> 2024-04-09 20:37 ` Re: post-freeze damage control Tom Lane <[email protected]> 2024-04-09 20:48 ` Re: post-freeze damage control Alexander Korotkov <[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