Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Hooking into partition management process #22

Closed
1803 opened this issue Aug 23, 2016 · 20 comments
Closed

Hooking into partition management process #22

1803 opened this issue Aug 23, 2016 · 20 comments
Assignees
Milestone

Comments

@1803
Copy link

1803 commented Aug 23, 2016

In some cases it is required to perform custom actions when a partition related action is performed, such as populate triggers on partition creation or remove certain triggers (or as in my case, include it into logical replication set) when partition is detached.

It seems though event triggers can solve this issue if used correctly, but I feel like a built in solution would fit here.

@1803 1803 changed the title Extensibility issue Hooking into partition management process Aug 23, 2016
@funbringer
Copy link
Collaborator

Hi @1803,

Thanks for your feedback!

We could possibly add two columns to 'pathman_config' to store regprocedures (basically, function Oids) to be called on partition construction & destruction. For example, one could create plpgsql functions:

custom_init(relid regclass);
custom_fini(relid regclass);

which would be called whenever partition is attached (constructed) or detached(destructed). Would that suit your case?

@1803
Copy link
Author

1803 commented Aug 23, 2016

That sounds great. Is it possible to make some variables visible inside those functions?
For example, since these functions are going to be called from corresponding service function of pg_pathman I'd assume that parent_oid, partition_type along with other potentially useful variables are available. Being able to use those would be very nice. (just like NEW.* in regular trigger functions)

@funbringer funbringer self-assigned this Aug 23, 2016
@funbringer
Copy link
Collaborator

I think we could pass all available parameters directly to a callback. It would be fairly easy to implement a callback system with a fixed function signature. Could you provide us with a full list of parameters you would like to be able to use?

@funbringer
Copy link
Collaborator

Alternatively we could create a custom type containing everything you might want to observe (kinda reflection), but it would definitely take more time.

@1803
Copy link
Author

1803 commented Aug 23, 2016

I honestly don't see a need in a new type. In my implementation I just passed some variables as arguments to my custom function as I saw fit. Custom type makes it easier to upgrade to a newer version with more parameters but I honestly don't see many more parameters that are of any use here, so your initial proposal seems good enough.

On a quick glance, here's a list of variables:
Parent info:

  • parent_oid,
  • parttype

Should be enough, since these two seem to identify a record in pathman_config table (the rest can be queried as needed), although this might not be the case and then we'd need a row from pathman_config.

Child info:

  • child_oid,
  • start_value,
  • end_value,
  • interval (for range partition only)

@funbringer
Copy link
Collaborator

funbringer commented Aug 23, 2016

That sounds reasonable, thanks! We're going to release soon, so this will have to wait. In meantime you're welcome to propose any new ideas (regarding the implementation details) here.

@funbringer
Copy link
Collaborator

This feature has been added to our roadmap.

@funbringer funbringer added this to the Release 1.1 milestone Aug 31, 2016
@zilder
Copy link
Collaborator

zilder commented Sep 16, 2016

Hi,

I've added support for callbacks on partition creation. It is still in development and probably interface may change slightly but the main idea is the same as described above. It only works for RANGE partitioned relation so far since HASH partitioning doesn't allow to add/remove partitions. So there is a new function set_callback() which enables you to specify your own callback function for handling partition creation events. Your callback function must meet certain requirements particularly:

  • it only accept one argument of type JSONB;
  • return type of function is VOID.

We decided to use jsonb object to pass all necessary parameters as it allows us to pass additional info in future without breaking existing user callback functions.

Here is a simple example:

create table log(id serial, message text);
create function abc_on_partition_created_callback(args jsonb)
returns void as
$$
begin
    insert into log (message) values (args);
end
$$ language plpgsql;

create table abc(a serial, b int);
select create_range_partitions('abc', 'a', 1, 100, 3);
select set_callback('abc', 'abc_on_partition_created_callback');
insert into abc (a) values (350);

select * from log;
 id |                                               message                                               
----+-----------------------------------------------------------------------------------------------------
  1 | {"end": 401, "start": 301, "parent": 183672, "part_type": 2, "partition": 183693, "value_type": 23}
(1 row)

This feature wasn't merged into master yet. But you may try it from callbacks branch on github. Does this implementation works for you?

ps: I'll be out of town for the next two weeks and won't have a laptop with me. So if you'll have questions or suggestions I'll answer as soon as I get back.

@1803
Copy link
Author

1803 commented Sep 17, 2016

This is great. Thanks a lot.
I will do some testing later today.
Does calling split_range_partition/merge_range_partitions also trigger these?

@zilder
Copy link
Collaborator

zilder commented Sep 17, 2016

Split invokes callback, but merge doesn't since it doesn't create a new partition. This is interesting question should the merge function trigger the callback? And should there be a callback on partitions removed?

@1803
Copy link
Author

1803 commented Sep 17, 2016

Lets assume we merge two tables p1 and p2.
Merging involves removing data from table p2. If the removal is performed by the means of delete then it will(might) fire some triggers which is not very good, but I can't think of a reason why should it trigger any callbacks.
As I see it, the main use case for callback is to perform necessary actions in response to automated actions of extension, such as partition creation. However for manual actions, such as merge/split, all the necessary actions can be (and probably should be) done manually before or after invoking those functions.

That being said, I'm perfectly fine with split triggering callbacks.

@1803
Copy link
Author

1803 commented Oct 6, 2016

I've been testing this new feature for a while now and didn't notice any problems so far.

However there was a problem with error handling in the callback function. Whenever there's an exception inside the callback all I get is an error message:

[XX000] ERROR: Attempt to append new partitions to relation "[relname]" failed

I tried to work this around by wrapping the code of my function with exception block that outputs the error with raise warning, but I didn't get any messages either.

Other than that it seems to work just fine. Thank you very much for your hard work!

@funbringer
Copy link
Collaborator

@1803,

This is due to the fact that sometimes new partitions may be spawned using the special background worker (which acts as a separate session), and currently we don't transmit exceptions to the parent session that started the worker. It's better to write exception messages to server log. This behavior likely will change.

@funbringer
Copy link
Collaborator

@1803,

We're assembling all of new features in branch rel_1_1_beta, so you could examine the README.md to learn more about what we've changed so far.

@1803
Copy link
Author

1803 commented Oct 6, 2016

I tried to go deeper and implement some typical, although very simplified, use-case.

Feel free to skip the writing below and proceed to Conclusion section.


For the sake of this study lets assume we have legacy software that cannot be changed and we need to make sure it won't screw up our database.

Set up:

We have a simple table

create table pt 
(
    pt_id bigserial primary key, 
    pt_ts timestamp not null, 
    pt_dt text
);

And we want old values to be logged in a table like this:

create table pt_log
(
    ptl_action_ts timestamp, 
    ptl_dt text -- yes we log only the dt value. It doesn't have to make perfect sense right?
); 

To prevalidate the date we'd use a before-insert trigger that would do the trick for us.
Being cautios developers we are we proceed with the following trigger:

create or replace function test_tfnc_pt_prevalidate()
    returns trigger as
$body$
begin
    if (new.pt_ts is null) then
        new.pt_ts=localtimestamp;
    end if;
    if (new.pt_dt is null) then
        new.pt_dt='';
    end if;
    return new;
end;
$body$
    language plpgsql volatile;

create trigger test_tg_bi_pt_prevalidate
    before insert
    on pt
    for each row
    execute procedure test_tfnc_pt_prevalidate();

and to log values changed we make this other trigger:

create or replace function test_tfnc_pt_log_old_values()
    returns trigger as
$body$
begin
    -- we're not even going to check if pt_dt changed, since we'd have to add TG_OP condition for that and we're really lazy
    insert into pt_log(ptl_action_ts, pts_dt)
        values (localtimestamp, old.pt_dt);
    return null;
end;
$body$
    language plpgsql volatile;

create trigger test_tg_pt_aud_log_old_values
    after update or delete
    on pt
    for each row
    execute procedure test_tfnc_pt_log_old_values();

Voilà, we're ready to deal with legacy software.


Issues I encountered implementing same case trying to partition pt on pt_ts with pg_pathman's range_partition:

  1. Before triggers are ignored thus making it virtually impossible to prevalidate data.
    Same trigger applied to child-table could do the trick, but not when the side application sends null-input on partition field (expecting our trigger to change it to localtimestamp).
  2. If we're about to create a large number of partitions, say:
select create_range_partitions('pt'::regclass, 'pt_ts', date_trunc('month', localtimestamp-interval '2 years'), interval '1 month', 25);

We also need to attach our logging trigger to child-tables, since we want to keep our logging;
We're planning to use callbacks to do the trick for us with a callback like this:

create or replace function test_fnc_pt_partition_add_callback(args jsonb)
    returns void
    language plpgsql volatile as
$body$
begin
    -- this is fairly simple case. We just want to apply the trigger to the child tables. The function itself doesn't change.
    execute
    (
        select
            format
            (
                'create trigger test_tg_pt_aud_log_old_values' || -- btw, aud stands for after update/delete
                    'after update or delete ' ||
                    'on %I ' ||
                    'for each row ' ||
                    'execute procedure test_tfnc_pt_log_old_values();',
                (args ->> 'partition')::int::oid::regclass
            )
    )
    ;
end;
$body$;

select set_callback('pt'::regclass, 'test_fnc_pt_partition_add_callback'::regproc);

We have to also run a query that would populate the trigger to already created partitions.


Conclusions/TL;DR:

  1. before insert triggers on parent were a nice thing to have around. Its sad to see them go.
  2. Please add a way to attach callback within create_range_partitions function so that callback-function is called for the first set of partitions as well.

@funbringer
Copy link
Collaborator

Please add a way to attach callback within create_range_partitions function so that callback-function is called for the first set of partitions as well.

Actually there's a way: you just have to call set_init_callback() on a table to be partitioned beforehand. This is fine because pg_pathman doesn't perform any sanity checks on column partrel of pathman_config_params (except for uniqueness). The table you're passing to set_init_callback() doesn't even have to exist, it's solely your responsibility.

@1803
Copy link
Author

1803 commented Oct 7, 2016

@funbringer
Yeah I saw that function mentioned in readme-file, but unfortunately I was not able to build #rel_1_1_beta, and this feature is not available on #callbacks branch.

The error I get seems to be related to some missing libraries, but I checked manually and they are all there.

src/copy_stmt_hooking.o:copy_stmt_hooking.c:(.rdata$.refptr.XactReadOnly[.refptr.XactReadOnly]+0x0): undefined reference to `XactReadOnly'
src/copy_stmt_hooking.o:copy_stmt_hooking.c:(.rdata$.refptr.FrontendProtocol[.refptr.FrontendProtocol]+0x0): undefined reference to `FrontendProtocol'

I will keep trying..

@zilder
Copy link
Collaborator

zilder commented Oct 12, 2016

Hi,

I was not able to build #rel_1_1_beta, and this feature is not available on #callbacks branch.

What platform are you building on? We've encountered the same issue on windows. It seems the problem is that msvc compiler doesn't export extern variables which weren't defined (XactReadOnly and FrontendProtocol particularly). Thus 'unresolved external symbol' error occurs. We can't see a way to overcome this at the moment so we decided to disable COPY feature for Windows. Please try the latest version from rel_1_1_beta branch.

@1803
Copy link
Author

1803 commented Oct 13, 2016

@zilder, I was indeed building on windows, but I was using mingw not msvc, since msvc requires modifying the source code and I try to avoid that.

The error was present in beta, but the release builds just fine.

we decided to disable COPY feature for Windows

Does this mean that plans for copy won't be modified and I have to manually choose which partition to write into?

@funbringer
Copy link
Collaborator

Does this mean that plans for copy won't be modified and I have to manually choose which partition to write into?

Precisely

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants