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

feat: Adds 'skip-empty-xacts' support #248

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

adrijshikhar
Copy link

fixes #106

@adrijshikhar
Copy link
Author

@eulerto Please have a look and let me know what changes are required

Copy link
Owner

@eulerto eulerto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initial review.

README.md Show resolved Hide resolved
sql/skip_empty_xacts.sql Outdated Show resolved Hide resolved
sql/skip_empty_xacts.sql Outdated Show resolved Hide resolved
-- predictability
SET synchronous_commit = on;

SELECT 'init' FROM pg_create_logical_replication_slot('regression_slot', 'wal2json');
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do these tests exercise the proposal? Why don't you use simple statements (for example, using filter-tables or add-tables option)? You should include tests for both formats (see other test files).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

wal2json.c Outdated Show resolved Hide resolved
wal2json.c Outdated
{
JsonDecodingData *data = ctx->output_plugin_private;

data->nr_changes = 0;

/* Transaction starts */
OutputPluginPrepareWrite(ctx, true);
OutputPluginPrepareWrite(ctx, last_write);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you test this modification with the write-in-chunks option?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no i have not
i will benchmark this change with write-in-chunks option

wal2json.c Outdated Show resolved Hide resolved
wal2json.c Outdated Show resolved Hide resolved
wal2json.c Outdated
@@ -170,7 +186,7 @@ static void pg_decode_truncate_v1(LogicalDecodingContext *ctx,

/* version 2 */
static void pg_decode_begin_txn_v2(LogicalDecodingContext *ctx,
ReorderBufferTXN *txn);
ReorderBufferTXN *txn, bool last_write);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is an additional space here.

* has requested to skip the empty transactions we can skip the empty streams
* even though the transaction has written some changes.
*/
typedef struct
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you need this new struct? Can't you use data->nr_changes?

@adrijshikhar
Copy link
Author

@eulerto can you please go through the comments I posted?
Thanks

@eulerto
Copy link
Owner

eulerto commented Sep 20, 2022

I don't follow. You asked a question about pgindent. AFAICS you didn't post another update solving all concerns. I suggest that you read Creating Clean Patches and Development Tools and Help (there is a subsection called What's the formatting style used in PostgreSQL source code?.

@adrijshikhar
Copy link
Author

adrijshikhar commented Sep 22, 2022

thanks I will go through that. I have addressed all of your comments and made changes accordingly. I am stuck on pgindent. Somehow it changes all of the indentations. I will try to fix it and then update you.

@eulerto
Copy link
Owner

eulerto commented Sep 22, 2022

If it is changing a lot, don't run it. Instead, fix some superfluous changes (such as whitespace and new line additions). I can clean it up later, if required.

@adrijshikhar
Copy link
Author

yeah sure thanks

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

Successfully merging this pull request may close these issues.

Getting an overwhelming amount of transactions with empty change sets despite using add-tables
2 participants