References: <CAPpHfdurb9ycV8udYqM=o0sPS66PJ4RCBM1g-bBpvzUfogY0EA@mail.gmail.com>
 <E468301D-BB6A-48D9-9833-8001A065DD2B@enterprisedb.com>
 <CAPpHfdv0g30VPKBwK=jCEmDdybgdu2Ro8VSNbLnc22C6qKsaZA@mail.gmail.com>
 <E20E3B08-A271-44A4-B302-297BDF391A04@enterprisedb.com>
 <CAPpHfduNh1iTo1KGBLAWBBt6MZQ00YkSVvUPgFDp6vUKY5Qo8w@mail.gmail.com>
 <CAPpHfduey2T2jebBzUiVAVivyNjRQ22EH_K=F6VCozB=f13tzQ@mail.gmail.com>
 <CALT9ZEF5i_bhrtUhM4bR5uFFk-aCXsWYAWSrv0FAELXuFYk2Lw@mail.gmail.com>
 <CAPpHfdvkessZ_Nj8O6o+PxfrZjVpESkhcBKT1HHjMezB1_zFWg@mail.gmail.com>
 <ME3P282MB3166E42AEB15A41E6EBADC67B62C2@ME3P282MB3166.AUSP282.PROD.OUTLOOK.COM>
 <CAPpHfduSX=7sEP7yp_ecsjaBDogXfHgXZ75Mg802r9XoJLxJLQ@mail.gmail.com>
 <CALT9ZEHLPS8ZJVH4MyszsH_-quxPgH_rh7ZuaVYiedHx_tnzqA@mail.gmail.com>
 <CALT9ZEHNeagO5PLb4Nv9J_ZaCtp+ArdVmbSLc0RHUzx_RPAa4w@mail.gmail.com>
 <CALT9ZEFVRmQfFrawTnHNCRxY1Nc+gV-wbZKrCKO4H9kA0i-8tg@mail.gmail.com>
 <CAPpHfdv6fDMkVZyJKuoaYdg5+zVnRNU6dMRM9m4mBH6+4W89ig@mail.gmail.com>
 <CALT9ZEFb0yVmZ_OQACFhYDEP3OSo4BPAMk9kju2Ap274D6SqJg@mail.gmail.com>
 <CAPpHfduE-Uk=E4W7DMyPpEeBHays-U+VzvrMgLbcEvX=ZKOiww@mail.gmail.com>
 <CALT9ZEGNiQH57Edrdk7LRdbMQ3spc_8pQ1auem1BjphKH5z=4A@mail.gmail.com>
 <CALT9ZEH+8XWxCVcjf7Hm0eH+z7-TrvHOxnr7ndscDSA90T+2cQ@mail.gmail.com>
 <CAPpHfdvz22sHkg7VpFB-XN1eZN6VfBY6mJc7zNa4aHZw+RqRXg@mail.gmail.com>
User-agent: mu4e 1.4.15; emacs 28.1
From: Japin Li <japinli@hotmail.com>
To: Alexander Korotkov <aekorotkov@gmail.com>
Cc: Pavel Borisov <pashkin.elfe@gmail.com>, Mark Dilger
 <mark.dilger@enterprisedb.com>, pgsql-hackers@lists.postgresql.org
Subject: Re: Table AM Interface Enhancements
In-reply-to: <CAPpHfdvz22sHkg7VpFB-XN1eZN6VfBY6mJc7zNa4aHZw+RqRXg@mail.gmail.com>
Date: Thu, 28 Mar 2024 23:23:05 +0800
Message-ID:
 <ME3P282MB3166C45F15AB60418656CD7FB63B2@ME3P282MB3166.AUSP282.PROD.OUTLOOK.COM>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-MessageSentRepresentingType: 1
X-MS-PublicTrafficType: Email
X-MS-TrafficTypeDiagnostic: ME3P282MB3166:EE_|SY5P282MB4400:EE_
X-MS-Office365-Filtering-Correlation-Id: bf7962ed-f9fd-4e32-401a-08dc4f3af9fd
X-Microsoft-Antispam: BCL:0;
X-Microsoft-Antispam-Message-Info:
	49WvbjkhLUct5S3HksGxZJG6wk209lBR03h1HXRGL1Q91x+dbzdDQxqBeLlPVFoye7VjTq8fhF0WD1Gh8j18eTKiL2WScn2X9c9uWe6c4m3+KslTfShft/A/Bj4Ev29Qsyaj27griC0OGRBQEbXanOKmZ20XUX2dDi3ewyN7K5jm0wQMwx4EVZOqex0quqZTfm81jjins2tYih1y4T3ASJFhCE30cWeDrRp/7CHBAGn8HG5MHX8rMQPwxBwf7V5RIo52rHIj339JY/57FYQEVue3fZ+hIwP5+9uSKZdV1rtdS8+7oYWH8epkvpZVe4MO8PT5DnI70aptpk0AjFbtQUUlox/7Bx8DdOq2l5juRALRi6J7vnuGsLsD5qZwrEyJVCebEOFPTaB8T/HKCamP2b/u8inLbCeMBTEatt/06GbTjKKzVbOcW3OWoQeRliSlUhGjfcUYwCs00Qug+eleBLJIz+OMhXVu6vuBvhCjY21ZozbpZAbZDmM0BI166TMlKTfnSD/2ltyrXJg82f0OvWA5EV3VOvDHGPOSKNIF5SBE4oi6dsdeWFQThM8pE/7y
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?UFBCVTZpR3BkWktLTzhaRVV1TnVON1VQMUVJY0ZMbTZiSUxtOHpYaHo0RUVG?=
 =?utf-8?B?aXFBNWRjWnBXVytTcFMwdEROK1o0cEFmSkRtbW1oWEdrL09GVDVpWG1OajNy?=
 =?utf-8?B?OUVZNUN0YU5mb29rTVRYNEhvVWhhRHVyUStNdU00RE9HZERSUmpoU2h1SEQx?=
 =?utf-8?B?NXNnS0NSYnI3cWtlWFZ3T1NDcGNLdE9XQUtBZzJROG4zMDczanZpTVFKeWF2?=
 =?utf-8?B?eGNCS1hBVjdFTnhRMnBBLzc5cWZnYVZ2aytTaW05bVg3RE92ZDBSTE9QOEN6?=
 =?utf-8?B?R2RLZ1cyemJJUjVZNjh5Zjc0a1BZbHpCRTQ2ZC95MVpyZ09Od0xFamwreFdW?=
 =?utf-8?B?UWpvZ3FXRVFEQmZuNVY2VUY3QlR4V3RmeXFndmRwdXovM2hMbUY2cDF5WS92?=
 =?utf-8?B?OWdHcHJEam1pMWF1bXJ1dXpGRmRjV0NtSmpWWm13cS9JSk5uWkpxcVp6REpm?=
 =?utf-8?B?WWxmRC9scFQrNWVlVlhqS1kwZjM1VzdGQ3JyNW0rdmQraW9LeWlWcTF0VmVt?=
 =?utf-8?B?QnBQTno1NkVTSjE2Z1ZDdDllMGh0VnJaQXlGbTZTYUpoYVNwZ3Z1QkdDNit5?=
 =?utf-8?B?ZUlwNmpUT1VYamZRR3V6UFBzekc0ODhZL3d1d0NqWFlZSEs5cTJheW5iYmt6?=
 =?utf-8?B?TkM2UEFGcmlKcVRlLzJheGFCbHdVM2U0bFFvczRmblNmL3hpTERqRytNM2Fy?=
 =?utf-8?B?S3Zab2RIRmdHTVN3VE54THBoMWo4Wk9SMDMrQS9uU2dCT0VScWhUaFIweE90?=
 =?utf-8?B?ODlEZWlDVlhwNURUNHB2NUFZSU03VlJRUTEydW03bkhlcXFoNkFJNGlXSzNG?=
 =?utf-8?B?UXdmWUs5aVRmblNWQXpmWFdvRU1vdm9sT3ZrSmZaUi9Db2VENDlLbXRsdUc3?=
 =?utf-8?B?RnByNy9iWVVXQi9RRitDcWlVUDZleG05NW9UVFJ5QnVGcWNSQ3F5ZVBFekhy?=
 =?utf-8?B?K25Ib3JTeHJ1THVBNG1MNTRWNTV0aXgzaTdOOWpaREhOd2d1a2tjQUV5QUpk?=
 =?utf-8?B?d1owY1BLeFdTNFNXT2JBZ1dyN01JOUZ1d2JKbzVIUzNCUGRWQzY0Q0NhVkw3?=
 =?utf-8?B?TUl2dSszUFZibDg3bmF4TFdVSlpkdXYzV2N4d1lQNDBmYm1PTTF5SVJLS0xH?=
 =?utf-8?B?bXI1dGMyM3FBWi8yTW1JQk1lWEhoVVc5RHp5R3lXNDJqU0tKSFU0UWxsMi9W?=
 =?utf-8?B?cktEMjBKR3hjM2hMUkxmeEs5dGUrbmdlZE1TVEltVXJsK3BsMk9tOU80VGpZ?=
 =?utf-8?B?TG5HQlRHL0c5K04xV3pic1l2cUVaN014a0NDZ3gvOEtQUlpiS0M3VkFRL1Zw?=
 =?utf-8?B?RGJKRFViSkxXYjE0Y2FyQzJ3N2dGUURPb3BOTlozeDFOalFGM2F1dk54enRJ?=
 =?utf-8?B?UWlsWUZ5a3UwNkg5eGNlck8xYUNEOHJ5VzRobEhhR0pLVHpUZnBteUFPTXVJ?=
 =?utf-8?B?ZlAyWEZiQVZ1WURFVHk3ZnY0V1RKNmVDQ2JQQ0NGQVg0eVU3cjRTVG5ZMkx0?=
 =?utf-8?B?T0JNZEl4K2N6UlBTQkRUbUpaRlVBY0IzLzhJMXczWHhDMFFKUTE0Q1BCTjZT?=
 =?utf-8?B?UFkzVXA4bVUvQzQ4cyttdkJlaGtmL3pkYzNURVBTUS9LVHlzR0xuVUF4V3VY?=
 =?utf-8?Q?xOh9mu2Xo2uQ4BsDf5YXpm2zNTRjd5mw6b+jjF2gMRk4=3D?=
X-OriginatorOrg: sct-15-20-4755-11-msonline-outlook-746f3.templateTenant
X-MS-Exchange-CrossTenant-Network-Message-Id: bf7962ed-f9fd-4e32-401a-08dc4f3af9fd
X-MS-Exchange-CrossTenant-AuthSource: ME3P282MB3166.AUSP282.PROD.OUTLOOK.COM
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 28 Mar 2024 15:23:10.6206
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 84df9e7f-e9f6-40af-b435-aaaaaaaaaaaa
X-MS-Exchange-CrossTenant-RMS-PersistedConsumerOrg:
	00000000-0000-0000-0000-000000000000
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SY5P282MB4400
List-Id: <pgsql-hackers.lists.postgresql.org>
List-Help: <https://lists.postgresql.org/manage/>
List-Subscribe: <https://lists.postgresql.org/manage/>
List-Post: <mailto:pgsql-hackers@lists.postgresql.org>
List-Owner: <mailto:pgsql-hackers-owner@lists.postgresql.org>
List-Archive: <https://www.postgresql.org/list/pgsql-hackers>
Archived-At: <https://www.postgresql.org/message-id/ME3P282MB3166C45F15AB60418656CD7FB63B2%40ME3P282MB3166.AUSP282.PROD.OUTLOOK.COM>
Precedence: bulk


On Thu, 28 Mar 2024 at 21:26, Alexander Korotkov <aekorotkov@gmail.com> wro=
te:
> Hi Pavel!
>
> Revised patchset is attached.
>
> On Thu, Mar 28, 2024 at 3:12=E2=80=AFPM Pavel Borisov <pashkin.elfe@gmail=
.com> wrote:
>> The other extensibility that seems quite clear and uncontroversial to me=
 is 0006.
>>
>> It simply shifts the decision on whether tuple inserts should invoke ins=
erts to the related indices to the table am level. It doesn't change the cu=
rrent heap insert behavior so it's safe for the existing heap access method=
. But new table access methods could redefine this (only for tables created=
 with these am's) and make index inserts independently of ExecInsertIndexTu=
ples inside their own implementations of tuple_insert/tuple_multi_insert me=
thods.
>>
>> I'd propose changing the comment:
>>
>> 1405  * This function sets `*insert_indexes` to true if expects caller t=
o return
>> 1406  * the relevant index tuples.  If `*insert_indexes` is set to false=
, then
>> 1407  * this function cares about indexes itself.
>>
>> in the following way
>>
>> Tableam implementation of tuple_insert should set `*insert_indexes` to t=
rue
>> if it expects the caller to insert the relevant index tuples (as in heap
>>  implementation). It should set `*insert_indexes` to false if it cares
>> about index inserts itself and doesn't want the caller to do index inser=
ts.
>
> Changed as you proposed.
>
>> Maybe, a commit message is also better to reformulate to describe better=
 who should do what.
>
> Done.
>
> Also, I removed extra includes in 0001 as you proposed and edited the
> commit message in 0002.
>
>> I think, with rebase and correction in the comments/commit message patch=
 0006 is ready to be committed.
>
> I'm going to push 0001, 0002 and 0006 if no objections.

Thanks for updating the patches.  Here are some minor sugesstion.

0003

+static inline TupleTableSlot *
+heapam_tuple_insert_with_arbiter(ResultRelInfo *resultRelInfo,

I'm not entirely certain whether the "inline" keyword has any effect.

0004

+static bytea *
+heapam_indexoptions(amoptions_function amoptions, char relkind,
+                                       Datum reloptions, bool validate)
+{
+       return index_reloptions(amoptions, reloptions, validate);
+}

Could you please explain why we are not verifying the relkind like
heapam_reloptions()?


-               case RELKIND_VIEW:
                case RELKIND_MATVIEW:
+               case RELKIND_VIEW:
                case RELKIND_PARTITIONED_TABLE:

I think this change is unnecessary.

+                       {
+                               Form_pg_class classForm;
+                               HeapTuple       classTup;
+
+                               /* fetch the relation's relcache entry */
+                               if (relation->rd_index->indrelid >=3D First=
NormalObjectId)
+                               {
+                                       classTup =3D SearchSysCacheCopy1(RE=
LOID, ObjectIdGetDatum(relation->rd_index->indrelid));
+                                       classForm =3D (Form_pg_class) GETST=
RUCT(classTup);
+                                       if (classForm->relam >=3D FirstNorm=
alObjectId)
+                                               tableam =3D GetTableAmRouti=
neByAmOid(classForm->relam);
+                                       else
+                                               tableam =3D GetHeapamTableA=
mRoutine();
+                                       heap_freetuple(classTup);
+                               }
+                               else
+                               {
+                                       tableam =3D GetHeapamTableAmRoutine=
();
+                               }
+                               amoptsfn =3D relation->rd_indam->amoptions;
+                       }

- We can reduce the indentation by moving the classFrom and classTup into
  the if branch.
- Perhaps we could remove the brace of else branch to maintain consistency
  in the code style.

--
Regards,
Japin Li