Thread: Should we replace the checks for access method OID with handler OID?
Hi All, While reviewing the patch for pg_surgery contrib module - [1], Asim Praveen suggested that it would be better to replace the check for access method OID with handler OID. Otherwise, if someone creates a new AM using the AM handler that is originally supported for e.g. "heap_tableam_handler" and if this new AM is used to create a table, then one cannot perform surgery on such tables because we have checks for access method OID which would reject this new AM as we only allow heap AM. For e.g. if we do this: create access method myam type table handler heap_tableam_handler; create table mytable (…) using myam; And use an access method OID check, we won't be able to perform surgery on mytable created above because it isn't the heap table although its table structure is actually heap. This problem won't be there if the check for access method OID is replaced with handler OID. I liked this suggestion from Asim and did the changes accordingly. However, while browsing the code for other contrib modules, I could find such checks present in some of the contrib modules like pgstattuple, pageinspect and pgrowlocks as well. So, just wondering if we should be doing similar changes in these contrib modules also. Thoughts? [1] - https://www.postgresql.org/message-id/1D56CEFD-E195-4E6B-B870-3383E3E8C65E%40vmware.com -- With Regards, Ashutosh Sharma EnterpriseDB:http://www.enterprisedb.com
On Thu, Aug 27, 2020 at 5:37 AM Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > While reviewing the patch for pg_surgery contrib module - [1], Asim > Praveen suggested that it would be better to replace the check for > access method OID with handler OID. Otherwise, if someone creates a > new AM using the AM handler that is originally supported for e.g. > "heap_tableam_handler" and if this new AM is used to create a table, > then one cannot perform surgery on such tables because we have checks > for access method OID which would reject this new AM as we only allow > heap AM. For e.g. if we do this: > > create access method myam type table handler heap_tableam_handler; > create table mytable (…) using myam; > > And use an access method OID check, we won't be able to perform > surgery on mytable created above because it isn't the heap table > although its table structure is actually heap. The only reason I can see why it would make sense to do this sort of thing is if you wanted to create a new AM for testing purposes which behaves like some existing AM but is technically a different AM. And if you did that, then I guess the change you are proposing would make it behave more like it's the same thing after all, which seems like it might be missing the point. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Thu, Aug 27, 2020 at 9:21 PM Robert Haas <robertmhaas@gmail.com> wrote: > > On Thu, Aug 27, 2020 at 5:37 AM Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > > While reviewing the patch for pg_surgery contrib module - [1], Asim > > Praveen suggested that it would be better to replace the check for > > access method OID with handler OID. Otherwise, if someone creates a > > new AM using the AM handler that is originally supported for e.g. > > "heap_tableam_handler" and if this new AM is used to create a table, > > then one cannot perform surgery on such tables because we have checks > > for access method OID which would reject this new AM as we only allow > > heap AM. For e.g. if we do this: > > > > create access method myam type table handler heap_tableam_handler; > > create table mytable (…) using myam; > > > > And use an access method OID check, we won't be able to perform > > surgery on mytable created above because it isn't the heap table > > although its table structure is actually heap. > > The only reason I can see why it would make sense to do this sort of > thing is if you wanted to create a new AM for testing purposes which > behaves like some existing AM but is technically a different AM. And > if you did that, then I guess the change you are proposing would make > it behave more like it's the same thing after all, which seems like it > might be missing the point. > Okay, understood. -- With Regards, Ashutosh Sharma EnterpriseDB:http://www.enterprisedb.com