Implement show_chunks in C and have drop_chunks use it #642

Ngalstyan4 · 2018-08-06T16:54:12Z

Timescale provides an efficient and easy to use api to drop individual
chunks from timescale database through drop_chunks. This PR builds on
that functionality and through a new show_chunks function gives the
opportunity to see the chunks that would be dropped if drop_chunks was run.
Additionally, it adds a newer_than option to drop_chunks (also supported
by show_chunks) that allows to see/drop chunks in an interval or newer
than a point in time.

This commit includes:
- Implementation of show_chunks in C
- Additional helper functions to work with chunks
- New version of drop_chunks in sql that uses show_chunks. This
also adds a newer_than option to drop_chunks
- More enhanced tests of drop_chunks and new tests for show_chunks

Among other reasons, show_chunks was implemented in C in order
to be able to have both older_than and newer_than arguments be null. This
was not possible in SQL because the arguments had to have polymorphic types
and whether they are used in function body or not, PL/pgSQL requires these
arguments to typecheck.
Implements and resolves #572

Ngalstyan4

Added some questions for discussion before merge

Ngalstyan4 · 2018-08-06T16:56:07Z

sql/ddl_api.sql

-    SELECT _timescaledb_internal.time_to_internal(older_than, pg_typeof(older_than)) INTO older_than_internal;
-    PERFORM _timescaledb_internal.drop_chunks_impl(older_than_internal, table_name, schema_name, cascade);
+    PERFORM _timescaledb_internal.drop_chunks_impl(older_than, table_name, schema_name, cascade,
+    truncate_before => FALSE, newer_than_time => newer_than);


What is truncate_before argument of drop_chunks_impl for?
it is not ever used and there is no public facing api for it.

I think this is functionality hooks for an enterprise feature that might not be used anymore. @cevian would know more.

src/chunk.c

Ngalstyan4 · 2018-08-06T17:06:56Z

src/chunk.c

+	if (IS_INTEGER_TYPE(time_column_type) && IS_INTEGER_TYPE(arg_type))
+		return;
+
+	if (arg_type == INTERVALOID)


Should we give a deprecation warning when drop_chunks or show_chunks is used with INTERVAL argument type?
During discussion of show_chunks api it has come up several times.
Main arguments for deprecating

It is as simple as passing now()- interval instead of interval when the option is not supported but makes it more explicit

It becomes less clear what exactly it means when there also is a newer_than interval (is it now() - interval or now() + interval ?)

@mfreed @cevian @erimatnor @JLockerman @amytai

codecov-io · 2018-08-06T17:51:34Z

Codecov Report

Merging #642 into master will increase coverage by 0.06%.
The diff coverage is 89.61%.

@@            Coverage Diff             @@
##           master     #642      +/-   ##
==========================================
+ Coverage   89.92%   89.99%   +0.06%     
==========================================
  Files          79       80       +1     
  Lines        8827     8978     +151     
==========================================
+ Hits         7938     8080     +142     
- Misses        889      898       +9

Impacted Files	Coverage Δ
src/catalog.c	`86.66% <ø> (ø)`	⬆️
src/dimension_slice.c	`99.44% <ø> (ø)`	⬆️
src/extension_utils.c	`74.28% <ø> (ø)`	⬆️
src/planner.c	`93.91% <ø> (ø)`	⬆️
src/plan_add_hashagg.c	`97.09% <ø> (ø)`	⬆️
src/compat.c	`0% <0%> (ø)`
src/dimension.c	`96.51% <100%> (-0.14%)`	⬇️
src/hypertable.c	`88.1% <100%> (+0.19%)`	⬆️
src/utils.c	`61.9% <78.57%> (-0.05%)`	⬇️
src/chunk.c	`92.04% <93.75%> (+0.45%)`	⬆️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d461959...37fc267. Read the comment docs.

Ngalstyan4 · 2018-08-06T20:15:49Z

sql/updates/0.10.1--0.11.0-dev.sql

@@ -0,0 +1 @@
+DROP FUNCTION IF EXISTS _timescaledb_internal.dimension_get_time(integer);


depending on which release this function ends up in, we should remember to put this in relevant update file.

Ngalstyan4 · 2018-08-06T20:38:22Z

sql/updates/0.10.1--0.11.0-dev.sql

+DROP FUNCTION IF EXISTS drop_chunks(INTERVAL, NAME, NAME, BOOLEAN);
+DROP FUNCTION IF EXISTS drop_chunks(ANYELEMENT, NAME, NAME, BOOLEAN);
+DROP FUNCTION IF EXISTS _timescaledb_internal.drop_chunks_impl(BIGINT, NAME, NAME, BOOLEAN, BOOLEAN)
+DROP FUNCTION IF EXISTS _timescaledb_internal.dimension_get_time(INTEGER);


depending on which release this function ends up in, we should remember to put this in relevant update file.

Ngalstyan4 · 2018-08-06T21:54:53Z

sql/updates/0.10.1--0.11.0-dev.sql

+DROP FUNCTION IF EXISTS drop_chunks(ANYELEMENT, NAME, NAME, BOOLEAN);
+DROP FUNCTION IF EXISTS _timescaledb_internal.drop_chunks_impl(BIGINT, NAME, NAME, BOOLEAN, BOOLEAN);
+DROP FUNCTION IF EXISTS _timescaledb_internal.drop_chunks_type_check(REGTYPE, NAME, NAME);
+DROP FUNCTION IF EXISTS _timescaledb_internal.dimension_get_time(INTEGER);


depending on which release this function ends up in, we should remember to put this in relevant update file.
Also if we plan a 0.12.0 release, this file should be renamed right @RobAtticus ?

You may have to rebase and move it there manually, not entirely sure how this will be handled. good job surfacing it early though

sql/ddl_api.sql

erimatnor · 2018-08-06T19:27:57Z

sql/ddl_api.sql

-    SELECT _timescaledb_internal.time_to_internal(older_than, pg_typeof(older_than)) INTO older_than_internal;
-    PERFORM _timescaledb_internal.drop_chunks_impl(older_than_internal, table_name, schema_name, cascade);
+    PERFORM _timescaledb_internal.drop_chunks_impl(older_than, table_name, schema_name, cascade,
+    truncate_before => FALSE, newer_than_time => newer_than);


I think this is functionality hooks for an enterprise feature that might not be used anymore. @cevian would know more.

erimatnor · 2018-08-06T19:29:55Z

sql/ddl_api.sql

+-- if and only if all the hypertables in the database
+-- have the same type as the given time constraint argument
+CREATE OR REPLACE FUNCTION show_chunks(
+    hypertable_name  REGCLASS = NULL,


If this is a REGCLASS the parameter should probably not be hypertable_name since the argument is really an OID (a name gets implicitly converted to the REGCLASS). In other words, you can do show_chunks(12423, ...);

erimatnor · 2018-08-07T12:13:22Z

sql/ddl_internal.sql

+-- specifying the caller name. This makes it easier to taylor
+-- error messages to the caller function context.
+CREATE OR REPLACE FUNCTION _timescaledb_internal.show_chunks_impl(
+    hypertable_name  REGCLASS = NULL,


Some here as above w.r.t. name vs regclass.

I also noticed the discrepancy.
The reason I went with this was to be compatible with drop_chunks naming.
I think it would be ideal fro the two to have identical APIs so any call of drop_chunks could be refactored into a call of show_chunks by only changing function name.
I am happy to change this to hypertable or hypertable_regclass if you think that is better.
Could also add a drop_chunks implementation with similar API to provide interoperability

I prefer hypertable. Regarding drop_chunks, it is using two NAME parameters as opposed to a REGCLASS so there _namesuffix is appropriate. Still, I think that drop_chunks should probably take a REGCLASS instead. I don't think there's a good reason why it now takes two NAME params (apart from being legacy and probably my fault).

We should discuss changing drop_chunks to use REGCLASS and I think for most use-cases it would be safe since with both old and new function you can do:

drop_chunks(now() - '1 day', 'mytable');

And only in case of specifying a schema in the old way it would break.

sql/ddl_api.sql

src/chunk.c

erimatnor · 2018-08-08T16:04:05Z

src/chunk.c

+		}
+		else
+		{
+			ht = hypertable_cache_get_entry(hypertable_cache, table_relid);


You can do hypertable_get_by_name here to avoid the cache altogether.

erimatnor · 2018-08-08T16:06:26Z

src/chunk.c

+		hypertable_cache = hypertable_cache_pin();
+		if (PG_ARGISNULL(0))
+		{
+			hypertable_get_all(&hypertables, CurrentMemoryContext);


You actually need only a set of dimension_scans here. But maybe getting the full hypertables are fine, since that's probably what you do in the single case.

erimatnor · 2018-08-08T16:16:44Z

src/hypertable.c

+		.scandirection = ForwardScanDirection,
+		.result_mctx = mctx,
+		.tuplock = {//not sure what correct values here would be
+			.waitpolicy = LockWaitBlock,


No need to set anything here if not grabbing tuple locks. But why can't this function simply use hypertable_scan_limit_internal?

erimatnor · 2018-08-08T16:19:44Z

src/utils.c

+ * set returning function for this to work.
+ */
+Datum
+srf_return_list(FunctionCallInfo fcinfo)


Do we really need this as a utility function if its not used elsewhere? I also think this obscures what is going on in the SRF functions that call this list, especially since they also have to do SRF_IS_FIRSTCALL().

Ngalstyan4

Answered @erimatnor's questions and made some clarifying comments in the code.
Will squash and rebase once we are done discussing.
Did not rebase now to preserve some of the old comments. Also will need to talk with Rob to see what I need to change in update files for 0.12.

sql/ddl_api.sql

Ngalstyan4 · 2018-08-14T13:41:18Z

sql/ddl_internal.sql

+-- specifying the caller name. This makes it easier to taylor
+-- error messages to the caller function context.
+CREATE OR REPLACE FUNCTION _timescaledb_internal.show_chunks_impl(
+    hypertable_name  REGCLASS = NULL,


I also noticed the discrepancy.
The reason I went with this was to be compatible with drop_chunks naming.
I think it would be ideal fro the two to have identical APIs so any call of drop_chunks could be refactored into a call of show_chunks by only changing function name.
I am happy to change this to hypertable or hypertable_regclass if you think that is better.
Could also add a drop_chunks implementation with similar API to provide interoperability

Ngalstyan4 · 2018-08-14T13:44:12Z

test/sql/chunk_utils.sql

+-- Note that currently this failure is triggered by SQL typecheker when it tries and fails to resolve
+-- ANYELEMENT args to a concrete type. Explicit error checking may be necessary if drop_chunks implementation
+-- is fully ported to C.
+SELECT drop_chunks();


Test to make sure drop_chunks without args does not succeed. Discussed this above

Ngalstyan4 · 2018-08-14T13:52:41Z

src/chunk.c

@@ -833,6 +852,14 @@ set_complete_chunk(ChunkScanCtx *scanctx, Chunk *chunk)
 	return false;
 }

+static bool
+chunk_scan_context_add_chunk(ChunkScanCtx *scanctx, Chunk *chunk)


RE(erik):

Why is this function name set_all_chunks although only one chunk is set?
chunk_scan_context_append_chunk() seems like more accurate name.
Not even sure why this function is necessary since the chunks are already in the scan context and you can iterate the context similar to how you iterate a list?

What do you mean chunks are already in the scan context?
I could not find a direct way of extracting them and anything I tried involved quite some code repetition. Looks like to do it directly, I would need to repeat a lot of chunk_scan_ctx_foreach_chunk code which includes quite some boilerplate.

What I meant was that the scan context already contains the chunks you need, so instead of returning a new "container" of the chunks "extracted" from the context, just return the context. So, you would do like follows:

For each hypertable:

Scan for chunks -> return scan context

Add the size of the scan context (# hash table entries) to a cumulative num_chunks count.

Add scan context to a list or array of size num_hypertables

Once done:

Allocate a Chunk * array of size num_chunks

Iterate each scan context and add each chunk to the Chunk * array

Sort the Chunk * array

This would avoid the boilerplate code needed for the Chunks container and avoid a lot of repallocs

Ngalstyan4 · 2018-08-14T15:32:16Z

src/chunk.c

+ * set returning function for this to work.
+ */
+static Datum
+chunks_return_srf(FunctionCallInfo fcinfo)


RE:

Do we really need this as a utility function if its not used elsewhere? I also think this obscures what is going on in the SRF functions that call this list, especially since they also have to do SRF_IS_FIRSTCALL().

refactored and made static
having this exposed made even less sense when it became so Chunk-specific

Ngalstyan4 · 2018-08-14T15:40:18Z

src/chunk.c

+			ereport(ERROR,
+					(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+					 errmsg("When calling the internal function for show_chunks "
+							"caller_name cannot be null")


should I error here?
I can just add a check and consider this to be same "show_chunks" default case as above?
This C function is registered as 2 SQL functions. when external SQL show_chunks is called, only 3 arguments are passed and this case never arises. This will arise iff the function is called in C with 4 args and the last one is set to NULL explicitly.

Ngalstyan4 · 2018-08-14T15:43:42Z

src/chunk.c

+	Chunk			v1 = *((const Chunk *) ch1);
+	Chunk			v2 = *((const Chunk *) ch2);
+
+	if (v1.fd.hypertable_id < v2.fd.hypertable_id)


Should this be hypertable id or hypertable oid?
@erimatnor in your initial review comment you said hypertable_oid but I thought id would make more sense.

Initially, @cevian suggested to just order everything by chunk oid without considering hypertables since it gives them in creation time order. Just something to think about when deciding the final sort order.

src/chunk.h

Ngalstyan4 · 2018-08-14T15:58:03Z

src/chunk.c

+
+	if (SRF_IS_FIRSTCALL())
+	{
+		MemoryContext oldcontext;


This if does 3 main things

declare and populate local variables from passed arguments

typecheck all these variables

for each hypertable call other functions to get chunks

first two take most of the space. the second cannot easily be moved to a different function because each paragraph of code that does checking works with >7 variables from current context and it would not be ideal to pass all these variables around.

One option would be to pass fcinfo of current context to another function and do all the checking there but postgres C function definition guide recommends against this style.
I just though there is nothing complex in this block and most of it is routine typechecking and breaking it up might make it less readable so was not sure how to change.

I did get rid of some list->array ->list conversions which made this slightly shorter.

Ngalstyan4 · 2018-08-14T16:13:42Z

src/chunk.c

+		}
+		else
+		{
+			ht = hypertable_cache_get_entry(hypertable_cache, table_relid);


RE:

You can do hypertable_get_by_name here to avoid the cache altogether.

I am not sure I can do that because the function, unlike drop_chunks takes regclass hypertable oid and not schema NAME and table NAME.

FWIW, you can easily get the relation name and schema from the relid (e.g., get_rel_name).

alanhamlett · 2018-09-23T17:35:10Z

What's the status of this PR? I'm interested in using newer_than with show_chunks and drop_chunks to facilitate exporting and archiving ranges of data.

cevian

Hey Narek, still some stuff to do but I think it's coming along.

sql/ddl_internal.sql

src/chunk.c

src/chunk.h

src/utils.h

src/chunk.c

cevian · 2018-11-03T20:57:27Z

src/chunk.c

+		foreach(lc, hypertables)
+		{
+			ht = lfirst(lc);
+


could we break out line 1190 to line 1245 into a separate function? Another word it would return chunks_added based on a hypertable, time_dimension, older_than, older_than_type, newer_than, newer_than_type, funcctx->multi_call_memory_ctx.

Ok, I factored it out in a separate function. Let me know what you think. That function takes a lot of arguments and really is useful only if arguments depend directly on user input. I am not sure it would be reusable in other contexts

amytai · 2018-11-05T15:23:53Z

src/chunk.c

+
+	if (SRF_IS_FIRSTCALL())
+	{
+		MemoryContext oldcontext;


I think you can (and should) put the main logic inside this if in a separate function. You can do all the argument checking before passing them into a function. In particular, that separate function will be reusable (I've already refactored it myself in a C implementation of drop_chunks). It's true that the function will take something like 7 variables, but if it is reusable and makes for more readable code, I don't think that matters.

src/chunk.c

Ngalstyan4

Addressed the comments above and added some comments that I would like response to.
Left my individual commits in case you would like to see the diffs between them but will squash before merge.

Ngalstyan4 · 2018-11-05T21:55:06Z

sql/CMakeLists.txt

@@ -82,6 +82,7 @@ set(MOD_FILES
  updates/1.0.0-rc2--1.0.0-rc3.sql
  updates/1.0.0-rc3--1.0.0.sql
  updates/1.0.0--1.0.1-dev.sql
+  updates/1.0.1--1.1.0.sql


Not sure which update file these will end up in but looks like it will not be 1.0.*

Ngalstyan4 · 2018-11-05T21:56:35Z

src/compat.c

+ */
+/* qsort comparison function for Oids */
+int
+oid_cmp(const void *p1, const void *p2)


The current implementation of chunk sorting uses their hypertable and chunk IDs to sort. So oid_cmp is in fact not used as comparator. I can remove this if you think it is not likely to be useful later on.

This is still the case.

Ngalstyan4 · 2018-11-05T21:59:22Z

src/chunk.c

+		foreach(lc, hypertables)
+		{
+			ht = lfirst(lc);
+


Ok, I factored it out in a separate function. Let me know what you think. That function takes a lot of arguments and really is useful only if arguments depend directly on user input. I am not sure it would be reusable in other contexts

Ngalstyan4 · 2018-11-05T22:01:21Z

src/chunk.c

+		{
+			MemoryContext oldcontext = MemoryContextSwitchTo(funcctx->multi_call_memory_ctx);
+
+			chunks = chunks_alloc(0);


I just added this. Previously, client crashed if there were no hypertables and show_chunks was called.
As an alternative, it would make sense to error in here instead of returning 0 rows. That is what, for example \dt does in psql. Let me know what you think. @cevian

erimatnor · 2018-08-16T14:51:30Z

sql/ddl_internal.sql

+-- specifying the caller name. This makes it easier to taylor
+-- error messages to the caller function context.
+CREATE OR REPLACE FUNCTION _timescaledb_internal.show_chunks_impl(
+    hypertable_name  REGCLASS = NULL,


I prefer hypertable. Regarding drop_chunks, it is using two NAME parameters as opposed to a REGCLASS so there _namesuffix is appropriate. Still, I think that drop_chunks should probably take a REGCLASS instead. I don't think there's a good reason why it now takes two NAME params (apart from being legacy and probably my fault).

We should discuss changing drop_chunks to use REGCLASS and I think for most use-cases it would be safe since with both old and new function you can do:

drop_chunks(now() - '1 day', 'mytable');

And only in case of specifying a schema in the old way it would break.

erimatnor · 2018-11-13T12:57:25Z

src/chunk.c

+		}
+		else
+		{
+			ht = hypertable_cache_get_entry(hypertable_cache, table_relid);


FWIW, you can easily get the relation name and schema from the relid (e.g., get_rel_name).

erimatnor · 2018-11-13T13:07:39Z

src/chunk.c

@@ -833,6 +852,14 @@ set_complete_chunk(ChunkScanCtx *scanctx, Chunk *chunk)
 	return false;
 }

+static bool
+chunk_scan_context_add_chunk(ChunkScanCtx *scanctx, Chunk *chunk)


What I meant was that the scan context already contains the chunks you need, so instead of returning a new "container" of the chunks "extracted" from the context, just return the context. So, you would do like follows:

For each hypertable:

Scan for chunks -> return scan context

Add the size of the scan context (# hash table entries) to a cumulative num_chunks count.

Add scan context to a list or array of size num_hypertables

Once done:

Allocate a Chunk * array of size num_chunks

Iterate each scan context and add each chunk to the Chunk * array

Sort the Chunk * array

This would avoid the boilerplate code needed for the Chunks container and avoid a lot of repallocs

erimatnor · 2018-11-13T13:13:55Z

src/chunk.c

+static List *
+chunk_scan_ctx_get_all_chunks(ChunkScanCtx *ctx)
+{
+	ctx->data = chunks_alloc(CHUNKS_DEFAULT_CAPACITY);


Here you can just allocate with the number of entries in the hash table within the scan context. I am not sure you even need the expandable Chunks container. I think you might be able to get away with a fixed size Chunk * array without all the boilerplate code for the Chunks type. See also my comment above about this.

Ngalstyan4

Got rid of expanding Chunk-array data structure and made sure all allocations are of predetermined size.

Ngalstyan4 · 2018-11-26T03:15:32Z

src/chunk.c

+	 * num_chunks can safely be 0 as palloc protects against unportable
+	 * behavior.
+	 */
+	chunks = palloc(sizeof(Chunk *) * num_chunks);


Should I allocate for Chunk* pointers here? Note that this is called after hash context destroy. It looks like hash context destroy only affects ctx->htab and this allocation works so Chunks are not freed.
But I am not sure where exactly are Chunks structs freed. There is a function for doing that but is never called.
@erimatnor just wanted to draw your attention on this to make sure this is not going to result in some kind of leak.

There is the option of actually allocating Chunk structs as opposed to pointers and the behavior in that case will definitely be clearer to understand from the code. However, Chunks are pretty big.
On the other hand, we could make it an array of Oids and it would actually simplify and generalize the chunks_return_srf function. But this becomes problematic if we want to sort the oids in the order defined by chunk_cmp as it uses other info from Chunk for sorting.

Ngalstyan4 · 2018-11-26T03:21:02Z

src/chunk.c

+	{
+		/* Get all the chunks from the context */
+		ctxs[i]->data = current;
+		chunk_scan_ctx_foreach_chunk(ctxs[i], chunk_scan_context_add_chunk, -1);


hypertables already come in sorted order so we could technically do this instead of sorting everything later
qsort(current, (Chunk **) ctxs[i]->data - current, sizeof(Chunk *), chunk_cmp);

Not sure it is worth it. Also silently breaks if someone changes ordering of hypertables.

Fwiw, if you'd do a partial sort, I a further optimization is probably to combine that with a lazy scan of chunks. So, on firstcall you just scan for hypertables, then in chunks_return_srf you lazily scan for chunks for each hypertable as needed. This would reduce memory usage as you only keep one per-hypertable chunks array at a time and you need not scan some chunks in case of a LIMIT clause.

Just suggesting as feedback, but I don't think it is required that you do that now.

Ngalstyan4 · 2018-11-26T03:25:19Z

src/compat.c

+ */
+/* qsort comparison function for Oids */
+int
+oid_cmp(const void *p1, const void *p2)


This is still the case.

Ngalstyan4 · 2018-11-26T03:29:10Z

test/expected/chunk_utils.out

 CREATE VIEW dependent_view AS SELECT * FROM _timescaledb_internal._hyper_1_1_chunk;
 \set ON_ERROR_STOP 0
 SELECT drop_chunks(2);
 ERROR:  cannot drop table _timescaledb_internal._hyper_1_1_chunk because other objects depend on it
 SELECT drop_chunks(NULL::interval);
-ERROR:  can only use drop_chunks with an INTERVAL for TIMESTAMP, TIMESTAMPTZ, and DATE types
+ERROR:  older_than and newer_than timestamps provided to drop_chunks cannot both be NULL


@erimatnor @svenklemm note that the error messages here differ after introducing show_chunks
I think the new ones make more sense in this context but just wanted to get your opinion on this as well.
Tests introduced at #861

@Ngalstyan4 Sure, i didnt focus on the error messages themselves only that there are tests for NULL and that NULL doesnt crash the backend or produces weird errors. Feel free to improve the wording of the error messages.

erimatnor

Approving, but added some suggestions for clarity fixes.

src/hypertable.c

src/chunk.c

erimatnor · 2018-11-27T09:09:23Z

src/chunk.c

+	{
+		/* Get all the chunks from the context */
+		ctxs[i]->data = current;
+		chunk_scan_ctx_foreach_chunk(ctxs[i], chunk_scan_context_add_chunk, -1);


Not sure it is worth it. Also silently breaks if someone changes ordering of hypertables.

src/chunk.c

erimatnor · 2018-11-27T09:16:33Z

src/chunk.c

+	{
+		/* Get all the chunks from the context */
+		ctxs[i]->data = current;
+		chunk_scan_ctx_foreach_chunk(ctxs[i], chunk_scan_context_add_chunk, -1);


Fwiw, if you'd do a partial sort, I a further optimization is probably to combine that with a lazy scan of chunks. So, on firstcall you just scan for hypertables, then in chunks_return_srf you lazily scan for chunks for each hypertable as needed. This would reduce memory usage as you only keep one per-hypertable chunks array at a time and you need not scan some chunks in case of a LIMIT clause.

Just suggesting as feedback, but I don't think it is required that you do that now.

src/chunk.c

Timescale provides an efficient and easy to use api to drop individual chunks from timescale database through drop_chunks. This PR builds on that functionality and through a new show_chunks function gives the opportunity to see the chunks that would be dropped if drop_chunks was run. Additionally, it adds a newer_than option to drop_chunks (also supported by show_chunks) that allows to see/drop chunks in an interval or newer than a point in time. This commit includes: - Implementation of show_chunks in C - Additional helper functions to work with chunks - New version of drop_chunks in sql that uses show_chunks. This also adds a newer_than option to drop_chunks - More enhanced tests of drop_chunks and new tests for show_chunks Among other reasons, show_chunks was implemented in C in order to be able to have both older_than and newer_than arguments be null. This was not possible in SQL because the arguments had to have polymorphic types and whether they are used in function body or not, PL/pgSQL requires these arguments to typecheck.

Ngalstyan4 commented Aug 6, 2018

View reviewed changes

Ngalstyan4 force-pushed the show_chunks branch 2 times, most recently from 1995bf9 to a8e7698 Compare August 6, 2018 17:51

Ngalstyan4 force-pushed the show_chunks branch 4 times, most recently from daeaf1c to 955977d Compare August 6, 2018 20:15

Ngalstyan4 commented Aug 6, 2018

View reviewed changes

Ngalstyan4 force-pushed the show_chunks branch 3 times, most recently from a02defd to 9e9c00e Compare August 6, 2018 20:38

Ngalstyan4 commented Aug 6, 2018

View reviewed changes

Ngalstyan4 force-pushed the show_chunks branch 4 times, most recently from 84f42fa to 630971f Compare August 6, 2018 21:38

Ngalstyan4 commented Aug 6, 2018

View reviewed changes

erimatnor requested changes Aug 8, 2018

View reviewed changes

Ngalstyan4 force-pushed the show_chunks branch 2 times, most recently from ee941e3 to d3566b9 Compare August 14, 2018 15:47

Ngalstyan4 commented Aug 14, 2018

View reviewed changes

RobAtticus added this to the 1.1.0 milestone Sep 12, 2018

alanhamlett mentioned this pull request Sep 23, 2018

Proposal: support for exporting data before deleting it via drop_chunks #572

Closed

Ngalstyan4 force-pushed the show_chunks branch 2 times, most recently from eb5b133 to a4bd4bc Compare October 5, 2018 20:07

cevian requested changes Nov 3, 2018

View reviewed changes

amytai reviewed Nov 5, 2018

View reviewed changes

Ngalstyan4 force-pushed the show_chunks branch 3 times, most recently from a705ba7 to 578edb8 Compare November 5, 2018 21:47

Ngalstyan4 commented Nov 5, 2018

View reviewed changes

erimatnor requested changes Nov 13, 2018

View reviewed changes

Ngalstyan4 force-pushed the show_chunks branch 3 times, most recently from 256b549 to 3b1d846 Compare November 26, 2018 03:31

Ngalstyan4 commented Nov 26, 2018

View reviewed changes

Ngalstyan4 force-pushed the show_chunks branch from 3b1d846 to 3723a5c Compare November 26, 2018 03:46

erimatnor approved these changes Nov 27, 2018

View reviewed changes

cevian approved these changes Nov 27, 2018

View reviewed changes

Ngalstyan4 force-pushed the show_chunks branch from 3723a5c to 37fc267 Compare November 28, 2018 16:50

cevian merged commit 9a34028 into timescale:master Nov 28, 2018

		@@ -0,0 +1 @@
		DROP FUNCTION IF EXISTS _timescaledb_internal.dimension_get_time(integer);

Implement show_chunks in C and have drop_chunks use it #642

Implement show_chunks in C and have drop_chunks use it #642

Conversation

Ngalstyan4 commented Aug 6, 2018

Ngalstyan4 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Aug 6, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ngalstyan4 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erimatnor Nov 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ngalstyan4 Aug 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alanhamlett commented Sep 23, 2018

cevian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ngalstyan4 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erimatnor Nov 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ngalstyan4 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erimatnor left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Aug 6, 2018 •

edited

Loading

erimatnor Nov 13, 2018 •

edited

Loading

Ngalstyan4 Aug 14, 2018 •

edited

Loading

erimatnor Nov 13, 2018 •

edited

Loading

erimatnor left a comment •

edited

Loading