Re: Pluggable Storage - Andres's take

From: Andres Freund <andres(at)anarazel(dot)de>
To: Haribabu Kommi <kommi(dot)haribabu(at)gmail(dot)com>
Cc: Heikki Linnakangas <hlinnaka(at)iki(dot)fi>, Asim R P <apraveen(at)pivotal(dot)io>, Pg Hackers <pgsql-hackers(at)postgresql(dot)org>, Ashwin Agrawal <aagrawal(at)pivotal(dot)io>, Dmitry Dolgov <9erthalion6(at)gmail(dot)com>, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, Alexander Korotkov <a(dot)korotkov(at)postgrespro(dot)ru>
Subject: Re: Pluggable Storage - Andres's take
Date: 2019-03-21 18:15:57
Message-ID: 20190321181557.o7sx6ul47t2ojjcm@alap3.anarazel.de
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-hackers

Hi,

Attached is a version of just the first patch. I'm still updating it,
but it's getting closer to commit:

- There were no tests testing EPQ interactions with DELETE, and only an
accidental test for EPQ in UPDATE with a concurrent DELETE. I've added
tests. Plan to commit that ahead of the big change.

- I was pretty unhappy about how the EPQ integration looked before, I've
changed that now.

I still wonder if we should restore EvalPlanQualFetch and move the
table_lock_tuple() calls in ExecDelete/Update into it. But it seems
like it'd not gain that much, because there's custom surrounding code,
and it's not that much code.

- I changed heapam_tuple_tuple to return *WouldBlock rather than just
the last result. I think that's one of the reason Haribabu had
neutered a few asserts.

- I moved comments from heapam.h to tableam.h where appropriate

- I updated the name of HeapUpdateFailureData to TM_FailureData,
HTSU_Result to TM_Result, TM_Results members now properly distinguish
between update vs modifications (delete & update).

- I separated the HEAP_INSERT_ flags into TABLE_INSERT_* and
HEAP_INSERT_ with the latter being a copy of TABLE_INSERT_ with the
sole addition of _SPECULATIVE. table_insert_speculative callers now
don't specify that anymore.

Pending work:
- Wondering if table_insert/delete/update should rather be
table_tuple_insert etc. Would be a bit more consistent with the
callback names, but a bigger departure from existing code.

- I'm not yet happy with TableTupleDeleted computation in heapam.c, I
want to revise that further

- formatting

- commit message

- a few comments need a bit of polishing (ExecCheckTIDVisible, heapam_tuple_lock)

- Rename TableTupleMayBeModified to TableTupleOk, but also probably a s/TableTuple/TableMod/

- I'll probably move TUPLE_LOCK_FLAG_LOCK_* into tableam.h

- two more passes through the patch

On 2019-03-21 15:07:04 +1100, Haribabu Kommi wrote:
> As you are modifying the 0003 patch for modify API's, I went and reviewed
> the
> existing patch and found couple corrections that are needed, in case if you
> are not
> taken care of them already.

Some I have...

> + /* Update the tuple with table oid */
> + slot->tts_tableOid = RelationGetRelid(relation);
> + if (slot->tts_tableOid != InvalidOid)
> + tuple->t_tableOid = slot->tts_tableOid;
>
> The setting of slot->tts_tableOid is not required in this function,
> After set the check is happening, the above code is present in both
> heapam_heap_insert and heapam_heap_insert_speculative.

I'm not following? Those functions are independent?

> + slot->tts_tableOid = RelationGetRelid(relation);
>
> In heapam_heap_update, i don't think there is a need to update
> slot->tts_tableOid.

Why?

> + default:
> + elog(ERROR, "unrecognized heap_update status: %u", result);
>
> heap_update --> table_update?
>
>
> + default:
> + elog(ERROR, "unrecognized heap_delete status: %u", result);
>
> same as above?

I've fixed that in a number of places.

> + /*hari FIXME*/
> + /*Assert(result != HeapTupleUpdated && hufd.traversed);*/
>
> Removing the commented codes in both ExecDelete and ExecUpdate functions.

I don't think that's the right fix. I've refactored that code
significantly now, and restored the assert in a, imo, correct version.

> + /**/
> + if (epqreturnslot)
> + {
> + *epqreturnslot = epqslot;
> + return NULL;
> + }
>
> comment update missed?

Well, you'd deleted a comment around there ;). I've added something back
now...

Greetings,

Andres Freund

Attachment Content-Type Size
v20-0001-Expand-EPQ-tests-for-UPDATEs-and-DELETEs.patch text/x-diff 12.8 KB
v20-0002-tableam-Add-insert-delete-update-lock_tuple.patch text/x-diff 143.8 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Shawn Debnath 2019-03-21 18:20:50 Re: Introduce timeout capability for ConditionVariableSleep
Previous Message David Steele 2019-03-21 18:06:36 Re: Re: INSTALL file