Remove PG code from pgduckdb_tables

[Y--]

Convert pgduckdb_tables file to a pure C++ file.

[JelteF]

What's the idea behind this wrapper function? Why not declare the original definition in the header? If this would contain a lock-held assert then it would make sense to me (see other comment).

[Y--]

I don't want to bring them in the header because it would bring PG's headers with it. The point of wrapping them is to decouple PG code from places where we use C++.

[JelteF]

I'm confused. What headers would that bring in? Can't you do:

extern "C" {
void estimate_rel_size(Relation rel, int32_t *attr_widths, BlockNumber *pages, double *tuples, double *allvisfrac);
}

[Y--]

Ah sorry I misunderstood. Yeah absolutely, though I actually meant to wrap the call with a PG guard since it's not trivial.

[JelteF]

Ah alright, yeah then having the wrapper makes total sense.

[JelteF]

Why does only this one need tho PD prefix? does ::RelationGetDescr not work for macros?

[Y--]

No, macros are basically inline (as text) directly in the source code.

So if you have something like this:

#define foo(a, b) something_foo(a, b)

And you try to declare a function of the same name:

// In the header
void foo(int a, int b);

// In the source file
#include "my_foo_macro.h"

void foo(int a, int b) {
  ::foo(a, b);
}

Then the pre-processor will transform the source file to:

void foo(int a, int b) {
  something_foo(a, b)(a, b);
}

Which sadly doesn't compile.

We should be able to do this:

void foo(int a, int b) {
  foo
}

As long as the parameter match, but that is super hacky and not very readable IMHO

[JelteF]

Ah yeah makes sense. But in this case you're not actually calling the original macro. It's still defined though, and thus even expands the function definition. But it seems nicer to #undef it, then we can use the same names as Postgres uses.

#undef RelationGetDescr
TupleDesc RelationGetDescr(Relation relation) {
	return relation->rd_att;
}

[JelteF]

Why do we need to declare these here. Especially the idx_t seems strange to declare. Why not include duckdb.hpp anymore?

I guess PostgresScanGlobalState and PostgresScanLocalState forward declarations are useful because include/pgduckdb/scan/postgres_scan.hpp is including postgres.h. Can we change that instead?

[Y--]

Because pgduckdb_types.hpp is included in a bunch of file I wanted to remain very conservative with adding headers, especially duckdb.h. Instead of declaring idx_t I've updated the interface to remain consistent, that's even better I think.

because include/pgduckdb/scan/postgres_scan.hpp is including postgres.h. Can we change that instead?

yep, it's coming :-)