Thread
Commits
-
tableam: Rename wrapper functions to match callback names.
- 73b8c3bd2889 12.0 landed
-
Inconsistency between table am callback and table function names
Ashwin Agrawal <aagrawal@pivotal.io> — 2019-05-08T07:32:22Z
The general theme for table function names seem to be "table_<am_callback_name>". For example table_scan_getnextslot() and its corresponding callback scan_getnextslot(). Most of the table functions and callbacks follow mentioned convention except following ones table_beginscan table_endscan table_rescan table_fetch_row_version table_get_latest_tid table_insert table_insert_speculative table_complete_speculative table_delete table_update table_lock_tuple the corresponding callback names for them are scan_begin scan_end scan_rescan tuple_fetch_row_version tuple_get_latest_tid tuple_insert tuple_insert_speculative tuple_delete tuple_update tuple_lock It confuses while browsing through the code and hence I would like to propose we make them consistent. Either fix the callback names or table functions but all should follow the same convention, makes it easy to browse around and refer to as well. Personally, I would say fix the table function names as callback names seem fine. So, for example, make it table_scan_begin(). Also, some of these table function names read little odd table_relation_set_new_filenode table_relation_nontransactional_truncate table_relation_copy_data table_relation_copy_for_cluster table_relation_vacuum table_relation_estimate_size Can we drop relation word from callback names and as a result from these function names as well? Just have callback names as set_new_filenode, copy_data, estimate_size? Also, a question about comments. Currently, redundant comments are written above callback functions as also above table functions. They differ sometimes a little bit on descriptions but majority content still being the same. Should we have only one place finalized to have the comments to keep them in sync and also know which one to refer to? Plus, file name amapi.h now seems very broad, if possible should be renamed to indexamapi.h or indexam.h to follow tableam.h. No idea what's our policy around header file renames.
-
Re: Inconsistency between table am callback and table function names
Andres Freund <andres@anarazel.de> — 2019-05-08T21:51:35Z
Hi, On 2019-05-08 00:32:22 -0700, Ashwin Agrawal wrote: > The general theme for table function names seem to be > "table_<am_callback_name>". For example table_scan_getnextslot() and its > corresponding callback scan_getnextslot(). Most of the table functions and > callbacks follow mentioned convention except following ones > > table_beginscan > table_endscan > table_rescan > table_fetch_row_version > table_get_latest_tid > table_insert > table_insert_speculative > table_complete_speculative > table_delete > table_update > table_lock_tuple > > the corresponding callback names for them are > > scan_begin > scan_end > scan_rescan The mismatch here is just due of backward compat with the existing function names. > tuple_fetch_row_version > tuple_get_latest_tid Hm, I'd not object to adding a tuple_ to the wrapper. > tuple_insert > tuple_insert_speculative > tuple_delete > tuple_update > tuple_lock That again is to keep the naming similar to the existing functions. > Also, some of these table function names read little odd > > table_relation_set_new_filenode > table_relation_nontransactional_truncate > table_relation_copy_data > table_relation_copy_for_cluster > table_relation_vacuum > table_relation_estimate_size > > Can we drop relation word from callback names and as a result from these > function names as well? Just have callback names as set_new_filenode, > copy_data, estimate_size? I'm strongly against that. These all work on a full relation size, rather than on individual tuples, and that seems worth pointing out. > Also, a question about comments. Currently, redundant comments are written > above callback functions as also above table functions. They differ > sometimes a little bit on descriptions but majority content still being the > same. Should we have only one place finalized to have the comments to keep > them in sync and also know which one to refer to? Note that the non-differing comments usually just refer to the other place. And there's legitimate differences in most of the ones that are both at the callback and the external functions - since the audience of both are difference, that IMO makes sense. > Plus, file name amapi.h now seems very broad, if possible should be renamed > to indexamapi.h or indexam.h to follow tableam.h. No idea what's our policy > around header file renames. We probably should rename it, but not in 12... Greetings, Andres Freund
-
Re: Inconsistency between table am callback and table function names
Ashwin Agrawal <aagrawal@pivotal.io> — 2019-05-09T00:05:07Z
On Wed, May 8, 2019 at 2:51 PM Andres Freund <andres@anarazel.de> wrote: > > Hi, > > On 2019-05-08 00:32:22 -0700, Ashwin Agrawal wrote: > > The general theme for table function names seem to be > > "table_<am_callback_name>". For example table_scan_getnextslot() and its > > corresponding callback scan_getnextslot(). Most of the table functions and > > callbacks follow mentioned convention except following ones > > > > table_beginscan > > table_endscan > > table_rescan > > table_fetch_row_version > > table_get_latest_tid > > table_insert > > table_insert_speculative > > table_complete_speculative > > table_delete > > table_update > > table_lock_tuple > > > > the corresponding callback names for them are > > > > scan_begin > > scan_end > > scan_rescan > > The mismatch here is just due of backward compat with the existing > function names. I am missing something here, would like to know more. table_ seem all new fresh naming. Hence IMO having consistency with surrounding and related code carries more weight as I don't know backward compat serving what purpose. Heap function names can continue to call with same old names for backward compat if required. > > Also, a question about comments. Currently, redundant comments are written > > above callback functions as also above table functions. They differ > > sometimes a little bit on descriptions but majority content still being the > > same. Should we have only one place finalized to have the comments to keep > > them in sync and also know which one to refer to? > > Note that the non-differing comments usually just refer to the other > place. And there's legitimate differences in most of the ones that are > both at the callback and the external functions - since the audience of > both are difference, that IMO makes sense. > Not having consistency is the main aspect I wish to bring to attention. Like for some callback functions the comment is /* see table_insert() for reference about parameters */ void (*tuple_insert) (Relation rel, TupleTableSlot *slot, CommandId cid, int options, struct BulkInsertStateData *bistate); /* see table_insert_speculative() for reference about parameters */ void (*tuple_insert_speculative) (Relation rel, TupleTableSlot *slot, CommandId cid, int options, struct BulkInsertStateData *bistate, uint32 specToken); Whereas for some other callbacks the parameter explanation exist in both the places. Seems we should be consistent. I feel in long run becomes pain to keep them in sync as comments evolve. Like for example /* * Estimate the size of shared memory needed for a parallel scan of this * relation. The snapshot does not need to be accounted for. */ Size (*parallelscan_estimate) (Relation rel); parallescan_estimate is not having snapshot argument passed to it, but table_parallescan_estimate does. So, this way chances are high they going out of sync and missing to modify in both the places. Agree though on audience being different for both. So, seems going with the refer XXX for parameters seems fine approach for all the callbacks and then only specific things to flag out for the AM layer to be aware can live above the callback function. > > Plus, file name amapi.h now seems very broad, if possible should be renamed > > to indexamapi.h or indexam.h to follow tableam.h. No idea what's our policy > > around header file renames. > > We probably should rename it, but not in 12... Okay good to know. -
Re: Inconsistency between table am callback and table function names
Andres Freund <andres@anarazel.de> — 2019-05-09T14:34:15Z
Hi, On 2019-05-08 17:05:07 -0700, Ashwin Agrawal wrote: > On Wed, May 8, 2019 at 2:51 PM Andres Freund <andres@anarazel.de> wrote: > > On 2019-05-08 00:32:22 -0700, Ashwin Agrawal wrote: > > > The general theme for table function names seem to be > > > "table_<am_callback_name>". For example table_scan_getnextslot() and its > > > corresponding callback scan_getnextslot(). Most of the table functions and > > > callbacks follow mentioned convention except following ones > > > > > > table_beginscan > > > table_endscan > > > table_rescan > > > table_fetch_row_version > > > table_get_latest_tid > > > table_insert > > > table_insert_speculative > > > table_complete_speculative > > > table_delete > > > table_update > > > table_lock_tuple > > > > > > the corresponding callback names for them are > > > > > > scan_begin > > > scan_end > > > scan_rescan > > > > The mismatch here is just due of backward compat with the existing > > function names. > > I am missing something here, would like to know more. table_ seem all > new fresh naming. Hence IMO having consistency with surrounding and > related code carries more weight as I don't know backward compat > serving what purpose. Heap function names can continue to call with > same old names for backward compat if required. The changes necessary for tableam were already huge. Changing naming schemes for functions that are used all over the backend (e.g. ~80 calls to table_beginscan), and where there's other wrapper functions that also widely used (237 calls to systable_beginscan) which didn't have to be touched, at the same time would have made it even harder to review. Greetings, Andres Freund
-
Re: Inconsistency between table am callback and table function names
Ashwin Agrawal <aagrawal@pivotal.io> — 2019-05-10T17:43:44Z
On Thu, May 9, 2019 at 8:52 AM Andres Freund <andres@anarazel.de> wrote: > The changes necessary for tableam were already huge. Changing naming > schemes for functions that are used all over the backend (e.g. ~80 calls > to table_beginscan), and where there's other wrapper functions that also > widely used (237 calls to systable_beginscan) which didn't have to be > touched, at the same time would have made it even harder to review. If there are no objections to renaming now, as separate independent patch, I am happy to do the same and send it across. Will rename to make it consistent as mentioned at start of the thread leaving table_relation_xxx() ones as is today.
-
Re: Inconsistency between table am callback and table function names
Andres Freund <andres@anarazel.de> — 2019-05-10T17:51:41Z
Hi, On 2019-05-10 10:43:44 -0700, Ashwin Agrawal wrote: > On Thu, May 9, 2019 at 8:52 AM Andres Freund <andres@anarazel.de> wrote: > > The changes necessary for tableam were already huge. Changing naming > > schemes for functions that are used all over the backend (e.g. ~80 calls > > to table_beginscan), and where there's other wrapper functions that also > > widely used (237 calls to systable_beginscan) which didn't have to be > > touched, at the same time would have made it even harder to review. > > If there are no objections to renaming now, as separate independent > patch, I am happy to do the same and send it across. Will rename to > make it consistent as mentioned at start of the thread leaving > table_relation_xxx() ones as is today. What would you want to rename precisely? Don't think it's useful to start sending patches before we agree on something concrete. I'm not on board with patching hundreds systable_beginscan calls (that'll break a lot of external code, besides the churn of in-core code), nor with the APIs around that having a diverging name scheme. Greetings, Andres Freund
-
Re: Inconsistency between table am callback and table function names
Ashwin Agrawal <aagrawal@pivotal.io> — 2019-05-10T19:43:06Z
On Fri, May 10, 2019 at 10:51 AM Andres Freund <andres@anarazel.de> wrote: > > Hi, > > On 2019-05-10 10:43:44 -0700, Ashwin Agrawal wrote: > > On Thu, May 9, 2019 at 8:52 AM Andres Freund <andres@anarazel.de> wrote: > > > The changes necessary for tableam were already huge. Changing naming > > > schemes for functions that are used all over the backend (e.g. ~80 calls > > > to table_beginscan), and where there's other wrapper functions that also > > > widely used (237 calls to systable_beginscan) which didn't have to be > > > touched, at the same time would have made it even harder to review. > > > > If there are no objections to renaming now, as separate independent > > patch, I am happy to do the same and send it across. Will rename to > > make it consistent as mentioned at start of the thread leaving > > table_relation_xxx() ones as is today. > > What would you want to rename precisely? Don't think it's useful to > start sending patches before we agree on something concrete. I'm not on > board with patching hundreds systable_beginscan calls (that'll break a > lot of external code, besides the churn of in-core code), nor with the > APIs around that having a diverging name scheme. Meant to stick the question mark in that email, somehow missed. Yes not planning to spend any time on it if objections. Here is the list of renames I wish to perform. Lets start with low hanging ones. table_rescan -> table_scan_rescan git grep table_rescan | wc -l 6 table_insert -> table_tuple_insert git grep tuple_insert | wc -l 13 table_insert_speculative -> table_tuple_insert_speculative git grep tuple_insert_speculative | wc -l 5 table_delete -> table_tuple_delete (table_delete reads incorrect as not deleting the table) git grep tuple_delete | wc -l 8 table_update -> table_tuple_update git grep tuple_update | wc -l 5 table_lock_tuple -> table_tuple_lock git grep tuple_lock | wc -l 26 Below two you already mentioned no objections to rename table_fetch_row_version -> table_tuple_fetch_row_version table_get_latest_tid -> table_tuple_get_latest_tid Now, table_beginscan and table_endscan are the ones which are wide-spread. Desire seems we should keep it consistent with systable_beginscan. Understand the constraints and churn aspect, given that diverging naming scheme is unavoidable. Why not leave systable_beginscan as it is and only rename table_beginscan and its counterparts table_beginscan_xxx() atleast? Index interfaces and table interfaces have some diverged naming scheme already like index_getnext_slot and table_scan_getnextslot. Not proposing to change them. But at least reducing wherever possible sooner would be helpful.
-
Re: Inconsistency between table am callback and table function names
Andres Freund <andres@anarazel.de> — 2019-05-10T19:50:49Z
Hi, On 2019-05-10 12:43:06 -0700, Ashwin Agrawal wrote: > On Fri, May 10, 2019 at 10:51 AM Andres Freund <andres@anarazel.de> wrote: > > > > Hi, > > > > On 2019-05-10 10:43:44 -0700, Ashwin Agrawal wrote: > > > On Thu, May 9, 2019 at 8:52 AM Andres Freund <andres@anarazel.de> wrote: > > > > The changes necessary for tableam were already huge. Changing naming > > > > schemes for functions that are used all over the backend (e.g. ~80 calls > > > > to table_beginscan), and where there's other wrapper functions that also > > > > widely used (237 calls to systable_beginscan) which didn't have to be > > > > touched, at the same time would have made it even harder to review. > > > > > > If there are no objections to renaming now, as separate independent > > > patch, I am happy to do the same and send it across. Will rename to > > > make it consistent as mentioned at start of the thread leaving > > > table_relation_xxx() ones as is today. > > > > What would you want to rename precisely? Don't think it's useful to > > start sending patches before we agree on something concrete. I'm not on > > board with patching hundreds systable_beginscan calls (that'll break a > > lot of external code, besides the churn of in-core code), nor with the > > APIs around that having a diverging name scheme. > > Meant to stick the question mark in that email, somehow missed. Yes > not planning to spend any time on it if objections. Here is the list > of renames I wish to perform. > > Lets start with low hanging ones. > > table_rescan -> table_scan_rescan > git grep table_rescan | wc -l > 6 > > table_insert -> table_tuple_insert > git grep tuple_insert | wc -l > 13 > > table_insert_speculative -> table_tuple_insert_speculative > git grep tuple_insert_speculative | wc -l > 5 > > table_delete -> table_tuple_delete (table_delete reads incorrect as > not deleting the table) > git grep tuple_delete | wc -l > 8 > > table_update -> table_tuple_update > git grep tuple_update | wc -l > 5 > > table_lock_tuple -> table_tuple_lock > git grep tuple_lock | wc -l > 26 > > > Below two you already mentioned no objections to rename > table_fetch_row_version -> table_tuple_fetch_row_version > table_get_latest_tid -> table_tuple_get_latest_tid > > > Now, table_beginscan and table_endscan are the ones which are > wide-spread. Desire seems we should keep it consistent with > systable_beginscan. Understand the constraints and churn aspect, given > that diverging naming scheme is unavoidable. Why not leave > systable_beginscan as it is and only rename table_beginscan and its > counterparts table_beginscan_xxx() atleast? > > Index interfaces and table interfaces have some diverged naming scheme > already like index_getnext_slot and table_scan_getnextslot. Not > proposing to change them. But at least reducing wherever possible > sooner would be helpful. My personal opinion is that this is more churn than I think is useful to tackle after feature freeze, with not sufficient benefits. If others chime in, voting to do this, I'm OK with doing that, but otherwise I think there's more important stuff to do. Greetings, Andres Freund
-
Re: Inconsistency between table am callback and table function names
Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-05-10T20:18:32Z
On 2019-May-10, Andres Freund wrote: > My personal opinion is that this is more churn than I think is useful to > tackle after feature freeze, with not sufficient benefits. If others > chime in, voting to do this, I'm OK with doing that, but otherwise I > think there's more important stuff to do. One issue is that if we don't change things now, we can never change it afterwards, so we should make some effort to ensure that naming is sensible. And we already changed the names of the whole interface. I'm not voting to accept all of Ashwin's proposals right away, only to have the names considered. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
-
Re: Inconsistency between table am callback and table function names
Andres Freund <andres@anarazel.de> — 2019-05-10T20:26:47Z
Hi, On 2019-05-10 16:18:32 -0400, Alvaro Herrera wrote: > On 2019-May-10, Andres Freund wrote: > > > My personal opinion is that this is more churn than I think is useful to > > tackle after feature freeze, with not sufficient benefits. If others > > chime in, voting to do this, I'm OK with doing that, but otherwise I > > think there's more important stuff to do. > > One issue is that if we don't change things now, we can never change it > afterwards, so we should make some effort to ensure that naming is > sensible. And we already changed the names of the whole interface. Well, the point is that there's symmetry with a lot of similar functions that were *not* affected by the tableam changes. Cf. systable_beginscan et al. We could add wrappers etc to make it less painful, but then there's no urgency either. Greetings, Andres Freund
-
Re: Inconsistency between table am callback and table function names
Tom Lane <tgl@sss.pgh.pa.us> — 2019-05-10T20:28:32Z
Alvaro Herrera <alvherre@2ndquadrant.com> writes: > On 2019-May-10, Andres Freund wrote: >> My personal opinion is that this is more churn than I think is useful to >> tackle after feature freeze, with not sufficient benefits. If others >> chime in, voting to do this, I'm OK with doing that, but otherwise I >> think there's more important stuff to do. > One issue is that if we don't change things now, we can never change it > afterwards, so we should make some effort to ensure that naming is > sensible. And we already changed the names of the whole interface. Yeah. I do not have an opinion on whether these changes are actually improvements, but renaming right now is way less painful than it would be to rename post-v12. Let's try to get it right the first time, especially with functions we already renamed in this cycle. I do think that the "too much churn" argument has merit for places that were *not* already changed in v12. In particular I'd vote against renaming the systable_xxx functions. regards, tom lane
-
Re: Inconsistency between table am callback and table function names
Robert Haas <robertmhaas@gmail.com> — 2019-05-13T19:50:56Z
On Fri, May 10, 2019 at 3:43 PM Ashwin Agrawal <aagrawal@pivotal.io> wrote: > Meant to stick the question mark in that email, somehow missed. Yes > not planning to spend any time on it if objections. Here is the list > of renames I wish to perform. > > Lets start with low hanging ones. > > table_rescan -> table_scan_rescan > table_insert -> table_tuple_insert > table_insert_speculative -> table_tuple_insert_speculative > table_delete -> table_tuple_delete > table_update -> table_tuple_update > table_lock_tuple -> table_tuple_lock > > Below two you already mentioned no objections to rename > table_fetch_row_version -> table_tuple_fetch_row_version > table_get_latest_tid -> table_tuple_get_latest_tid > > Now, table_beginscan and table_endscan are the ones which are > wide-spread. I vote to rename all the ones where the new name would contain "tuple" and to leave the others alone. i.e. leave table_beginscan, table_endscan, and table_rescan as they are. I think that there's little benefit in standardizing table_rescan but not the other two, and we seem to agree that tinkering with the other two gets into a painful amount of churn. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
-
Re: Inconsistency between table am callback and table function names
Ashwin Agrawal <aagrawal@pivotal.io> — 2019-05-14T19:11:46Z
On Mon, May 13, 2019 at 12:51 PM Robert Haas <robertmhaas@gmail.com> wrote: > On Fri, May 10, 2019 at 3:43 PM Ashwin Agrawal <aagrawal@pivotal.io> > wrote: > > Meant to stick the question mark in that email, somehow missed. Yes > > not planning to spend any time on it if objections. Here is the list > > of renames I wish to perform. > > > > Lets start with low hanging ones. > > > > table_rescan -> table_scan_rescan > > table_insert -> table_tuple_insert > > table_insert_speculative -> table_tuple_insert_speculative > > table_delete -> table_tuple_delete > > table_update -> table_tuple_update > > table_lock_tuple -> table_tuple_lock > > > > Below two you already mentioned no objections to rename > > table_fetch_row_version -> table_tuple_fetch_row_version > > table_get_latest_tid -> table_tuple_get_latest_tid > > > > Now, table_beginscan and table_endscan are the ones which are > > wide-spread. > > I vote to rename all the ones where the new name would contain "tuple" > and to leave the others alone. i.e. leave table_beginscan, > table_endscan, and table_rescan as they are. I think that there's > little benefit in standardizing table_rescan but not the other two, > and we seem to agree that tinkering with the other two gets into a > painful amount of churn. > Thank you. Please find the patch to rename the agreed functions. It would be good to make all consistent instead of applying exception to three functions but seems no consensus on it. Given table_ api are new, we could modify them leaving systable_ ones as is, but as objections left that as is.
-
Re: Inconsistency between table am callback and table function names
Ashwin Agrawal <aagrawal@pivotal.io> — 2019-05-14T19:17:35Z
On Wed, May 8, 2019 at 5:05 PM Ashwin Agrawal <aagrawal@pivotal.io> wrote: > Not having consistency is the main aspect I wish to bring to > attention. Like for some callback functions the comment is > > /* see table_insert() for reference about parameters */ > void (*tuple_insert) (Relation rel, TupleTableSlot *slot, > CommandId cid, int options, > struct BulkInsertStateData *bistate); > > /* see table_insert_speculative() for reference about parameters > */ > void (*tuple_insert_speculative) (Relation rel, > TupleTableSlot *slot, > CommandId cid, > int options, > struct > BulkInsertStateData *bistate, > uint32 specToken); > > Whereas for some other callbacks the parameter explanation exist in > both the places. Seems we should be consistent. > I feel in long run becomes pain to keep them in sync as comments > evolve. Like for example > > /* > * Estimate the size of shared memory needed for a parallel scan > of this > * relation. The snapshot does not need to be accounted for. > */ > Size (*parallelscan_estimate) (Relation rel); > > parallescan_estimate is not having snapshot argument passed to it, but > table_parallescan_estimate does. So, this way chances are high they > going out of sync and missing to modify in both the places. Agree > though on audience being different for both. So, seems going with the > refer XXX for parameters seems fine approach for all the callbacks and > then only specific things to flag out for the AM layer to be aware can > live above the callback function. > The topic of consistency for comment style for all wrappers seems got lost with other discussion, would like to seek opinion on the same.
-
Re: Inconsistency between table am callback and table function names
Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-05-14T20:27:47Z
On 2019-May-14, Ashwin Agrawal wrote: > Thank you. Please find the patch to rename the agreed functions. It would > be good to make all consistent instead of applying exception to three > functions but seems no consensus on it. Given table_ api are new, we could > modify them leaving systable_ ones as is, but as objections left that as is. Hmm .. I'm surprised to find out that we only have one caller of simple_table_insert, simple_table_delete, simple_table_update. I'm not sure I agree to the new names those got in the renaming patch, since they're not really part of table AM proper ... do we really want to offer those as separate entry points? Why not just remove those routines? Somewhat related: it's strange that CatalogTupleUpdate etc use simple_heap_update instead of the tableam variants wrappers (I suppose that's either because of bootstrapping concerns, or performance). Would it be too strange to have CatalogTupleInsert call heap_insert() directly, and do away with simple_heap_insert? (Equivalently for update, delete). I think those wrappers made perfect sense when we had simple_heap_insert all around the place ... but now that we introduced the CatalogTupleFoo wrappers, I don't think it does any longer. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
-
Re: Inconsistency between table am callback and table function names
Andres Freund <andres@anarazel.de> — 2019-05-14T20:37:14Z
Hi, On 2019-05-14 16:27:47 -0400, Alvaro Herrera wrote: > On 2019-May-14, Ashwin Agrawal wrote: > > > Thank you. Please find the patch to rename the agreed functions. It would > > be good to make all consistent instead of applying exception to three > > functions but seems no consensus on it. Given table_ api are new, we could > > modify them leaving systable_ ones as is, but as objections left that as is. > > Hmm .. I'm surprised to find out that we only have one caller of > simple_table_insert, simple_table_delete, simple_table_update. I'm not > sure I agree to the new names those got in the renaming patch, since > they're not really part of table AM proper ... do we really want to > offer those as separate entry points? Why not just remove those routines? I don't think it'd be better if execReplication.c has them inline - we'd just have the exact same code inline. There's plenty extension out there that use simple_heap_*, and without such wrappers, they'll all have to copy the contents of simple_table_* too. Also we'll probably want to switch CatalogTuple* over to them at some point. > Somewhat related: it's strange that CatalogTupleUpdate etc use > simple_heap_update instead of the tableam variants wrappers (I suppose > that's either because of bootstrapping concerns, or performance). It's because the callers currently expect to work with heap tuples, rather than slots. And changing that would have been a *LOT* of work (as in: prohibitively much for v12). I didn't want to create a slot for each insertion, because that'd make them slower. But as Robert said on IM (discussing something else), we already create a slot in most cases, via CatalogIndexInsert(). Not sure if HOT updates and deletes are common enough to make the slot creation in those cases measurable. > Would it be too strange to have CatalogTupleInsert call heap_insert() > directly, and do away with simple_heap_insert? (Equivalently for > update, delete). I think those wrappers made perfect sense when we had > simple_heap_insert all around the place ... but now that we introduced > the CatalogTupleFoo wrappers, I don't think it does any longer. I don't really see the advantage. Won't that just break a lot of code that will continue to work otherwise, as long as you just use heap tables? With the sole benefit of moving a bit of code from one place to another? Greetings, Andres Freund
-
Re: Inconsistency between table am callback and table function names
Andres Freund <andres@anarazel.de> — 2019-05-23T23:32:24Z
Hi, On 2019-05-14 12:11:46 -0700, Ashwin Agrawal wrote: > Thank you. Please find the patch to rename the agreed functions. It would > be good to make all consistent instead of applying exception to three > functions but seems no consensus on it. Given table_ api are new, we could > modify them leaving systable_ ones as is, but as objections left that as is. I've pushed a slightly modified version (rebase, some additional newlines due to the longer function names) now. Thanks for the patch! Greetings, Andres Freund
-
Re: Inconsistency between table am callback and table function names
Ashwin Agrawal <aagrawal@pivotal.io> — 2019-05-24T16:40:46Z
On Thu, May 23, 2019 at 4:32 PM Andres Freund <andres@anarazel.de> wrote: > Hi, > > On 2019-05-14 12:11:46 -0700, Ashwin Agrawal wrote: > > Thank you. Please find the patch to rename the agreed functions. It would > > be good to make all consistent instead of applying exception to three > > functions but seems no consensus on it. Given table_ api are new, we > could > > modify them leaving systable_ ones as is, but as objections left that as > is. > > I've pushed a slightly modified version (rebase, some additional > newlines due to the longer function names) now. Thanks for the patch! > Thanks a lot Andres. With pg_intend run before the patch on master, I can imagine possibly generated additional work for you on this.