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

AAP-15927 Use ATTACH PARTITION to avoid exclusive table lock for events #14433

Merged
merged 5 commits into from
Sep 21, 2023

Conversation

AlanCoding
Copy link
Member

SUMMARY

This parrots the example from postgresql docs

https://www.postgresql.org/docs/current/ddl-partitioning.html

Specifically, see the example:

CREATE TABLE measurement_y2008m02
  (LIKE measurement INCLUDING DEFAULTS INCLUDING CONSTRAINTS)
  TABLESPACE fasttablespace;

ALTER TABLE measurement_y2008m02 ADD CONSTRAINT y2008m02
   CHECK ( logdate >= DATE '2008-02-01' AND logdate < DATE '2008-03-01' );

\copy measurement_y2008m02 from 'measurement_y2008m02'
-- possibly some other data preparation work

ALTER TABLE measurement ATTACH PARTITION measurement_y2008m02
    FOR VALUES FROM ('2008-02-01') TO ('2008-03-01' );

This just replaces our current SQL for creating a partition with this example, editing the details to use the tables names and timestamps that we use.

Right now this is working, in that new jobs will create a partition and that goes fine.

I did add a new bit of code to this to pre-check for existence of the partition. Letting it error in postgres created a lot of log noise that we've found very distracting in the past so I wanted to go ahead and take care of this, particularly since I'm running a new version of our SQL code. I do think we should remove our existing error handling since I added this, so expect a new commit to do that.

ISSUE TYPE
  • Bug, Docs Fix or other nominal change
COMPONENT NAME
  • API
ADDITIONAL INFORMATION

I'm not yet sure if we need this part:

                    f'ALTER TABLE {tblname}_{partition_label} ADD CONSTRAINT y{partition_label} '
                    f'CHECK ( job_created >= TIMESTAMP \'{start_timestamp}\' AND job_created < TIMESTAMP \'{end_timestamp}\' ); '

Because historically, the old method did not create this constraint. So I think we could just delete these lines, but including it is more compliant to the docs so I wanted to start with this.

@AlanCoding AlanCoding changed the title Use ATTACH PARTITION to avoid exclusive table lock for events AAP-15927 Use ATTACH PARTITION to avoid exclusive table lock for events Sep 13, 2023
@AlanCoding AlanCoding marked this pull request as ready for review September 13, 2023 13:54
@AlanCoding
Copy link
Member Author

Update:

From the CI logs, I discovered that we're hitting a new kind of race condition. It is ever-so slightly different from the established one.

2023-09-18 15:18:07,557 ERROR    [bf05ce1b] awx.main.tasks.jobs job 9 (running) Exception occurred while running task
Traceback (most recent call last):
  File "/var/lib/awx/venv/awx/lib64/python3.9/site-packages/django/db/backends/utils.py", line 87, in _execute
    return self.cursor.execute(sql)
  File "/var/lib/awx/venv/awx/lib64/python3.9/site-packages/psycopg/cursor.py", line 723, in execute
    raise ex.with_traceback(None)
psycopg.errors.UniqueViolation: duplicate key value violates unique constraint "pg_type_typname_nsp_index"
DETAIL:  Key (typname, typnamespace)=(main_jobevent_20230918_15, 2200) already exists.

The above exception was the direct cause of the following exception:
Traceback (most recent call last):
  File "/awx_devel/awx/main/tasks/jobs.py", line 491, in run
    self.pre_run_hook(self.instance, private_data_dir)
  File "/awx_devel/awx/main/tasks/jobs.py", line 1058, in pre_run_hook
    super(RunJob, self).pre_run_hook(job, private_data_dir)
  File "/awx_devel/awx/main/tasks/jobs.py", line 425, in pre_run_hook
    create_partition(instance.event_class._meta.db_table, start=instance.created)
  File "/awx_devel/awx/main/utils/common.py", line 1174, in create_partition
    cursor.execute(
  File "/var/lib/awx/venv/awx/lib64/python3.9/site-packages/django/db/backends/utils.py", line 102, in execute
    return super().execute(sql, params)
  File "/var/lib/awx/venv/awx/lib64/python3.9/site-packages/django/db/backends/utils.py", line 67, in execute
    return self._execute_with_wrappers(
  File "/var/lib/awx/venv/awx/lib64/python3.9/site-packages/django/db/backends/utils.py", line 80, in _execute_with_wrappers
    return executor(sql, params, many, context)
  File "/var/lib/awx/venv/awx/lib64/python3.9/site-packages/django/db/backends/utils.py", line 89, in _execute
    return self.cursor.execute(sql, params)
  File "/var/lib/awx/venv/awx/lib64/python3.9/site-packages/django/db/utils.py", line 91, in __exit__
    raise dj_exc_value.with_traceback(traceback) from exc_value
  File "/var/lib/awx/venv/awx/lib64/python3.9/site-packages/django/db/backends/utils.py", line 87, in _execute
    return self.cursor.execute(sql)
  File "/var/lib/awx/venv/awx/lib64/python3.9/site-packages/psycopg/cursor.py", line 723, in execute
    raise ex.with_traceback(None)
django.db.utils.IntegrityError: duplicate key value violates unique constraint "pg_type_typname_nsp_index"
DETAIL:  Key (typname, typnamespace)=(main_jobevent_20230918_15, 2200) already exists.

As such, I propose that we keep the current structure but add IntegrityError to the list of what we catch.

@TheRealHaoLiu
Copy link
Member

already pass SOLOYO so I'm gonna go ahead and merge it

@AlanCoding AlanCoding merged commit 3e607f8 into ansible:devel Sep 21, 2023
19 checks passed
kdelee pushed a commit to kdelee/awx that referenced this pull request May 8, 2024
…r events (ansible#14433) (ansible#6491)

* AAP-15927 Use ATTACH PARTITION to avoid exclusive table lock for events (ansible#14433)
* Add null value handling in create_partition
* Use cursor to fetchone, the established Django pattern

---------

Co-authored-by: Alan Rominger <[email protected]>
djyasin pushed a commit to djyasin/awx that referenced this pull request Sep 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants