Re: Pluggable Storage - Andres's take
От | Haribabu Kommi |
---|---|
Тема | Re: Pluggable Storage - Andres's take |
Дата | |
Msg-id | CAJrrPGeBmvWOMNnmrambEaDnb6CP3+e64m1RxLwBDA4bEfa8GA@mail.gmail.com обсуждение исходный текст |
Ответ на | Re: Pluggable Storage - Andres's take (Andres Freund <andres@anarazel.de>) |
Список | pgsql-hackers |
On Fri, Mar 22, 2019 at 5:16 AM Andres Freund <andres@anarazel.de> wrote:
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
Thanks for the corrections.
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?
In those functions, the slot->tts_tableOid is set and in the next statement
checked whether it is invalid or not? Callers of table_insert should have
already set that. So setting the value and checking in the next line is it required?
The value cannot be InvalidOid.
> + slot->tts_tableOid = RelationGetRelid(relation);
>
> In heapam_heap_update, i don't think there is a need to update
> slot->tts_tableOid.
Why?
The slot->tts_tableOid should have been updated before the call to heap_update.
setting it again after the heap_update is required?
I also observed setting slot->tts_tableOid after table_insert_XXX calls also in
Exec_insert function?
Is this to make sure that AM hasn't modified that value?
> + 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.
OK.
> + /**/
> + if (epqreturnslot)
> + {
> + *epqreturnslot = epqslot;
> + return NULL;
> + }
>
> comment update missed?
Well, you'd deleted a comment around there ;). I've added something back
now...
This is not only the problem I could have introduced, All the comments that
listed are introduced by me ;).
Regards,
Haribabu Kommi
Fujitsu Australia
В списке pgsql-hackers по дате отправления:
Предыдущее
От: Robert HaasДата:
Сообщение: Re: "WIP: Data at rest encryption" patch and, PostgreSQL 11-beta3
Следующее
От: Simon RiggsДата:
Сообщение: Re: Connections hang indefinitely while taking a gin index's LWLockbuffer_content lock