On Tue, 19 Mar 2024 at 21:05, Alexander Korotkov <[email protected]> wrote:
> Hi, Pavel!
>
> On Tue, Mar 19, 2024 at 11:34 AM Pavel Borisov <[email protected]> wrote:
>> On Tue, 19 Mar 2024 at 03:34, Alexander Korotkov <[email protected]>
>> wrote:
>>>
>>> On Sun, Mar 3, 2024 at 1:50 PM Alexander Korotkov <[email protected]>
>>> wrote:
>>> > On Mon, Nov 27, 2023 at 10:18 PM Mark Dilger
>>> > <[email protected]> wrote:
>>> > >
>>> > > > On Nov 25, 2023, at 9:47 AM, Alexander Korotkov
>>> > > > <[email protected]> wrote:
>>> > > >
>>> > > >> Should the patch at least document which parts of the EState are
>>> > > >> expected to be in which states, and which parts should be viewed as
>>> > > >> undefined? If the implementors of table AMs rely on any/all aspects
>>> > > >> of EState, doesn't that prevent future changes to how that structure
>>> > > >> is used?
>>> > > >
>>> > > > New tuple tuple_insert_with_arbiter() table AM API method needs EState
>>> > > > argument to call executor functions: ExecCheckIndexConstraints(),
>>> > > > ExecUpdateLockMode(), and ExecInsertIndexTuples(). I think we
>>> > > > probably need to invent some opaque way to call this executor function
>>> > > > without revealing EState to table AM. Do you think this could work?
>>> > >
>>> > > We're clearly not accessing all of the EState, just some specific
>>> > > fields, such as es_per_tuple_exprcontext. I think you could at least
>>> > > refactor to pass the minimum amount of state information through the
>>> > > table AM API.
>>> >
>>> > Yes, the table AM doesn't need the full EState, just the ability to do
>>> > specific manipulation with tuples. I'll refactor the patch to make a
>>> > better isolation for this.
>>>
>>> Please find the revised patchset attached. The changes are following:
>>> 1. Patchset is rebase. to the current master.
>>> 2. Patchset is reordered. I tried to put less debatable patches to the top.
>>> 3. tuple_is_current() method is moved from the Table AM API to the
>>> slot as proposed by Matthias van de Meent.
>>> 4. Assert added to the table_free_rd_amcache() as proposed by Pavel Borisov.
>>
>>
>> Patches 0001-0002 are unchanged compared to the last version in thread [1].
>> In my opinion, it's still ready to be committed, which was not done for time
>> were too close to feature freeze one year ago.
>>
>> 0003 - Assert added from previous version. I still have a strong opinion
>> that allowing multi-chunked data structures instead of single chunks is
>> completely safe and makes natural process of Postgres improvement that is
>> self-justified. The patch is simple enough and ready to be pushed.
>>
>> 0004 (previously 0007) - Have not changed, and there is consensus that this
>> is reasonable. I've re-checked the current code. Looks safe considering
>> returning a different slot, which I doubted before. So consider this patch
>> also ready.
>>
>> 0005 (previously 0004) - Unused argument in the is_current_xact_tuple()
>> signature is removed. Also comparing to v1 the code shifted from tableam
>> methods to TTS's level.
>>
>> I'd propose to remove Assert(!TTS_EMPTY(slot)) for
>> tts_minimal_is_current_xact_tuple() and tts_virtual_is_current_xact_tuple()
>> as these are only error reporting functions that don't use slot actually.
>>
>> Comment similar to:
>> +/*
>> + * VirtualTupleTableSlots never have a storage tuples. We generally
>> + * shouldn't get here, but provide a user-friendly message if we do.
>> + */
>> also applies to tts_minimal_is_current_xact_tuple()
>>
>> I'd propose changes for clarity of this comment:
>> %s/a storage tuples/storage tuples/g
>> %s/generally//g
>>
>> Otherwise patch 0005 also looks good to me.
>>
>> I'm planning to review the remaining patches. Meanwhile think pushing what
>> is now ready and uncontroversial is a good intention.
>> Thank you for the work done on this patchset!
>
> Thank you, Pavel!
>
> Regarding 0005, I did apply "a storage tuples" grammar fix. Regarding
> the rest of the things, I'd like to keep methods
> tts_*_is_current_xact_tuple() to be similar to nearby
> tts_*_getsysattr(). This is why I'm keeping the rest unchanged. I
> think we could refactor that later, but together with
> tts_*_getsysattr() methods.
>
> I'm going to push 0003, 0004 and 0005 if there are no objections.
>
> And I'll update 0001 and 0002 in their dedicated thread.
>
When I try to test the patch on Ubuntu 22.04 with GCC 11.4.0. There are some
warnings as following:
/home/japin/Codes/postgres/build/../src/backend/access/heap/heapam_handler.c:
In function ‘heapam_acquire_sample_rows’:
/home/japin/Codes/postgres/build/../src/backend/access/heap/heapam_handler.c:1603:28:
warning: implicit declaration of function
‘get_tablespace_maintenance_io_concurrency’ [-Wimplicit-function-declaration]
1603 | prefetch_maximum =
get_tablespace_maintenance_io_concurrency(onerel->rd_rel->reltablespace);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/japin/Codes/postgres/build/../src/backend/access/heap/heapam_handler.c:1757:30:
warning: implicit declaration of function ‘floor’
[-Wimplicit-function-declaration]
1757 | *totalrows = floor((liverows / bs.m) * totalblocks +
0.5);
| ^~~~~
/home/japin/Codes/postgres/build/../src/backend/access/heap/heapam_handler.c:49:1:
note: include ‘<math.h>’ or provide a declaration of ‘floor’
48 | #include "utils/sampling.h"
+++ |+#include <math.h>
49 |
/home/japin/Codes/postgres/build/../src/backend/access/heap/heapam_handler.c:1757:30:
warning: incompatible implicit declaration of built-in function ‘floor’
[-Wbuiltin-declaration-mismatch]
1757 | *totalrows = floor((liverows / bs.m) * totalblocks +
0.5);
| ^~~~~
/home/japin/Codes/postgres/build/../src/backend/access/heap/heapam_handler.c:1757:30:
note: include ‘<math.h>’ or provide a declaration of ‘floor’
/home/japin/Codes/postgres/build/../src/backend/access/heap/heapam_handler.c:1603:21:
warning: implicit declaration of function
'get_tablespace_maintenance_io_concurrency' is invalid in C99
[-Wimplicit-function-declaration]
prefetch_maximum =
get_tablespace_maintenance_io_concurrency(onerel->rd_rel->reltablespace);
^
It seems you forgot to include math.h and utils/spccache.h header files
in heapam_handler.c.
diff --git a/src/backend/access/heap/heapam_handler.c
b/src/backend/access/heap/heapam_handler.c
index ac24691bd2..04365394f1 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -19,6 +19,8 @@
*/
#include "postgres.h"
+#include <math.h>
+
#include "access/genam.h"
#include "access/heapam.h"
#include "access/heaptoast.h"
@@ -46,6 +48,7 @@
#include "utils/builtins.h"
#include "utils/rel.h"
#include "utils/sampling.h"
+#include "utils/spccache.h"
static TM_Result heapam_tuple_lock(Relation relation, Datum tupleid,
Snapshot
snapshot, TupleTableSlot *slot,
