palkan / logidze

Database changes log for Rails
MIT License
1.59k stars 74 forks source link

Partition support (after trigger) #226

Closed prog-supdex closed 1 year ago

prog-supdex commented 1 year ago

What is the purpose of this pull request?

Add ability to support partitioned tables for versions 11 and 12

Is there anything you'd like reviewers to focus on?

The problem in the issue #215 appears only for versions postgresql 11 and 12

BEFORE ROW triggers, if necessary, must be defined on individual partitions, not the partitioned table

In postgresql version 13 and above supports BEFORE ROW triggers for partitioned tables Postgresql docs also says that versions 9.6 and 10 do not support ROW triggers for partitioned table

Row triggers, if necessary, must be defined on individual partitions, not the partitioned table.

So, this PR resolves the problem for 11 and 12 versions of postgresql. Maybe, I misunderstood the problem and resolve was not right

I added to model/triggers/logidze.sql check for getting a current version of PG and is partitioned table or not and then it decides, which type of trigger to use (maybe it is not a good solution) So, I think, this migration will be work in every version of PG, for example, if somebody will change from 11 pg to 13

Also, logic from function logidze_logger was moved to logidze_generate_log_data And here we have one moment - in version 9.6, 10 we cannot set the type "record" in arguments of function, so, here was set "anyelement", and then I pass it to variables with record types

About specs I added migration only for pg versions 11 and above

This seems is not a good solution, but I need to skip partitionated logic for versions 9.6 and 10. Also was added helper pg_versions

Preferably use before triggers, because "before" solution has more performance, therefore in this PR we use "before" trigger when can (13 version and above)

Early I asked about "do we need to add option --after_trigger or automatic detect type of trigger" and here I decided to use "automatic detect type of trigger". This PR add more complex to this project and maybe it has some not good solutions (for example, dynamic test by current pg version) and if we do not need an "automatic detect type trigger", so I can add option --after_trigger or if this solution is ok, I can refactor/fix not good places in the code

Checklist

215

palkan commented 1 year ago

Thanks for the PR and for the research! Great to know that recent PostgreSQL version do not need this hack. That means we don't need to keep this feature for a long time. And keeping that...

Early I asked about "do we need to add option --after_trigger or automatic detect type of trigger" and here I decided to use "automatic detect type of trigger".

I think, automatic detection is too much for this use case (though I really appreciate you effort here; we will probably re-use the code for checking PG versions in the future).

Since we consider this to be a temporary hack, I'd prefer to keep changes to the current implementation as minimal as possible:

I don't want to ask you to refactor your solution—that would be too much. My plan is to cherry-pick the relevant parts (maybe, from #225, too) and build the final version.

prog-supdex commented 1 year ago

Thank you @palkan for the comment and the advices, how to do it better and right

If you need my help, I'll be happy to help

thank you!

palkan commented 1 year ago

Opened #228, please take a look. (And thanks for your help; you can clearly find the bits from your PR)