Re: making update/delete of inheritance trees scale better
Tom Lane <tgl@sss.pgh.pa.us>
From: Tom Lane <tgl@sss.pgh.pa.us>
To: Amit Langote <amitlangote09@gmail.com>
Cc: Robert Haas <robertmhaas@gmail.com>, Heikki Linnakangas <hlinnaka@iki.fi>,
PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Date: 2021-03-30T04:51:15Z
Lists: pgsql-hackers
Attachments
- v13-0001-Overhaul-how-updates-compute-new-tuple.patch (text/x-diff) patch v13-0001
- v13-0002-Revise-how-inherited-update-delete-are-handled.patch (text/x-diff) patch v13-0002
Here's a v13 patchset that I feel pretty good about. My original thought for replacing the "fake variable" design was to add another RTE holding the extra variables, and then have setrefs.c translate the placeholder variables to the real thing at the last possible moment. I soon realized that instead of an actual RTE, it'd be better to invent a special varno value akin to INDEX_VAR (I called it ROWID_VAR, though I'm not wedded to that name). Info about the associated variables is kept in a list of RowIdentityVarInfo structs, which are more suitable than a regular RTE would be. I got that and the translate-in-setrefs approach more or less working, but it was fairly messy, because the need to know about these special variables spilled into FDWs and a lot of other places; for example indxpath.c needed a special check for them when deciding if an index-only scan is possible. What turns out to be a lot cleaner is to handle the translation in adjust_appendrel_attrs_mutator(), so that we have converted to real variables by the time we reach any relation-scan-level logic. I did end up having to break the API for FDW AddForeignUpdateTargets functions: they need to do things differently when adding junk columns, and they need different parameters. This seems all to the good though, because the old API has been a backwards-compatibility hack for some time (e.g., in not passing the "root" pointer). Some other random notes: * I was unimpressed with the idea of distinguishing different target relations by embedding integer constants in the plan. In the first place, the implementation was extremely fragile --- there was absolutely NOTHING tying the counter you used to the subplans' eventual indexes in the ModifyTable lists. Plus I don't have a lot of faith that setrefs.c will reliably do what you want in terms of bubbling the things up. Maybe that could be made more robust, but the other problem is that the EXPLAIN output is just about unreadable; nobody will understand what "(0)" means. So I went back to the idea of emitting tableoid, and installed a hashtable plus a one-entry lookup cache to make the run-time mapping as fast as I could. I'm not necessarily saying that this is how it has to be indefinitely, but I think we need more work on planner and EXPLAIN infrastructure before we can get the idea of directly providing a list index to work nicely. * I didn't agree with your decision to remove the now-failing test cases from postgres_fdw.sql. I think it's better to leave them there, especially in the cases where we were checking the plan as well as the execution. Hopefully we'll be able to un-break those soon. * I updated a lot of hereby-obsoleted comments, which makes the patch a bit longer than v12; but actually the code is a good bit smaller. There's a noticeable net code savings in src/backend/optimizer/, which there was not before. I've not made any attempt to do performance testing on this, but I think that's about the only thing standing between us and committing this thing. regards, tom lane
Commits
-
Rework planning and execution of UPDATE and DELETE.
- 86dc90056dfd 14.0 landed
-
Faster expression evaluation and targetlist projection.
- b8d7f053c5c2 10.0 cited