Re: Pluggable Storage - Andres's take

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