Re: Add --include-table-data-where option to pg_dump, to export only a subset of table data

From: Carter Thaxton <carter(dot)thaxton(at)gmail(dot)com>
To: Thomas Munro <thomas(dot)munro(at)enterprisedb(dot)com>
Cc: Stephen Frost <sfrost(at)snowman(dot)net>, Pg Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Add --include-table-data-where option to pg_dump, to export only a subset of table data
Date: 2018-05-23 10:18:52
Message-ID: CAGiT_HPF-0P_Az5aJ3UayJfmCd-0zqO1o1v-MJEqmzC1r959xw@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

>
> pg_dump.c:2323:2: warning: ISO C90 forbids mixed declarations and code
> [-Wdeclaration-after-statement]
> char *filter_clause = NULL;
> ^
>
> You need to declare this variable at the top of its scope. If you're
> using GCC or Clang you might consider building with COPT=-Werror so
> that any compiler warnings will stop the build from succeeding.
>
> This doesn't build on Windows[1], probably for the same reason.
>

Done. And thanks for the tip about COPT=-Werror

> /*
> * Is OID present in the list?
> + * Also return extra pointer-sized data by setting extra_data paramter
> */
> bool
> -simple_oid_list_member(SimpleOidList *list, Oid val)
> +simple_oid_list_member2(SimpleOidList *list, Oid val, void **extra_data)
>
> I feel like that isn't in the spirit of Lisp "member". It's now a
> kind of association list. I wonder if we are really constrained to
> use the cave-man facilities in fe_utils anyway. Though I suppose this
> list is never going to be super large so maybe the data structure
> doesn't matter too much (famous last words).
>

Yeah, I'm just trying to fit into the surrounding code as much as
possible. If you have a specific recommendation, I'm all ears.
SimpleOidList is only used by pg_dump, so if we want to rename or refactor
this data structure, it won't have much widespread impact.

And you're right that the list is not going to be particularly large.
Consider that it's already a simple linked-list, and not some more complex
hashtable, for the use cases that it already covers in pg_dump. For all of
these uses, it will only be as large as the number of options provided on
the command-line.

> + char *where_clause = pg_malloc(strlen(filter_clause) + 8 + 1);
> + strcpy(where_clause, "WHERE (");
> + strcat(where_clause, filter_clause);
> + strcat(where_clause, ")");
>
> pg_dump.c seems to be allowed to use psprintf() which'd be less
> fragile than the above code.
>

Done. Didn't realize psprintf() was available here.

> typo
>
And fixed typos.

Thanks for the review!

Attachment Content-Type Size
pgdump-include-table-data-where-v3.patch application/octet-stream 10.7 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Stephen Frost 2018-05-23 11:13:08 Re: Shared PostgreSQL libraries and symbol versioning
Previous Message Vladimir Sitnikov 2018-05-23 10:10:49 Re: Subplan result caching