Обсуждение: Ought to use heap_multi_insert() for pg_attribute/depend insertions?
Hi,
Turns out in portions of the regression tests a good chunk of the
runtime is inside AddNewAttributeTuples() and
recordMultipleDependencies()'s heap insertions. Looking at a few
profiles I had lying around I found that in some production cases
too. ISTM we should use heap_multi_insert() for both, as the source
tuples ought to be around reasonably comfortably.
For recordMultipleDependencies() it'd obviously better if we collected
all dependencies for new objects, rather than doing so separately. Right
now e.g. the code for a new table looks like:
recordDependencyOn(&myself, &referenced, DEPENDENCY_NORMAL);
recordDependencyOnOwner(RelationRelationId, relid, ownerid);
recordDependencyOnNewAcl(RelationRelationId, relid, 0, ownerid, relacl);
recordDependencyOnCurrentExtension(&myself, false);
if (reloftypeid)
{
referenced.classId = TypeRelationId;
referenced.objectId = reloftypeid;
referenced.objectSubId = 0;
recordDependencyOn(&myself, &referenced, DEPENDENCY_NORMAL);
}
and it'd obviously be more efficient to do that once if we went to using
heap_multi_insert() in the dependency code. But I suspect even if we
just used an extended API in AddNewAttributeTuples() (for the type /
collation dependencies), it'd be a win.
I'm not planning to work on this soon, but I though it'd be worthwhile
to put this out there (even if potentially just as a note to myself).
Greetings,
Andres Freund
> On 13 Feb 2019, at 19:27, Andres Freund <andres@anarazel.de> wrote:
>
> Hi,
>
> Turns out in portions of the regression tests a good chunk of the
> runtime is inside AddNewAttributeTuples() and
> recordMultipleDependencies()'s heap insertions. Looking at a few
> profiles I had lying around I found that in some production cases
> too. ISTM we should use heap_multi_insert() for both, as the source
> tuples ought to be around reasonably comfortably.
>
> For recordMultipleDependencies() it'd obviously better if we collected
> all dependencies for new objects, rather than doing so separately. Right
> now e.g. the code for a new table looks like:
>
> recordDependencyOn(&myself, &referenced, DEPENDENCY_NORMAL);
>
> recordDependencyOnOwner(RelationRelationId, relid, ownerid);
>
> recordDependencyOnNewAcl(RelationRelationId, relid, 0, ownerid, relacl);
>
> recordDependencyOnCurrentExtension(&myself, false);
>
> if (reloftypeid)
> {
> referenced.classId = TypeRelationId;
> referenced.objectId = reloftypeid;
> referenced.objectSubId = 0;
> recordDependencyOn(&myself, &referenced, DEPENDENCY_NORMAL);
> }
>
> and it'd obviously be more efficient to do that once if we went to using
> heap_multi_insert() in the dependency code. But I suspect even if we
> just used an extended API in AddNewAttributeTuples() (for the type /
> collation dependencies), it'd be a win.
When a colleague was looking at heap_multi_insert in the COPY codepath I
remembered this and took a stab at a WIP patch inspired by this email, while
not following it to the letter. It’s not going the full route of collecting
all the dependencies for creating a table, but adding ways to perform
multi_heap_insert in the existing codepaths as it seemed like a good place to
start.
It introduces a new function CatalogMultiInsertWithInfo which takes a set of
slots for use in heap_multi_insert, used from recordMultipleDependencies and
InsertPgAttributeTuples (which replace calling InsertPgAttributeTuple
repeatedly). The code is still a WIP with some kludges, following the show-
early philosophy.
It passes make check and some light profiling around regress suites indicates
that it does improve a bit by reducing the somewhat costly calls. Is this
along the lines of what you were thinking or way off?
cheers ./daniel
Вложения
Hi,
On 2019-05-22 10:25:14 +0200, Daniel Gustafsson wrote:
> > On 13 Feb 2019, at 19:27, Andres Freund <andres@anarazel.de> wrote:
> >
> > Hi,
> >
> > Turns out in portions of the regression tests a good chunk of the
> > runtime is inside AddNewAttributeTuples() and
> > recordMultipleDependencies()'s heap insertions. Looking at a few
> > profiles I had lying around I found that in some production cases
> > too. ISTM we should use heap_multi_insert() for both, as the source
> > tuples ought to be around reasonably comfortably.
> >
> > For recordMultipleDependencies() it'd obviously better if we collected
> > all dependencies for new objects, rather than doing so separately. Right
> > now e.g. the code for a new table looks like:
> >
> > recordDependencyOn(&myself, &referenced, DEPENDENCY_NORMAL);
> >
> > recordDependencyOnOwner(RelationRelationId, relid, ownerid);
> >
> > recordDependencyOnNewAcl(RelationRelationId, relid, 0, ownerid, relacl);
> >
> > recordDependencyOnCurrentExtension(&myself, false);
> >
> > if (reloftypeid)
> > {
> > referenced.classId = TypeRelationId;
> > referenced.objectId = reloftypeid;
> > referenced.objectSubId = 0;
> > recordDependencyOn(&myself, &referenced, DEPENDENCY_NORMAL);
> > }
> >
> > and it'd obviously be more efficient to do that once if we went to using
> > heap_multi_insert() in the dependency code. But I suspect even if we
> > just used an extended API in AddNewAttributeTuples() (for the type /
> > collation dependencies), it'd be a win.
>
> When a colleague was looking at heap_multi_insert in the COPY codepath I
> remembered this and took a stab at a WIP patch inspired by this email, while
> not following it to the letter. It’s not going the full route of collecting
> all the dependencies for creating a table, but adding ways to perform
> multi_heap_insert in the existing codepaths as it seemed like a good place to
> start.
Cool. I don't quite have the energy to look at this right now, could you
create a CF entry for this?
Greetings,
Andres Freund
> On 23 May 2019, at 03:46, Andres Freund <andres@anarazel.de> wrote: > On 2019-05-22 10:25:14 +0200, Daniel Gustafsson wrote: >> When a colleague was looking at heap_multi_insert in the COPY codepath I >> remembered this and took a stab at a WIP patch inspired by this email, while >> not following it to the letter. It’s not going the full route of collecting >> all the dependencies for creating a table, but adding ways to perform >> multi_heap_insert in the existing codepaths as it seemed like a good place to >> start. > > Cool. I don't quite have the energy to look at this right now, could you > create a CF entry for this? Of course, done. cheers ./daniel
Hi, On 2019-05-22 10:25:14 +0200, Daniel Gustafsson wrote: > It passes make check and some light profiling around regress suites indicates > that it does improve a bit by reducing the somewhat costly calls. Just for the record, here is the profile I did: perf record --call-graph lbr make -s check-world -Otarget -j16 -s perf report Greetings, Andres Freund
> On 23 May 2019, at 03:46, Andres Freund <andres@anarazel.de> wrote: > On 2019-05-22 10:25:14 +0200, Daniel Gustafsson wrote: >> When a colleague was looking at heap_multi_insert in the COPY codepath I >> remembered this and took a stab at a WIP patch inspired by this email, while >> not following it to the letter. It’s not going the full route of collecting >> all the dependencies for creating a table, but adding ways to perform >> multi_heap_insert in the existing codepaths as it seemed like a good place to >> start. > > Cool. I don't quite have the energy to look at this right now, could you > create a CF entry for this? Attached is an updated version with some of the stuff we briefly discussed at PGCon. This version use the ObjectAddresses API already established to collect the dependencies, and perform a few more multi inserts. Profiling shows that we are spending less time in catalog insertions, but whether it’s enough to warrant the added complexity is up for debate. The patch is still rough around the edges (TODO’s left to mark some areas), but I prefer to get some feedback early rather than working too far in potentially the wrong direction, so parking this in the CF for now. cheers ./daniel
Вложения
Hi, On 2019-06-11 15:20:42 +0200, Daniel Gustafsson wrote: > Attached is an updated version with some of the stuff we briefly discussed at > PGCon. This version use the ObjectAddresses API already established to collect > the dependencies, and perform a few more multi inserts. Cool. > Profiling shows that > we are spending less time in catalog insertions, but whether it’s enough to > warrant the added complexity is up for debate. Probably worth benchmarking e.g. temp table creation speed in isolation. People do complain about that occasionally. Greetings, Andres Freund
On Wed, Jun 12, 2019 at 1:21 AM Daniel Gustafsson <daniel@yesql.se> wrote: > The patch is still rough around the edges (TODO’s left to mark some areas), but > I prefer to get some feedback early rather than working too far in potentially > the wrong direction, so parking this in the CF for now. Hi Daniel, Given the above disclaimers the following may be entirely expected, but just in case you weren't aware: t/010_logical_decoding_timelines.pl fails with this patch applied. https://travis-ci.org/postgresql-cfbot/postgresql/builds/555205042 -- Thomas Munro https://enterprisedb.com
> On 8 Jul 2019, at 00:02, Thomas Munro <thomas.munro@gmail.com> wrote: > > On Wed, Jun 12, 2019 at 1:21 AM Daniel Gustafsson <daniel@yesql.se> wrote: >> The patch is still rough around the edges (TODO’s left to mark some areas), but >> I prefer to get some feedback early rather than working too far in potentially >> the wrong direction, so parking this in the CF for now. > > Hi Daniel, > > Given the above disclaimers the following may be entirely expected, > but just in case you weren't aware: > t/010_logical_decoding_timelines.pl fails with this patch applied. > > https://travis-ci.org/postgresql-cfbot/postgresql/builds/555205042 I hadn’t seen since I had fat-fingered and accidentally run the full tests in a tree without assertions. The culprit here seems to an assertion in the logical decoding code which doesn’t account for heap_multi_insert into catalog relations (which there are none now, this patch introduce them and thus trip the assertion). As the issue is somewhat unrelated, I’ve opened a separate thread with a small patch: https://postgr.es/m/CBFFD532-C033-49EB-9A5A-F67EAEE9EB0B@yesql.se The attached v3 also has that fix in order to see if the cfbot is happier with this. cheers ./daniel
Вложения
On Tue, Jul 9, 2019 at 11:07 PM Daniel Gustafsson <daniel@yesql.se> wrote: > The attached v3 also has that fix in order to see if the cfbot is happier with > this. Noticed while moving this to the next CF: heap.c: In function ‘StorePartitionKey’: 1191heap.c:3582:3: error: ‘referenced’ undeclared (first use in this function) 1192 referenced.classId = RelationRelationId; 1193 ^ -- Thomas Munro https://enterprisedb.com
> On 2 Aug 2019, at 00:41, Thomas Munro <thomas.munro@gmail.com> wrote: > > On Tue, Jul 9, 2019 at 11:07 PM Daniel Gustafsson <daniel@yesql.se> wrote: >> The attached v3 also has that fix in order to see if the cfbot is happier with >> this. > > Noticed while moving this to the next CF: > > heap.c: In function ‘StorePartitionKey’: > 1191heap.c:3582:3: error: ‘referenced’ undeclared (first use in this function) > 1192 referenced.classId = RelationRelationId; > 1193 ^ Thanks, the attached v4 updates to patch to handle a0555ddab9b672a046 as well. cheers ./daniel
Вложения
On Mon, Aug 05, 2019 at 09:20:07AM +0200, Daniel Gustafsson wrote: > Thanks, the attached v4 updates to patch to handle a0555ddab9b672a046 as well. + if (referenced->numrefs == 1) + recordDependencyOn(depender, &referenced->refs[0], behavior); + else + recordMultipleDependencies(depender, + referenced->refs, referenced->numrefs, + behavior); This makes me wonder if we should not just add a shortcut in recordMultipleDependencies() to use recordDependencyOn if there is only one reference in the set. That would save the effort of a multi insert for all callers of recordMultipleDependencies() this way, including the future ones. And that could also be done independently of the addition of InsertPgAttributeTuples(), no? -- Michael
Вложения
On Mon, Aug 05, 2019 at 04:45:59PM +0900, Michael Paquier wrote:
> + if (referenced->numrefs == 1)
> + recordDependencyOn(depender, &referenced->refs[0], behavior);
> + else
> + recordMultipleDependencies(depender,
> + referenced->refs, referenced->numrefs,
> + behavior);
> This makes me wonder if we should not just add a shortcut in
> recordMultipleDependencies() to use recordDependencyOn if there is
> only one reference in the set. That would save the effort of a multi
> insert for all callers of recordMultipleDependencies() this way,
> including the future ones. And that could also be done independently
> of the addition of InsertPgAttributeTuples(), no?
A comment in heap_multi_insert() needs to be updated because it
becomes the case with your patch:
/*
* We don't use heap_multi_insert for catalog tuples yet, but
* better be prepared...
*/
There is also this one in DecodeMultiInsert()
* CONTAINS_NEW_TUPLE will always be set currently as multi_insert
* isn't used for catalogs, but better be future proof.
(I am going to comment on the assertion issue on the other thread, I
got some suggestions about it.)
--
Michael
Вложения
On Tue, Aug 06, 2019 at 11:24:46AM +0900, Michael Paquier wrote: > A comment in heap_multi_insert() needs to be updated because it > becomes the case with your patch: > /* > * We don't use heap_multi_insert for catalog tuples yet, but > * better be prepared... > */ > > There is also this one in DecodeMultiInsert() > * CONTAINS_NEW_TUPLE will always be set currently as multi_insert > * isn't used for catalogs, but better be future proof. Applying the latest patch, this results in an assertion failure for the tests of test_decoding. > (I am going to comment on the assertion issue on the other thread, I > got some suggestions about it.) This part has resulted in 75c1921, and we could just change DecodeMultiInsert() so as if there is no tuple data then we'd just leave. However, I don't feel completely comfortable with that either as it would be nice to still check that for normal relations we *always* have a FPW available. Daniel, your thoughts? I am switching the patch as waiting on author. -- Michael
Вложения
> On 11 Nov 2019, at 09:32, Michael Paquier <michael@paquier.xyz> wrote: > This part has resulted in 75c1921, and we could just change > DecodeMultiInsert() so as if there is no tuple data then we'd just > leave. However, I don't feel completely comfortable with that either > as it would be nice to still check that for normal relations we > *always* have a FPW available. XLH_INSERT_CONTAINS_NEW_TUPLE will only be set in case of catalog relations IIUC (that is, non logically decoded relations), so it seems to me that we can have a fastpath out of DecodeMultiInsert() which inspects that flag without problems. Is this proposal along the lines of what you were thinking? The patch is now generating an error in the regression tests as well, on your recently added CREATE INDEX test from 68ac9cf2499236996f3d4bf31f7f16d5bd3c77af. By using the ObjectAddresses API the dependencies are deduped before recorded, removing the duplicate entry from that test output. AFAICT there is nothing benefiting us from having duplicate dependencies, so this seems an improvement (albeit tiny and not very important), or am I missing something? Is there a use for duplicate dependencies? Attached version contains the above two fixes, as well as a multi_insert for dependencies in CREATE EXTENSION which I had missed to git add in previous versions. cheers ./daniel
Вложения
On 2019-11-12 16:25:06 +0100, Daniel Gustafsson wrote:
> > On 11 Nov 2019, at 09:32, Michael Paquier <michael@paquier.xyz> wrote:
>
> > This part has resulted in 75c1921, and we could just change
> > DecodeMultiInsert() so as if there is no tuple data then we'd just
> > leave. However, I don't feel completely comfortable with that either
> > as it would be nice to still check that for normal relations we
> > *always* have a FPW available.
>
> XLH_INSERT_CONTAINS_NEW_TUPLE will only be set in case of catalog relations
> IIUC (that is, non logically decoded relations), so it seems to me that we can
> have a fastpath out of DecodeMultiInsert() which inspects that flag without
> problems. Is this proposal along the lines of what you were thinking?
Maybe I'm missing something, but it's the opposite, no?
bool need_tuple_data = RelationIsLogicallyLogged(relation);
...
if (need_tuple_data)
xlrec->flags |= XLH_INSERT_CONTAINS_NEW_TUPLE;
or am I misunderstanding what you mean?
> @@ -1600,10 +1598,16 @@ recordDependencyOnExpr(const ObjectAddress *depender,
> /* Remove any duplicates */
> eliminate_duplicate_dependencies(context.addrs);
>
> - /* And record 'em */
> - recordMultipleDependencies(depender,
> - context.addrs->refs, context.addrs->numrefs,
> - behavior);
> + /*
> + * And record 'em. If we know we only have a single dependency then
> + * avoid the extra cost of setting up a multi insert.
> + */
> + if (context.addrs->numrefs == 1)
> + recordDependencyOn(depender, &context.addrs->refs[0], behavior);
> + else
> + recordMultipleDependencies(depender,
> + context.addrs->refs, context.addrs->numrefs,
> + behavior);
I'm not sure this is actually a worthwhile complexity to keep. Hard to
believe that setting up a multi-insert is goign to have a significant
enough overhead to matter here?
And if it does, is there a chance we can hide this repeated block
somewhere within recordMultipleDependencies() or such? I don't like the
repetitiveness here. Seems recordMultipleDependencies() could easily
optimize the case of there being exactly one dependency to insert?
> +/*
> + * InsertPgAttributeTuples
> + * Construct and insert multiple tuples in pg_attribute.
> + *
> + * This is a variant of InsertPgAttributeTuple() which dynamically allocates
> + * space for multiple tuples. Having two so similar functions is a kludge, but
> + * for now it's a TODO to make it less terrible.
> + */
> +void
> +InsertPgAttributeTuples(Relation pg_attribute_rel,
> + FormData_pg_attribute *new_attributes,
> + int natts,
> + CatalogIndexState indstate)
> +{
> + Datum values[Natts_pg_attribute];
> + bool nulls[Natts_pg_attribute];
> + HeapTuple tup;
> + int i;
> + TupleTableSlot **slot;
> +
> + /*
> + * The slots are dropped in CatalogMultiInsertWithInfo(). TODO: natts
> + * number of slots is not a reasonable assumption as a wide relation
> + * would be detrimental, figuring a good number is left as a TODO.
> + */
> + slot = palloc(sizeof(TupleTableSlot *) * natts);
Hm. Looking at
SELECT avg(pg_column_size(pa)) FROM pg_attribute pa;
yielding ~144 bytes, we can probably cap this at 128 or such, without
loosing efficiency. Or just use
#define MAX_BUFFERED_BYTES 65535
from copy.c or such (MAX_BUFFERED_BYTES / sizeof(FormData_pg_attribute)).
> + /* This is a tad tedious, but way cleaner than what we used to do... */
I don't like comments that refer to "what we used to" because there's no
way for anybody to make sense of that, because it's basically a dangling
reference :)
> + memset(values, 0, sizeof(values));
> + memset(nulls, false, sizeof(nulls));
> + /* start out with empty permissions and empty options */
> + nulls[Anum_pg_attribute_attacl - 1] = true;
> + nulls[Anum_pg_attribute_attoptions - 1] = true;
> + nulls[Anum_pg_attribute_attfdwoptions - 1] = true;
> + nulls[Anum_pg_attribute_attmissingval - 1] = true;
> +
> + /* attcacheoff is always -1 in storage */
> + values[Anum_pg_attribute_attcacheoff - 1] = Int32GetDatum(-1);
> +
> + for (i = 0; i < natts; i++)
> + {
> + values[Anum_pg_attribute_attrelid - 1] = ObjectIdGetDatum(new_attributes[i].attrelid);
> + values[Anum_pg_attribute_attname - 1] = NameGetDatum(&new_attributes[i].attname);
> + values[Anum_pg_attribute_atttypid - 1] = ObjectIdGetDatum(new_attributes[i].atttypid);
> + values[Anum_pg_attribute_attstattarget - 1] = Int32GetDatum(new_attributes[i].attstattarget);
> + values[Anum_pg_attribute_attlen - 1] = Int16GetDatum(new_attributes[i].attlen);
> + values[Anum_pg_attribute_attnum - 1] = Int16GetDatum(new_attributes[i].attnum);
> + values[Anum_pg_attribute_attndims - 1] = Int32GetDatum(new_attributes[i].attndims);
> + values[Anum_pg_attribute_atttypmod - 1] = Int32GetDatum(new_attributes[i].atttypmod);
> + values[Anum_pg_attribute_attbyval - 1] = BoolGetDatum(new_attributes[i].attbyval);
> + values[Anum_pg_attribute_attstorage - 1] = CharGetDatum(new_attributes[i].attstorage);
> + values[Anum_pg_attribute_attalign - 1] = CharGetDatum(new_attributes[i].attalign);
> + values[Anum_pg_attribute_attnotnull - 1] = BoolGetDatum(new_attributes[i].attnotnull);
> + values[Anum_pg_attribute_atthasdef - 1] = BoolGetDatum(new_attributes[i].atthasdef);
> + values[Anum_pg_attribute_atthasmissing - 1] = BoolGetDatum(new_attributes[i].atthasmissing);
> + values[Anum_pg_attribute_attidentity - 1] = CharGetDatum(new_attributes[i].attidentity);
> + values[Anum_pg_attribute_attgenerated - 1] = CharGetDatum(new_attributes[i].attgenerated);
> + values[Anum_pg_attribute_attisdropped - 1] = BoolGetDatum(new_attributes[i].attisdropped);
> + values[Anum_pg_attribute_attislocal - 1] = BoolGetDatum(new_attributes[i].attislocal);
> + values[Anum_pg_attribute_attinhcount - 1] = Int32GetDatum(new_attributes[i].attinhcount);
> + values[Anum_pg_attribute_attcollation - 1] = ObjectIdGetDatum(new_attributes[i].attcollation);
> +
> + slot[i] = MakeSingleTupleTableSlot(RelationGetDescr(pg_attribute_rel),
> + &TTSOpsHeapTuple);
> + tup = heap_form_tuple(RelationGetDescr(pg_attribute_rel), values, nulls);
> + ExecStoreHeapTuple(heap_copytuple(tup), slot[i], false);
This seems likely to waste some effort - during insertion the slot will
be materialized, which'll copy the tuple. I think it'd be better to
construct the tuple directly inside the slot's tts_values/isnull, and
then store it with ExecStoreVirtualTuple().
Also, why aren't you actually just specifying shouldFree = true? We'd
want the result of the heap_form_tuple() to be freed eagerly, no? Nor do
I get why you're *also* heap_copytuple'ing the tuple, despite having
just locally formed it, and not referencing the result of
heap_form_tuple() further? Obviously that's all unimportant if you
change the code to use ExecStoreVirtualTuple()?
> + }
> +
> + /* finally insert the new tuples, update the indexes, and clean up */
> + CatalogMultiInsertWithInfo(pg_attribute_rel, slot, natts, indstate);
Previous comment:
I think it might be worthwhile to clear and delete the slots
after inserting. That's potentially a good bit of memory, no?
Current comment:
I think I quite dislike the API of CatalogMultiInsertWithInfo freeing
the slots. It'd e.g. make it impossible to reuse them to insert more
data. It's also really hard to understand
> +}
> +
> /*
> * InsertPgAttributeTuple
> * Construct and insert a new tuple in pg_attribute.
> @@ -754,7 +827,7 @@ AddNewAttributeTuples(Oid new_rel_oid,
> TupleDesc tupdesc,
> char relkind)
> {
> - Form_pg_attribute attr;
> + Form_pg_attribute *attrs;
> int i;
> Relation rel;
> CatalogIndexState indstate;
> @@ -769,35 +842,42 @@ AddNewAttributeTuples(Oid new_rel_oid,
>
> indstate = CatalogOpenIndexes(rel);
>
> + attrs = palloc(sizeof(Form_pg_attribute) * natts);
Hm. Why we we need this separate allocation? Isn't this exactly the same
as what's in the tupledesc?
> +/*
> + * CatalogMultiInsertWithInfo
Hm. The current function is called CatalogTupleInsert(), wouldn't
CatalogTuplesInsertWithInfo() or such be more accurate? Or
CatalogTuplesMultiInsertWithInfo()?
> @@ -81,7 +128,14 @@ recordMultipleDependencies(const ObjectAddress *depender,
>
> memset(nulls, false, sizeof(nulls));
>
> - for (i = 0; i < nreferenced; i++, referenced++)
> + values[Anum_pg_depend_classid - 1] = ObjectIdGetDatum(depender->classId);
> + values[Anum_pg_depend_objid - 1] = ObjectIdGetDatum(depender->objectId);
> + values[Anum_pg_depend_objsubid - 1] = Int32GetDatum(depender->objectSubId);
> +
> + /* TODO is nreferenced a reasonable allocation of slots? */
> + slot = palloc(sizeof(TupleTableSlot *) * nreferenced);
> +
> + for (i = 0, ntuples = 0; i < nreferenced; i++, referenced++)
> {
> /*
> * If the referenced object is pinned by the system, there's no real
> @@ -94,30 +148,34 @@ recordMultipleDependencies(const ObjectAddress *depender,
> * Record the Dependency. Note we don't bother to check for
> * duplicate dependencies; there's no harm in them.
> */
> - values[Anum_pg_depend_classid - 1] = ObjectIdGetDatum(depender->classId);
> - values[Anum_pg_depend_objid - 1] = ObjectIdGetDatum(depender->objectId);
> - values[Anum_pg_depend_objsubid - 1] = Int32GetDatum(depender->objectSubId);
> -
> values[Anum_pg_depend_refclassid - 1] = ObjectIdGetDatum(referenced->classId);
> values[Anum_pg_depend_refobjid - 1] = ObjectIdGetDatum(referenced->objectId);
> values[Anum_pg_depend_refobjsubid - 1] = Int32GetDatum(referenced->objectSubId);
>
> values[Anum_pg_depend_deptype - 1] = CharGetDatum((char) behavior);
>
> - tup = heap_form_tuple(dependDesc->rd_att, values, nulls);
> + slot[ntuples] = MakeSingleTupleTableSlot(RelationGetDescr(dependDesc),
> + &TTSOpsHeapTuple);
> +
> + tuple = heap_form_tuple(dependDesc->rd_att, values, nulls);
> + ExecStoreHeapTuple(heap_copytuple(tuple), slot[ntuples], false);
> + ntuples++;
Same concern as in the equivalent pg_attribute code.
> + ntuples = 0;
> while (HeapTupleIsValid(tup = systable_getnext(scan)))
> {
> - HeapTuple newtup;
> -
> - newtup = heap_modify_tuple(tup, sdepDesc, values, nulls, replace);
> - CatalogTupleInsertWithInfo(sdepRel, newtup, indstate);
> + slot[ntuples] = MakeSingleTupleTableSlot(RelationGetDescr(sdepRel),
> + &TTSOpsHeapTuple);
> + newtuple = heap_modify_tuple(tup, sdepDesc, values, nulls, replace);
> + ExecStoreHeapTuple(heap_copytuple(newtuple), slot[ntuples], false);
> + ntuples++;
> - heap_freetuple(newtup);
> + if (ntuples == DEPEND_TUPLE_BUF)
> + {
> + CatalogMultiInsertWithInfo(sdepRel, slot, ntuples, indstate);
> + ntuples = 0;
> + }
> }
Too much copying again.
Greetings,
Andres Freund
On Tue, Nov 12, 2019 at 10:33:16AM -0800, Andres Freund wrote: > On 2019-11-12 16:25:06 +0100, Daniel Gustafsson wrote: >> On 11 Nov 2019, at 09:32, Michael Paquier <michael@paquier.xyz> wrote: >> >>> This part has resulted in 75c1921, and we could just change >>> DecodeMultiInsert() so as if there is no tuple data then we'd just >>> leave. However, I don't feel completely comfortable with that either >>> as it would be nice to still check that for normal relations we >>> *always* have a FPW available. >> >> XLH_INSERT_CONTAINS_NEW_TUPLE will only be set in case of catalog relations >> IIUC (that is, non logically decoded relations), so it seems to me that we can >> have a fastpath out of DecodeMultiInsert() which inspects that flag without >> problems. Is this proposal along the lines of what you were thinking? > > Maybe I'm missing something, but it's the opposite, no? > bool need_tuple_data = RelationIsLogicallyLogged(relation); Yep. This checks after IsCatalogRelation(). > ... > if (need_tuple_data) > xlrec->flags |= XLH_INSERT_CONTAINS_NEW_TUPLE; > > or am I misunderstanding what you mean? Not sure that I can think about a good way to properly track if the new tuple data is associated to a catalog or not, aka if the data is expected all the time or not to get a good sanity check when doing the multi-insert decoding. We could extend xl_multi_insert_tuple with a flag to track that, but that seems like an overkill for a simple sanity check.. -- Michael
Вложения
Hi, On 2019-11-13 15:58:28 +0900, Michael Paquier wrote: > On Tue, Nov 12, 2019 at 10:33:16AM -0800, Andres Freund wrote: > > On 2019-11-12 16:25:06 +0100, Daniel Gustafsson wrote: > >> On 11 Nov 2019, at 09:32, Michael Paquier <michael@paquier.xyz> wrote: > >> > >>> This part has resulted in 75c1921, and we could just change > >>> DecodeMultiInsert() so as if there is no tuple data then we'd just > >>> leave. However, I don't feel completely comfortable with that either > >>> as it would be nice to still check that for normal relations we > >>> *always* have a FPW available. > >> > >> XLH_INSERT_CONTAINS_NEW_TUPLE will only be set in case of catalog relations > >> IIUC (that is, non logically decoded relations), so it seems to me that we can > >> have a fastpath out of DecodeMultiInsert() which inspects that flag without > >> problems. Is this proposal along the lines of what you were thinking? > > > > Maybe I'm missing something, but it's the opposite, no? > > > bool need_tuple_data = RelationIsLogicallyLogged(relation); > > Yep. This checks after IsCatalogRelation(). > > > ... > > if (need_tuple_data) > > xlrec->flags |= XLH_INSERT_CONTAINS_NEW_TUPLE; > > > > or am I misunderstanding what you mean? > > Not sure that I can think about a good way to properly track if the > new tuple data is associated to a catalog or not, aka if the data is > expected all the time or not to get a good sanity check when doing the > multi-insert decoding. We could extend xl_multi_insert_tuple with a > flag to track that, but that seems like an overkill for a simple > sanity check.. My point was that I think there must be negation missing in Daniel's statement - XLH_INSERT_CONTAINS_NEW_TUPLE will only be set if *not* a catalog relation. But Daniel's statement says exactly the opposite, at least by my read. I can't follow what you're trying to get at in this sub discussion - why would we want to sanity check something about catalog tables here? Given that XLH_INSERT_CONTAINS_NEW_TUPLE is set exactly when the table is not a catalog table, that seems entirely superfluous? Greetings, Andres Freund
On Wed, Nov 13, 2019 at 05:55:12PM -0800, Andres Freund wrote:
> My point was that I think there must be negation missing in Daniel's
> statement - XLH_INSERT_CONTAINS_NEW_TUPLE will only be set if *not* a
> catalog relation. But Daniel's statement says exactly the opposite, at
> least by my read.
FWIW, I am reading the same, aka the sentence of Daniel is wrong. And
that what you say is right.
> I can't follow what you're trying to get at in this sub discussion - why
> would we want to sanity check something about catalog tables here? Given
> that XLH_INSERT_CONTAINS_NEW_TUPLE is set exactly when the table is not
> a catalog table, that seems entirely superfluous?
[ ... Looking ... ]
You are right, we could just rely on cross-checking that when we have
no data then XLH_INSERT_CONTAINS_NEW_TUPLE is not set, or something
like that:
@@ -901,11 +901,17 @@ DecodeMultiInsert(LogicalDecodingContext *ctx,
XLogRecordBuffer *buf)
return;
/*
- * As multi_insert is not used for catalogs yet, the block should always
- * have data even if a full-page write of it is taken.
+ * multi_insert can be used by catalogs, hence it is possible that
+ * the block does not have any data even if a full-page write of it
+ * is taken.
*/
tupledata = XLogRecGetBlockData(r, 0, &tuplelen);
- Assert(tupledata != NULL);
+ Assert(tupledata == NULL ||
+ (xlrec->flags & XLH_INSERT_CONTAINS_NEW_TUPLE) != 0);
+
+ /* No data, then leave */
+ if (tupledata == NULL)
+ return;
The latest patch does not apply, so it needs a rebase.
--
Michael
Вложения
> On 12 Nov 2019, at 19:33, Andres Freund <andres@anarazel.de> wrote: Thanks for reviewing! > On 2019-11-12 16:25:06 +0100, Daniel Gustafsson wrote: >>> On 11 Nov 2019, at 09:32, Michael Paquier <michael@paquier.xyz> wrote: >> >>> This part has resulted in 75c1921, and we could just change >>> DecodeMultiInsert() so as if there is no tuple data then we'd just >>> leave. However, I don't feel completely comfortable with that either >>> as it would be nice to still check that for normal relations we >>> *always* have a FPW available. >> >> XLH_INSERT_CONTAINS_NEW_TUPLE will only be set in case of catalog relations >> IIUC (that is, non logically decoded relations), so it seems to me that we can >> have a fastpath out of DecodeMultiInsert() which inspects that flag without >> problems. Is this proposal along the lines of what you were thinking? > > Maybe I'm missing something, but it's the opposite, no? > ... > or am I misunderstanding what you mean? Correct, as has been discussed in this thread already, I managed to write it backwards. >> @@ -1600,10 +1598,16 @@ recordDependencyOnExpr(const ObjectAddress *depender, >> /* Remove any duplicates */ >> eliminate_duplicate_dependencies(context.addrs); >> >> - /* And record 'em */ >> - recordMultipleDependencies(depender, >> - context.addrs->refs, context.addrs->numrefs, >> - behavior); >> + /* >> + * And record 'em. If we know we only have a single dependency then >> + * avoid the extra cost of setting up a multi insert. >> + */ >> + if (context.addrs->numrefs == 1) >> + recordDependencyOn(depender, &context.addrs->refs[0], behavior); >> + else >> + recordMultipleDependencies(depender, >> + context.addrs->refs, context.addrs->numrefs, >> + behavior); > > I'm not sure this is actually a worthwhile complexity to keep. Hard to > believe that setting up a multi-insert is goign to have a significant > enough overhead to matter here? > > And if it does, is there a chance we can hide this repeated block > somewhere within recordMultipleDependencies() or such? I don't like the > repetitiveness here. Seems recordMultipleDependencies() could easily > optimize the case of there being exactly one dependency to insert? Agreed, I've simplified by just calling recordMultipleDepencies() until that's found to be too expensive. >> + slot = palloc(sizeof(TupleTableSlot *) * natts); > > Hm. Looking at > > SELECT avg(pg_column_size(pa)) FROM pg_attribute pa; > > yielding ~144 bytes, we can probably cap this at 128 or such, without > loosing efficiency. Or just use > #define MAX_BUFFERED_BYTES 65535 > from copy.c or such (MAX_BUFFERED_BYTES / sizeof(FormData_pg_attribute)). Added a limit using MAX_BUFFERED_BYTES as above, or the number of tuples, whichever is smallest. >> + /* This is a tad tedious, but way cleaner than what we used to do... */ > > I don't like comments that refer to "what we used to" because there's no > way for anybody to make sense of that, because it's basically a dangling > reference :) This is a copy/paste from InsertPgAttributeTuple added in 03e5248d0f0, which in turn quite likely was a copy/paste from InsertPgClassTuple added in b7b78d24f7f back in 2006. Looking at that commit, it's clear what the comment refers to but it's quite useless in isolation. Since the current coding is its teens by now, perhaps we should just remove the two existing occurrences? I can submit a rewrite of the comments into something less gazing into the past if you feel like removing these. > This seems likely to waste some effort - during insertion the slot will > be materialized, which'll copy the tuple. I think it'd be better to > construct the tuple directly inside the slot's tts_values/isnull, and > then store it with ExecStoreVirtualTuple(). I'm not sure why it looked that way, but it's clearly rubbish. Rewrote by taking the TupleDesc as input (which addresses your other comment below too), and create the tuples directly by using ExecStoreVirtualTuple. >> + } >> + >> + /* finally insert the new tuples, update the indexes, and clean up */ >> + CatalogMultiInsertWithInfo(pg_attribute_rel, slot, natts, indstate); > > Previous comment: > > I think it might be worthwhile to clear and delete the slots > after inserting. That's potentially a good bit of memory, no? > > Current comment: > > I think I quite dislike the API of CatalogMultiInsertWithInfo freeing > the slots. It'd e.g. make it impossible to reuse them to insert more > data. It's also really hard to understand I don't have strong feelings, I see merit in both approches but the reuse aspect is clearly the winner. I've changed it such that the caller is responsible for freeing. >> + attrs = palloc(sizeof(Form_pg_attribute) * natts); > > Hm. Why we we need this separate allocation? Isn't this exactly the same > as what's in the tupledesc? Fixed. >> +/* >> + * CatalogMultiInsertWithInfo > > Hm. The current function is called CatalogTupleInsert(), wouldn't > CatalogTuplesInsertWithInfo() or such be more accurate? Or > CatalogTuplesMultiInsertWithInfo()? Fixed by opting for the latter, mostly since it seems best convey what the function does. > Same concern as in the equivalent pg_attribute code. > Too much copying again. Both of them fixed. The attached patch addresses all of the comments, thanks again for reviewing! cheers ./daniel
Вложения
On Sun, Nov 17, 2019 at 12:01:08AM +0100, Daniel Gustafsson wrote: > Fixed by opting for the latter, mostly since it seems best convey what the > function does. - recordMultipleDependencies(depender, - context.addrs->refs, context.addrs->numrefs, - behavior); + recordMultipleDependencies(depender, context.addrs->refs, + context.addrs->numrefs, behavior); Some noise diffs. --- a/src/test/regress/expected/create_index.out +++ b/src/test/regress/expected/create_index.out index concur_reindex_ind4 | column c1 of table - index concur_reindex_ind4 | column c1 of table index concur_reindex_ind4 | column c2 of table This removes a duplicated dependency with indexes using the same column multiple times. Guess that's good to get rid of, this was just unnecessary bloat in pg_depend. The regression tests of contrib/test_decoding are still failing here: +ERROR: could not resolve cmin/cmax of catalog tuple Getting rid the duplication between InsertPgAttributeTuples() and InsertPgAttributeTuple() would be nice. You would basically finish by just using a single slot when inserting one tuple.. -- Michael
Вложения
> On 26 Nov 2019, at 06:44, Michael Paquier <michael@paquier.xyz> wrote: Re this patch being in WoA state for some time [0]: > The regression tests of contrib/test_decoding are still failing here: > +ERROR: could not resolve cmin/cmax of catalog tuple This is the main thing left with this patch, and I've been unable so far to figure it out. I have an unscientific hunch that this patch is shaking out something (previously unexercised) in the logical decoding code supporting multi-inserts in the catalog. If anyone has ideas or insights, I would love the help. Once my plate clears up a bit I will return to this one, but feel free to mark it rwf for this cf. cheers ./daniel [0] postgr.es/m/20200121144937.n24oacjkegu4pnpe@development
On Wed, Jan 22, 2020 at 11:18:12PM +0100, Daniel Gustafsson wrote: > Once my plate clears up a bit I will return to this one, but feel free to mark > it rwf for this cf. Thanks for the update. I have switched the patch status to returned with feedback. -- Michael
Вложения
Hi, On 2020-01-22 23:18:12 +0100, Daniel Gustafsson wrote: > > On 26 Nov 2019, at 06:44, Michael Paquier <michael@paquier.xyz> wrote: > > Re this patch being in WoA state for some time [0]: > > > The regression tests of contrib/test_decoding are still failing here: > > +ERROR: could not resolve cmin/cmax of catalog tuple > > This is the main thing left with this patch, and I've been unable so far to > figure it out. I have an unscientific hunch that this patch is shaking out > something (previously unexercised) in the logical decoding code supporting > multi-inserts in the catalog. If anyone has ideas or insights, I would love > the help. Perhaps we can take a look at it together while at fosdem? Feel free to remind me... Greetings, Andres Freund
> On 26 Jan 2020, at 21:30, Andres Freund <andres@anarazel.de> wrote: > On 2020-01-22 23:18:12 +0100, Daniel Gustafsson wrote: >>> On 26 Nov 2019, at 06:44, Michael Paquier <michael@paquier.xyz> wrote: >> >> Re this patch being in WoA state for some time [0]: >> >>> The regression tests of contrib/test_decoding are still failing here: >>> +ERROR: could not resolve cmin/cmax of catalog tuple >> >> This is the main thing left with this patch, and I've been unable so far to >> figure it out. I have an unscientific hunch that this patch is shaking out >> something (previously unexercised) in the logical decoding code supporting >> multi-inserts in the catalog. If anyone has ideas or insights, I would love >> the help. > > Perhaps we can take a look at it together while at fosdem? Feel free to > remind me... Which we did, and after that I realized I had been looking at it from the wrong angle. Returning to it after coming home from FOSDEM I believe I have found the culprit. Turns out that we in heap_multi_insert missed to call log_heap_new_cid for the first tuple inserted, we only do it in the loop body for the subsequent ones. With the attached patch, the v6 of this patch posted upthead pass the tests for me. I have a v7 brewing which I'll submit shortly, but since this fix unrelated to that patchseries other than as a pre-requisite I figured I'd post that separately. cheers ./daniel
Вложения
On Sat, Feb 22, 2020 at 10:22:27PM +0100, Daniel Gustafsson wrote: > Turns out that we in heap_multi_insert missed to call log_heap_new_cid for the > first tuple inserted, we only do it in the loop body for the subsequent ones. > With the attached patch, the v6 of this patch posted upthead pass the tests for > me. I have a v7 brewing which I'll submit shortly, but since this fix > unrelated to that patchseries other than as a pre-requisite I figured I'd post > that separately. Good catch. I would not backpatch that as it is not a live bug because heap_multi_insert() is not used for catalogs yet. With your patch, that would be the case though.. -- Michael
Вложения
> On 23 Feb 2020, at 08:27, Michael Paquier <michael@paquier.xyz> wrote: > > On Sat, Feb 22, 2020 at 10:22:27PM +0100, Daniel Gustafsson wrote: >> Turns out that we in heap_multi_insert missed to call log_heap_new_cid for the >> first tuple inserted, we only do it in the loop body for the subsequent ones. >> With the attached patch, the v6 of this patch posted upthead pass the tests for >> me. I have a v7 brewing which I'll submit shortly, but since this fix >> unrelated to that patchseries other than as a pre-requisite I figured I'd post >> that separately. > > Good catch. I would not backpatch that as it is not a live bug > because heap_multi_insert() is not used for catalogs yet. With your > patch, that would be the case though.. I'll leave that call up to others, the bug is indeed unreachable with the current coding. Attached is a v7 of the catalog multi_insert patch which removes some code duplication which was previously commented on. There are still a few rouch edges but this version passes tests when paired with the heap_multi_insert cid patch. cheers ./daniel
Вложения
Hi, On 2020-02-23 16:27:57 +0900, Michael Paquier wrote: > On Sat, Feb 22, 2020 at 10:22:27PM +0100, Daniel Gustafsson wrote: > > Turns out that we in heap_multi_insert missed to call log_heap_new_cid for the > > first tuple inserted, we only do it in the loop body for the subsequent ones. > > With the attached patch, the v6 of this patch posted upthead pass the tests for > > me. I have a v7 brewing which I'll submit shortly, but since this fix > > unrelated to that patchseries other than as a pre-requisite I figured I'd post > > that separately. Thanks for finding! > Good catch. I would not backpatch that as it is not a live bug > because heap_multi_insert() is not used for catalogs yet. With your > patch, that would be the case though.. Thanks for pushing. I do find it a bit odd that you essentially duplicated the existing comment in a different phrasing. What's the point? Greetings, Andres Freund
On Mon, Feb 24, 2020 at 03:29:03PM -0800, Andres Freund wrote: > I do find it a bit odd that you essentially duplicated the existing > comment in a different phrasing. What's the point? A direct reference to catalog tuples is added for each code path calling log_heap_new_cid(), so that was more natural to me. -- Michael
Вложения
> On 25 Feb 2020, at 00:29, Andres Freund <andres@anarazel.de> wrote: > On 2020-02-23 16:27:57 +0900, Michael Paquier wrote: >> Good catch. I would not backpatch that as it is not a live bug >> because heap_multi_insert() is not used for catalogs yet. With your >> patch, that would be the case though.. > > Thanks for pushing. +1, thanks to the both of you for helping with the patch. Attached is a v8 of the patchset to make the cfbot happier, as the previous no longer applies. In doing that I realized that there is another hunk in this patch for fixing up logical decoding multi-insert support, which is independent of the patch in question here so I split it off. It's another issue which cause no harm at all today, but fails as soon as someone tries to perform multi inserts into the catalog. AFAICT, the current coding assumes that the multi-insert isn't from a catalog, and makes little attempts at surviving in case it is. The attached patch fixes that by taking a fast-path exit in case it is a catalog multi-insert, as there is nothing to decode. cheers ./daniel
Вложения
On Tue, Feb 25, 2020 at 10:44:40PM +0100, Daniel Gustafsson wrote: > In doing that I realized that there is another hunk in this patch for fixing up > logical decoding multi-insert support, which is independent of the patch in > question here so I split it off. It's another issue which cause no harm at all > today, but fails as soon as someone tries to perform multi inserts into the > catalog. Yep. I was wondering how we should do that for some time, but I was not able to come back to it. + /* + * CONTAINS_NEW_TUPLE will always be set unless the multi_insert was + * performed for a catalog. If it is a catalog, return immediately as + * there is nothing to logically decode. + */ + if (!(xlrec->flags & XLH_INSERT_CONTAINS_NEW_TUPLE)) + return; Hmm, OK. Consistency with DecodeInsert() is a good idea here, so count me in regarding the way your patch handles the problem. I would be fine to apply that part but, Andres, perhaps you would prefer taking care of it yourself? -- Michael
Вложения
On Fri, Feb 28, 2020 at 05:24:29PM +0900, Michael Paquier wrote:
> + /*
> + * CONTAINS_NEW_TUPLE will always be set unless the multi_insert was
> + * performed for a catalog. If it is a catalog, return immediately as
> + * there is nothing to logically decode.
> + */
> + if (!(xlrec->flags & XLH_INSERT_CONTAINS_NEW_TUPLE))
> + return;
> Hmm, OK. Consistency with DecodeInsert() is a good idea here, so
> count me in regarding the way your patch handles the problem. I would
> be fine to apply that part but, Andres, perhaps you would prefer
> taking care of it yourself?
Okay, I have applied this part.
One comment removal is missed in heapam.c (my fault, then).
+ * TODO this is defined in copy.c, if we want to use this to limit the number
+ * of slots in this patch, we need to figure out where to put it.
+ */
+#define MAX_BUFFERED_BYTES 65535
The use-case is different than copy.c, so aren't you looking at a
separate variable to handle that?
+reset_object_addresses(ObjectAddresses *addrs)
+{
+ if (addrs->numrefs == 0)
+ return;
Or just use an assert?
src/backend/commands/tablecmds.c: /* attribute.attacl is handled by
InsertPgAttributeTuple */
src/backend/catalog/heap.c: * This is a variant of
InsertPgAttributeTuple() which dynamically allocates
Those two references are not true anymore as your patch removes
InsertPgAttributeTuple() and replaces it by the plural flavor.
+/*
+ * InsertPgAttributeTuples
+ * Construct and insert multiple tuples in pg_attribute.
*
- * Caller has already opened and locked pg_attribute. new_attribute is the
- * attribute to insert. attcacheoff is always initialized to -1, attacl and
- * attoptions are always initialized to NULL.
- *
- * indstate is the index state for CatalogTupleInsertWithInfo. It can be
- * passed as NULL, in which case we'll fetch the necessary info. (Don't do
- * this when inserting multiple attributes, because it's a tad more
- * expensive.)
+ * This is a variant of InsertPgAttributeTuple() which dynamically allocates
+ * space for multiple tuples. Having two so similar functions is a kludge, but
+ * for now it's a TODO to make it less terrible.
And those comments largely need to remain around.
- attr = TupleDescAttr(tupdesc, i);
- /* Fill in the correct relation OID */
- attr->attrelid = new_rel_oid;
- /* Make sure this is OK, too */
- attr->attstattarget = -1;
-
- InsertPgAttributeTuple(rel, attr, indstate);
attstattarget handling is inconsistent here, no?
InsertPgAttributeTuples() does not touch this part, though your code
updates the attribute's attrelid.
- referenced.classId = RelationRelationId;
- referenced.objectId = heapRelationId;
- referenced.objectSubId = indexInfo->ii_IndexAttrNumbers[i];
-
- recordDependencyOn(&myself, &referenced, DEPENDENCY_AUTO);
-
+ add_object_address(OCLASS_CLASS, heapRelationId,
+ indexInfo->ii_IndexAttrNumbers[i],
+ refobjs);
Not convinced that we have to publish add_object_address() in the
headers, because we have already add_exact_object_address which is
able to do the same job. All code paths touched by your patch involve
already ObjectAddress objects, so switching to _exact_ creates less
code diffs.
+ if (ntuples == DEPEND_TUPLE_BUF)
+ {
+ CatalogTuplesMultiInsertWithInfo(sdepRel, slot, ntuples, indstate);
+ ntuples = 0;
+ }
Maybe this is a sufficient argument that we had better consider the
template copy as part of a separate patch, handled once the basics is
in place. It is not really obvious if 32 is a good thing, or not.
+ /* TODO is nreferenced a reasonable allocation of slots? */
+ slot = palloc(sizeof(TupleTableSlot *) * nreferenced);
I guess so.
/* construct new attribute's pg_attribute entry */
+ oldcontext = MemoryContextSwitchTo(CurTransactionContext);
This needs a comment as this change is not obvious for the reader.
And actually, why?
--
Michael
Вложения
> On 2 Mar 2020, at 03:06, Michael Paquier <michael@paquier.xyz> wrote:
Thanks a lot for another round of review, much appreciated!
> On Fri, Feb 28, 2020 at 05:24:29PM +0900, Michael Paquier wrote:
>> + /*
>> + * CONTAINS_NEW_TUPLE will always be set unless the multi_insert was
>> + * performed for a catalog. If it is a catalog, return immediately as
>> + * there is nothing to logically decode.
>> + */
>> + if (!(xlrec->flags & XLH_INSERT_CONTAINS_NEW_TUPLE))
>> + return;
>> Hmm, OK. Consistency with DecodeInsert() is a good idea here, so
>> count me in regarding the way your patch handles the problem. I would
>> be fine to apply that part but, Andres, perhaps you would prefer
>> taking care of it yourself?
>
> Okay, I have applied this part.
Thanks!
> +#define MAX_BUFFERED_BYTES 65535
> The use-case is different than copy.c, so aren't you looking at a
> separate variable to handle that?
Renamed to indicate current usecase.
> +reset_object_addresses(ObjectAddresses *addrs)
> +{
> + if (addrs->numrefs == 0)
> + return;
> Or just use an assert?
I don't see why we should prohibit calling reset_object_addresses so strongly,
it's nonsensical but is it wrong enough to Assert on? I can change it if you
feel it should be an assertion, but have left it for now.
> src/backend/commands/tablecmds.c: /* attribute.attacl is handled by
> InsertPgAttributeTuple */
> src/backend/catalog/heap.c: * This is a variant of
> InsertPgAttributeTuple() which dynamically allocates
> Those two references are not true anymore as your patch removes
> InsertPgAttributeTuple() and replaces it by the plural flavor.
Fixed.
> +/*
> + * InsertPgAttributeTuples
> ...
> And those comments largely need to remain around.
Fixed.
> attstattarget handling is inconsistent here, no?
Indeed it was, fixed.
> Not convinced that we have to publish add_object_address() in the
> headers, because we have already add_exact_object_address which is
> able to do the same job. All code paths touched by your patch involve
> already ObjectAddress objects, so switching to _exact_ creates less
> code diffs.
Good point, using _exact will make the changes smaller at the cost of additions
being larger. I do prefer the add_object_address API since it makes code more
readable IMO, but there is clear value in not exposing more functions. I've
changed to using add_exact_object_address in the attached version.
> + if (ntuples == DEPEND_TUPLE_BUF)
> + {
> + CatalogTuplesMultiInsertWithInfo(sdepRel, slot, ntuples, indstate);
> + ntuples = 0;
> + }
> Maybe this is a sufficient argument that we had better consider the
> template copy as part of a separate patch, handled once the basics is
> in place. It is not really obvious if 32 is a good thing, or not.
Fixed by instead avoiding the copying altogether and creating virtual tuples.
Andres commented on this upthread but I had missed fixing the code to do that,
so better late than never.
> /* construct new attribute's pg_attribute entry */
> + oldcontext = MemoryContextSwitchTo(CurTransactionContext);
> This needs a comment as this change is not obvious for the reader.
> And actually, why?
Thats a good question, this was a leftover from a different version the code
which I abandoned, but I missed removing the context handling. Sorry about the
sloppyness there. Removed.
The v9 patch attached addresses, I believe, all comments made to date. I tried
to read through earlier reviews too to ensure I hadn't missed anything so I
hope I've covered all findings so far.
cheers ./daniel
Вложения
> On 4 Mar 2020, at 23:16, Daniel Gustafsson <daniel@yesql.se> wrote: > The v9 patch attached addresses, I believe, all comments made to date. I tried > to read through earlier reviews too to ensure I hadn't missed anything so I > hope I've covered all findings so far. Attached is a rebased version which was updated to handle the changes for op class parameters introduced in 911e70207703799605. cheers ./daniel
Вложения
On Thu, Jun 25, 2020 at 09:38:23AM +0200, Daniel Gustafsson wrote: > Attached is a rebased version which was updated to handle the changes for op > class parameters introduced in 911e70207703799605. Thanks for the updated version. While re-reading the code, I got cold feet with the changes done in recordDependencyOn(). Sure, we could do as you propose, but that does not have to be part of this patch I think, aimed at switching more catalogs to use multi inserts, and it just feels a duplicate of recordMultipleDependencies(), with the same comments copied all over the place, etc. MAX_TEMPLATE_BYTES in pg_shdepend.c needs a comment to explain that this is to cap the number of slots used in copyTemplateDependencies() for pg_shdepend. Not much a fan of the changes in GenerateTypeDependencies(), particularly the use of refobjs[8], capped to the number of items from typeForm. If we add new members I think that this would break easily without us actually noticing that it broke. The use of ObjectAddressSet() is a good idea though, but it does not feel consistent if you don't the same coding rule to typbasetype, typcollation or typelem. I am also thinking to split this part of the cleanup in a first independent patch. pg_constraint.c, pg_operator.c, extension.c and pg_aggregate.c were using ObjectAddressSubSet() with subset set to 0 when registering a dependency. It is simpler to just use ObjectAddressSet(). As this updates the way dependencies are tracked and recorded, that's better if kept in the main patch. + /* TODO is nreferenced a reasonable allocation of slots? */ + slot = palloc(sizeof(TupleTableSlot *) * nreferenced); It seems to me that we could just apply the same rule as for pg_attribute and pg_shdepend, no? CatalogTupleInsertWithInfo() becomes mostly unused with this patch, its only caller being now LOs. Just noticing, I'd rather not remove it for now. The attached includes a bunch of modifications I have done while going through the patch (I indend to split and apply the changes of pg_type.c separately first, just lacked of time now to send a proper split), and there is the number of slots for pg_depend insertions that still needs to be addressed. On top of that pgindent has not been run yet. That's all I have for today, overall the patch is taking a committable shape :) -- Michael
Вложения
> On 26 Jun 2020, at 10:11, Michael Paquier <michael@paquier.xyz> wrote: > > On Thu, Jun 25, 2020 at 09:38:23AM +0200, Daniel Gustafsson wrote: >> Attached is a rebased version which was updated to handle the changes for op >> class parameters introduced in 911e70207703799605. > > Thanks for the updated version. Thanks for reviewing! > While re-reading the code, I got cold feet with the changes done in > recordDependencyOn(). Sure, we could do as you propose, but that does > not have to be part of this patch I think, aimed at switching more > catalogs to use multi inserts, and it just feels a duplicate of > recordMultipleDependencies(), with the same comments copied all over > the place, etc. Fair enough, I can take that to another patch later in the cycle. > MAX_TEMPLATE_BYTES in pg_shdepend.c needs a comment to explain that > this is to cap the number of slots used in > copyTemplateDependencies() for pg_shdepend. Agreed, +1 on the proposed wording. > Not much a fan of the changes in GenerateTypeDependencies(), > particularly the use of refobjs[8], capped to the number of items from > typeForm. If we add new members I think that this would break > easily without us actually noticing that it broke. Yeah, thats not good, it's better to leave that out. > The use of > ObjectAddressSet() is a good idea though, but it does not feel > consistent if you don't the same coding rule to typbasetype, > typcollation or typelem. I am also thinking to split this part of the > cleanup in a first independent patch. +1 on splitting into a separate patch. > pg_constraint.c, pg_operator.c, extension.c and pg_aggregate.c were > using ObjectAddressSubSet() with subset set to 0 when registering a > dependency. It is simpler to just use ObjectAddressSet(). Fair enough, either way, I don't have strong opinions. > As this > updates the way dependencies are tracked and recorded, that's better > if kept in the main patch. Agreed. > + /* TODO is nreferenced a reasonable allocation of slots? */ > + slot = palloc(sizeof(TupleTableSlot *) * nreferenced); > It seems to me that we could just apply the same rule as for > pg_attribute and pg_shdepend, no? I think so, I see no reason not to. > CatalogTupleInsertWithInfo() becomes mostly unused with this patch, > its only caller being now LOs. Just noticing, I'd rather not remove > it for now. Agreed, let's not bite off that too here, there's enough to chew on. > The attached includes a bunch of modifications I have done while going > through the patch (I indend to split and apply the changes of > pg_type.c separately first, just lacked of time now to send a proper > split), and there is the number of slots for pg_depend insertions that > still needs to be addressed. On top of that pgindent has not been run > yet. That's all I have for today, overall the patch is taking a > committable shape :) I like it, thanks for hacking on it. I will take another look at it later today when back at my laptop. cheers ./daniel
On Fri, Jun 26, 2020 at 02:26:50PM +0200, Daniel Gustafsson wrote: > On 26 Jun 2020, at 10:11, Michael Paquier <michael@paquier.xyz> wrote: >> + /* TODO is nreferenced a reasonable allocation of slots? */ >> + slot = palloc(sizeof(TupleTableSlot *) * nreferenced); >> It seems to me that we could just apply the same rule as for >> pg_attribute and pg_shdepend, no? > > I think so, I see no reason not to. I was actually looking a patch to potentially support REINDEX for partitioned table, and the CONCURRENTLY case may need this patch, still if a lot of dependencies are switched at once it may be a problem, so it is better to cap it. One thing though is that we may finish by allocating more slots than what's necessary if some dependencies are pinned, but using multi-inserts would be a gain anyway, and the patch does not insert in more slots than needed. > I like it, thanks for hacking on it. I will take another look at it later > today when back at my laptop. Thanks. I have been able to apply the independent part of pg_type.c as of 68de144. Attached is a rebased set, with more splitting work done after a new round of review. 0001 is more refactoring to use more ObjectAddress[Sub]Set() where we can, leading to some more cleanup: 5 files changed, 43 insertions(+), 120 deletions(-) In this round, I got confused with the way ObjectAddress items are assigned, assuming that we have to use the same dependency type for a bunch of dependencies to attach. Using add_exact_object_address() is fine for this purpose, but this also makes me wonder if we should try to think harder about this interface so as we would be able to insert a bunch of dependency tuples with multiple types of dependencies handled. So this has made me remove reset_object_addresses() from the patch series, as it was used when dependency types were mixed up. We could also discuss that separately, but that's not strongly mandatory here. There are however cases where it is straight-forward to gather and insert multiple records, like in InsertExtensionTuple() (as does already tsearchcmds.c), which is what 0002 does. An opposite example is StorePartitionKey(), where there is a mix of normal and internal dependencies, so I have removed it for now. 0003 is the main patch, where I have capped the number of slots used for pg_depend similarly what is done for pg_shdepend and pg_attribute to flush tuples in batches of 64kB. ExecDropSingleTupleTableSlot() was also called for an incorrect number of slots when it came to pg_shdepend. I was thinking if it could be possible to do more consolidation between the three places where we calculate the number of slots to use, but that would also mean to have more tuple slot dependency moving around, which is not great. Anyway, this leaves in patch 0003 only the multi-INSERT logic, without the pieces that manipulated the dependency recording and insertions (we still have three ObjectAddress[Sub]Set calls in heap.c but the same area of the code is reworked for attribute insertions). 0001 and 0002 are committable and independent pieces, while 0003 still needs more attention. One thing I also wanted to do with 0003 is measure the difference in WAL produced (say pg_shdepend when using the regression database as template) to have an idea of the gain. -- Michael
Вложения
> On 29 Jun 2020, at 08:57, Michael Paquier <michael@paquier.xyz> wrote: > > On Fri, Jun 26, 2020 at 02:26:50PM +0200, Daniel Gustafsson wrote: >> On 26 Jun 2020, at 10:11, Michael Paquier <michael@paquier.xyz> wrote: >>> + /* TODO is nreferenced a reasonable allocation of slots? */ >>> + slot = palloc(sizeof(TupleTableSlot *) * nreferenced); >>> It seems to me that we could just apply the same rule as for >>> pg_attribute and pg_shdepend, no? >> >> I think so, I see no reason not to. > > I was actually looking a patch to potentially support REINDEX for > partitioned table, and the CONCURRENTLY case may need this patch, > still if a lot of dependencies are switched at once it may be a > problem, so it is better to cap it. Agreed. > One thing though is that we may finish by allocating more slots than what's > necessary if some dependencies are pinned, but using multi-inserts would be a > gain anyway, and the patch does not insert in more slots than needed. I was playing around with a few approaches around this when I initially wrote this, but I was unable to find any way to calculate the correct number of slots which wasn't significantly more expensive than the extra allocations. > Attached is a rebased set, with more splitting work done after a new > round of review. 0001 is more refactoring to use more > ObjectAddress[Sub]Set() where we can, leading to some more cleanup: > 5 files changed, 43 insertions(+), 120 deletions(-) This patch seems quite straightforward, and in the case of the loops in for example CreateConstraintEntry() makes the code a lot more readable. +1 for applying 0001. > In this round, I got confused with the way ObjectAddress items are > assigned, assuming that we have to use the same dependency type for a > bunch of dependencies to attach. Using add_exact_object_address() is > fine for this purpose, but this also makes me wonder if we should try > to think harder about this interface so as we would be able to insert > a bunch of dependency tuples with multiple types of dependencies > handled. So this has made me remove reset_object_addresses() from the > patch series, as it was used when dependency types were mixed up. We > could also discuss that separately, but that's not strongly mandatory > here. Ok, once the final state of this patchset is known I can take a stab at recording multiple dependencies with different behaviors as a separate patchset. > There are however cases where it is straight-forward to gather and insert > multiple records, like in InsertExtensionTuple() (as does already > tsearchcmds.c), which is what 0002 does. +1 on 0002 as well. > An opposite example is StorePartitionKey(), where there is a mix of normal and > internal dependencies, so I have removed it for now. I don't think it makes the code any worse off to handle the NORMAL dependencies separate here, but MVV. > ExecDropSingleTupleTableSlot() was also called for an incorrect number > of slots when it came to pg_shdepend. Hmm, don't you mean in heap.c:InsertPgAttributeTuples()? > I was thinking if it could be possible to do more consolidation between the > three places where we calculate the number of slots to use, but that would also > mean to have more tuple slot dependency moving around, which is not great. If we do, we need to keep the cap consistent across all callers, else we'll end up with an API without an abstraction to make it worth more than saving a few lines of quite simple to read code. Currently this is the case, but that might not always hold, so not sure it if it's worth it. 0001 + 0002 + 0003 pass tests on my machine and nothing sticks out. cheers ./daniel
Re: Ought to use heap_multi_insert() for pg_attribute/depend insertions?
От
michael@paquier.xyz
Дата:
On Tue, Jun 30, 2020 at 02:25:07PM +0200, Daniel Gustafsson wrote: > Ok, once the final state of this patchset is known I can take a stab at > recording multiple dependencies with different behaviors as a separate > patchset. Thanks. I have applied 0001 and 0002 today, in a reversed order actually. > If we do, we need to keep the cap consistent across all callers, else we'll end > up with an API without an abstraction to make it worth more than saving a few > lines of quite simple to read code. Currently this is the case, but that might > not always hold, so not sure it if it's worth it. I am not sure either, still it looks worth thinking about it. Attached is a rebased version of the last patch. What this currently holds is just the switch to heap_multi_insert() for the three catalogs pg_attribute, pg_depend and pg_shdepend. One point that looks worth debating about is to how much to cap the data inserted at once. This uses 64kB for all three, with a number of slots chosen based on the size of each record, similarly to what we do for COPY. -- Michael
Вложения
On Wed, Jul 01, 2020 at 06:24:18PM +0900, Michael Paquier wrote:
> I am not sure either, still it looks worth thinking about it.
> Attached is a rebased version of the last patch. What this currently
> holds is just the switch to heap_multi_insert() for the three catalogs
> pg_attribute, pg_depend and pg_shdepend. One point that looks worth
> debating about is to how much to cap the data inserted at once. This
> uses 64kB for all three, with a number of slots chosen based on the
> size of each record, similarly to what we do for COPY.
I got an extra round of review done for this patch. One spot was
missed in heap_multi_insert() for a comment telling catalogs not using
multi inserts. After some consideration, I think that using 64kB as a
base number to calculate the number of slots should be fine, similarly
to COPY.
While on it, I have done some measurements to see the difference in
WAL produced and get an idea of the gain. For example, this function
would create one table with a wanted number of attributes:
CREATE OR REPLACE FUNCTION create_cols(tabname text, num_cols int)
RETURNS VOID AS
$func$
DECLARE
query text;
BEGIN
query := 'CREATE TABLE ' || tabname || ' (';
FOR i IN 1..num_cols LOOP
query := query || 'a_' || i::text || ' int';
IF i != num_cols THEN
query := query || ', ';
END IF;
END LOOP;
query := query || ')';
EXECUTE format(query);
END
$func$ LANGUAGE plpgsql;
On HEAD, with a table that has 1300 attributes, this leads to 563kB of
WAL produced. With the patch, we get down to 505kB. That's an
extreme case of course, but that's nice a nice gain.
A second test, after creating a database from a template that has
roughly 10k entries in pg_shdepend (10k empty tables actually), showed
a reduction from 2158kB to 1762kB in WAL.
Finally comes the third catalog, pg_depend, and there is one thing
that makes me itching about this part. We do a lot of useless work
for the allocation and destruction of the slots when there are pinned
dependencies, and there can be a lot of them. Just by running the
main regression tests, it is easy to see that in 0003 we still do a
lot of calls of recordMultipleDependencies() for one single
dependency, and that most of these are actually pinned. So we finish
by doing a lot of slot manipulation to insert nothing at the end,
contrary to the counterparts with pg_shdepend and pg_attribute. In
short, I think that for now it would be fine to commit a patch that
does the multi-INSERT optimization for pg_attribute and pg_shdepend,
but that we need more careful work for pg_depend. For example we
could go through all the dependencies first and recalculate the number
of slots to use depending on what is pinned or not, but this would
make sense actually when more dependencies are inserted at once in
more code paths, mostly for ALTER TABLE. So this needs more
consideration IMO.
Thoughts?
--
Michael
Вложения
> On 29 Jul 2020, at 10:32, Michael Paquier <michael@paquier.xyz> wrote: > > On Wed, Jul 01, 2020 at 06:24:18PM +0900, Michael Paquier wrote: >> I am not sure either, still it looks worth thinking about it. >> Attached is a rebased version of the last patch. What this currently >> holds is just the switch to heap_multi_insert() for the three catalogs >> pg_attribute, pg_depend and pg_shdepend. One point that looks worth >> debating about is to how much to cap the data inserted at once. This >> uses 64kB for all three, with a number of slots chosen based on the >> size of each record, similarly to what we do for COPY. > > I got an extra round of review done for this patch. Thanks! > While on it, I have done some measurements to see the difference in > WAL produced and get an idea of the gain. > On HEAD, with a table that has 1300 attributes, this leads to 563kB of > WAL produced. With the patch, we get down to 505kB. That's an > extreme case of course, but that's nice a nice gain. > > A second test, after creating a database from a template that has > roughly 10k entries in pg_shdepend (10k empty tables actually), showed > a reduction from 2158kB to 1762kB in WAL. Extreme cases for sure, but more importantly, there should be no cases when this would cause an increase wrt the status quo. > Finally comes the third catalog, pg_depend, and there is one thing > that makes me itching about this part. We do a lot of useless work > for the allocation and destruction of the slots when there are pinned > dependencies, and there can be a lot of them. Just by running the > main regression tests, it is easy to see that in 0003 we still do a > lot of calls of recordMultipleDependencies() for one single > dependency, and that most of these are actually pinned. So we finish > by doing a lot of slot manipulation to insert nothing at the end, > contrary to the counterparts with pg_shdepend and pg_attribute. Maybe it'd be worth pre-computing by a first pass which tracks pinned objects in a bitmap; with a second pass which then knows how many and which to insert into slots? > In > short, I think that for now it would be fine to commit a patch that > does the multi-INSERT optimization for pg_attribute and pg_shdepend, > but that we need more careful work for pg_depend. Fair enough, let's break out pg_depend and I'll have another go at that. cheers ./daniel
On Wed, Jul 29, 2020 at 11:34:07PM +0200, Daniel Gustafsson wrote: > Extreme cases for sure, but more importantly, there should be no cases when > this would cause an increase wrt the status quo. Yep. > Maybe it'd be worth pre-computing by a first pass which tracks pinned objects > in a bitmap; with a second pass which then knows how many and which to insert > into slots? Or it could be possible to just rebuild a new list of dependencies before insertion into the catalog. No objections with a bitmap, any approach would be fine here as long as there is a first pass on the item list. > Fair enough, let's break out pg_depend and I'll have another go at that. Thanks. Attached is a polished version of the patch that I intend to commit for pg_attribute and pg_shdepend. Let's look again at pg_depend later, as there are also links with the handling of dependencies for ALTER TABLE mainly. -- Michael
Вложения
> On 30 Jul 2020, at 03:28, Michael Paquier <michael@paquier.xyz> wrote: > On Wed, Jul 29, 2020 at 11:34:07PM +0200, Daniel Gustafsson wrote: >> Fair enough, let's break out pg_depend and I'll have another go at that. > > Thanks. Attached is a polished version of the patch that I intend to > commit for pg_attribute and pg_shdepend. Let's look again at > pg_depend later, as there are also links with the handling of > dependencies for ALTER TABLE mainly. Looks good, thanks. Let's close the CF entry with this and open a new for the pg_depend part when that's done. cheers ./daniel
On Thu, Jul 30, 2020 at 11:23:38PM +0200, Daniel Gustafsson wrote: > Looks good, thanks. Let's close the CF entry with this and open a new for the > pg_depend part when that's done. I have applied the patch, thanks. And actually, I have found just after that CREATE DATABASE gets severely impacted by the number of slots initialized when copying the template dependencies if there are few of them. The fix is as simple as delaying the initialization of the slots once we know they will be used. In my initial tests, I was using fsync = off, so I missed that. Sorry about that. The attached fixes this regression by delaying the slot initialization until we know that it will be used. This does not matter for pg_attribute as we know in advance the number of attributes to insert. -- Michael
Вложения
> On 31 Jul 2020, at 04:42, Michael Paquier <michael@paquier.xyz> wrote: > > On Thu, Jul 30, 2020 at 11:23:38PM +0200, Daniel Gustafsson wrote: >> Looks good, thanks. Let's close the CF entry with this and open a new for the >> pg_depend part when that's done. > > I have applied the patch, thanks. > > And actually, I have found just after that CREATE DATABASE gets > severely impacted by the number of slots initialized when copying the > template dependencies if there are few of them. The fix is as simple > as delaying the initialization of the slots once we know they will be > used. In my initial tests, I was using fsync = off, so I missed that. > Sorry about that. The attached fixes this regression by delaying the > slot initialization until we know that it will be used. This does not > matter for pg_attribute as we know in advance the number of attributes > to insert. Right, that makes sense. Sorry for missing that, and thanks for fixing. cheers ./daniel
Hi, On 2020-07-30 23:23:38 +0200, Daniel Gustafsson wrote: > > On 30 Jul 2020, at 03:28, Michael Paquier <michael@paquier.xyz> wrote: > > On Wed, Jul 29, 2020 at 11:34:07PM +0200, Daniel Gustafsson wrote: > > >> Fair enough, let's break out pg_depend and I'll have another go at that. > > > > Thanks. Attached is a polished version of the patch that I intend to > > commit for pg_attribute and pg_shdepend. Let's look again at > > pg_depend later, as there are also links with the handling of > > dependencies for ALTER TABLE mainly. > > Looks good, thanks. Nice work! Greetings, Andres Freund